revert shortcut for ids limit, add tests (#212)

bitner · web-flow · commit e3b063523908 · 2023-09-21T10:33:45.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [unreleased]
 
+### Fixed
+
+- Revert an optimisation which limited the number of results from a search query to the number of item IDs specified in the query.
+This fixes an issue where items with the same ID that are in multiple collections could be left out of search results.
+
 ### Changed
 
 - update `pydantic` requirement to `~=2.0`
@@ -16,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Add private jsonb column to items and collections table to hold private metadata that should not be returned as part of a stac item
 - Add generated columns to collections with the bounding box as a geometry and the datetime and end_datetime from the extents (this is to help with forthcoming work on collections search)
 - Add PLRust to the Docker postgres image for forthcoming work to add optional PLRust functions for expensive json manipulation (including hydration)
+- Remove default queryable for eo:cloud_cover
 
 ## [v0.7.10]
 
diff --git a/docker/pypgstac/bin/test b/docker/pypgstac/bin/test
@@ -26,6 +26,15 @@ ALTER DATABASE pgstac_test_db_template SET CLIENT_MIN_MESSAGES TO WARNING;
 ALTER DATABASE pgstac_test_db_template SET SEARCH_PATH to pgstac, public;
 \connect pgstac_test_db_template;
 \i pgstac.sql
+DO \$\$
+BEGIN
+    INSERT INTO queryables (name, definition, property_wrapper, property_index_type) VALUES
+    ('eo:cloud_cover','{"$ref": "https://stac-extensions.github.io/eo/v1.0.0/schema.json#/definitions/fieldsproperties/eo:cloud_cover"}','to_int','BTREE')
+    ON CONFLICT DO NOTHING;
+EXCEPTION WHEN unique_violation THEN
+    RAISE NOTICE '%', SQLERRM USING ERRCODE = SQLSTATE;
+END
+\$\$;
 EOSQL
 }
 
@@ -61,6 +70,7 @@ psql -X -q -v ON_ERROR_STOP=1 <<EOSQL
 DROP DATABASE IF EXISTS pgstac_test_pgtap WITH (force);
 CREATE DATABASE pgstac_test_pgtap TEMPLATE $TEMPLATEDB;
 ALTER DATABASE pgstac_test_pgtap SET client_min_messages to $CLIENTMESSAGES;
+
 EOSQL
 TESTOUTPUT=$(psql -X -q -v ON_ERROR_STOP=1 -f $PGSTACDIR/tests/pgtap.sql pgstac_test_pgtap)
 psql -X -q -v ON_ERROR_STOP=1 <<EOSQL
diff --git a/src/pgstac/migrations/pgstac.0.7.10-unreleased.sql b/src/pgstac/migrations/pgstac.0.7.10-unreleased.sql
@@ -257,6 +257,165 @@ AS $function$
 $function$
 ;
 
+CREATE OR REPLACE FUNCTION pgstac.search(_search jsonb DEFAULT '{}'::jsonb)
+ RETURNS jsonb
+ LANGUAGE plpgsql
+AS $function$
+DECLARE
+    searches searches%ROWTYPE;
+    _where text;
+    orderby text;
+    search_where search_wheres%ROWTYPE;
+    total_count bigint;
+    token record;
+    token_prev boolean;
+    token_item items%ROWTYPE;
+    token_where text;
+    full_where text;
+    init_ts timestamptz := clock_timestamp();
+    timer timestamptz := clock_timestamp();
+    hydrate bool := NOT (_search->'conf'->>'nohydrate' IS NOT NULL AND (_search->'conf'->>'nohydrate')::boolean = true);
+    prev text;
+    next text;
+    context jsonb;
+    collection jsonb;
+    out_records jsonb;
+    out_len int;
+    _limit int := coalesce((_search->>'limit')::int, 10);
+    _querylimit int;
+    _fields jsonb := coalesce(_search->'fields', '{}'::jsonb);
+    has_prev boolean := FALSE;
+    has_next boolean := FALSE;
+BEGIN
+    searches := search_query(_search);
+    _where := searches._where;
+    orderby := searches.orderby;
+    search_where := where_stats(_where);
+    total_count := coalesce(search_where.total_count, search_where.estimated_count);
+    RAISE NOTICE 'SEARCH:TOKEN: %', _search->>'token';
+    token := get_token_record(_search->>'token');
+    RAISE NOTICE '***TOKEN: %', token;
+    _querylimit := _limit + 1;
+    IF token IS NOT NULL THEN
+        token_prev := token.prev;
+        token_item := token.item;
+        token_where := get_token_filter(_search->'sortby', token_item, token_prev, FALSE);
+        RAISE LOG 'TOKEN_WHERE: % (%ms from search start)', token_where, age_ms(timer);
+        IF token_prev THEN -- if we are using a prev token, we know has_next is true
+            RAISE LOG 'There is a previous token, so automatically setting has_next to true';
+            has_next := TRUE;
+            orderby := sort_sqlorderby(_search, TRUE);
+        ELSE
+            RAISE LOG 'There is a next token, so automatically setting has_prev to true';
+            has_prev := TRUE;
+
+        END IF;
+    ELSE -- if there was no token, we know there is no prev
+        RAISE LOG 'There is no token, so we know there is no prev. setting has_prev to false';
+        has_prev := FALSE;
+    END IF;
+
+    full_where := concat_ws(' AND ', _where, token_where);
+    RAISE NOTICE 'FULL WHERE CLAUSE: %', full_where;
+    RAISE NOTICE 'Time to get counts and build query %', age_ms(timer);
+    timer := clock_timestamp();
+
+    IF hydrate THEN
+        RAISE NOTICE 'Getting hydrated data.';
+    ELSE
+        RAISE NOTICE 'Getting non-hydrated data.';
+    END IF;
+    RAISE NOTICE 'CACHE SET TO %', get_setting_bool('format_cache');
+    RAISE NOTICE 'Time to set hydration/formatting %', age_ms(timer);
+    timer := clock_timestamp();
+    SELECT jsonb_agg(format_item(i, _fields, hydrate)) INTO out_records
+    FROM search_rows(
+        full_where,
+        orderby,
+        search_where.partitions,
+        _querylimit
+    ) as i;
+
+    RAISE NOTICE 'Time to fetch rows %', age_ms(timer);
+    timer := clock_timestamp();
+
+
+    IF token_prev THEN
+        out_records := flip_jsonb_array(out_records);
+    END IF;
+
+    RAISE NOTICE 'Query returned % records.', jsonb_array_length(out_records);
+    RAISE LOG 'TOKEN:   % %', token_item.id, token_item.collection;
+    RAISE LOG 'RECORD_1: % %', out_records->0->>'id', out_records->0->>'collection';
+    RAISE LOG 'RECORD-1: % %', out_records->-1->>'id', out_records->-1->>'collection';
+
+    -- REMOVE records that were from our token
+    IF out_records->0->>'id' = token_item.id AND out_records->0->>'collection' = token_item.collection THEN
+        out_records := out_records - 0;
+    ELSIF out_records->-1->>'id' = token_item.id AND out_records->-1->>'collection' = token_item.collection THEN
+        out_records := out_records - -1;
+    END IF;
+
+    out_len := jsonb_array_length(out_records);
+
+    IF out_len = _limit + 1 THEN
+        IF token_prev THEN
+            has_prev := TRUE;
+            out_records := out_records - 0;
+        ELSE
+            has_next := TRUE;
+            out_records := out_records - -1;
+        END IF;
+    END IF;
+
+    IF has_next THEN
+        next := concat(out_records->-1->>'collection', ':', out_records->-1->>'id');
+        RAISE NOTICE 'HAS NEXT | %', next;
+    END IF;
+
+    IF has_prev THEN
+        prev := concat(out_records->0->>'collection', ':', out_records->0->>'id');
+        RAISE NOTICE 'HAS PREV | %', prev;
+    END IF;
+
+    RAISE NOTICE 'Time to get prev/next %', age_ms(timer);
+    timer := clock_timestamp();
+
+    IF context(_search->'conf') != 'off' THEN
+        context := jsonb_strip_nulls(jsonb_build_object(
+            'limit', _limit,
+            'matched', total_count,
+            'returned', coalesce(jsonb_array_length(out_records), 0)
+        ));
+    ELSE
+        context := jsonb_strip_nulls(jsonb_build_object(
+            'limit', _limit,
+            'returned', coalesce(jsonb_array_length(out_records), 0)
+        ));
+    END IF;
+
+    collection := jsonb_build_object(
+        'type', 'FeatureCollection',
+        'features', coalesce(out_records, '[]'::jsonb),
+        'next', next,
+        'prev', prev,
+        'context', context
+    );
+
+    IF get_setting_bool('timing', _search->'conf') THEN
+        collection = collection || jsonb_build_object('timing', age_ms(init_ts));
+    END IF;
+
+    RAISE NOTICE 'Time to build final json %', age_ms(timer);
+    timer := clock_timestamp();
+
+    RAISE NOTICE 'Total Time: %', age_ms(current_timestamp);
+    RAISE NOTICE 'RETURNING % records. NEXT: %. PREV: %', collection->'context'->>'returned', collection->>'next', collection->>'prev';
+    RETURN collection;
+END;
+$function$
+;
+
 
 -- END migra calculated SQL
 DO $$
diff --git a/src/pgstac/migrations/pgstac.unreleased.sql b/src/pgstac/migrations/pgstac.unreleased.sql
@@ -3596,22 +3596,11 @@ DECLARE
     out_records jsonb;
     out_len int;
     _limit int := coalesce((_search->>'limit')::int, 10);
-    init_limit int := _limit;
     _querylimit int;
     _fields jsonb := coalesce(_search->'fields', '{}'::jsonb);
     has_prev boolean := FALSE;
     has_next boolean := FALSE;
-    items_cnt int := coalesce(jsonb_array_length(_search->'ids'),0);
 BEGIN
-    RAISE NOTICE 'Items Count: %', items_cnt;
-    IF items_cnt > 0 THEN
-        IF items_cnt <= _limit THEN
-            _limit := items_cnt - 1;
-        ELSE
-            _limit := items_cnt;
-        END IF;
-        RAISE NOTICE 'Items is set. Changing limit to %', items_cnt;
-    END IF;
     searches := search_query(_search);
     _where := searches._where;
     orderby := searches.orderby;
@@ -3683,7 +3672,7 @@ BEGIN
 
     out_len := jsonb_array_length(out_records);
 
-    IF out_len = init_limit + 1 THEN
+    IF out_len = _limit + 1 THEN
         IF token_prev THEN
             has_prev := TRUE;
             out_records := out_records - 0;
@@ -3708,13 +3697,13 @@ BEGIN
 
     IF context(_search->'conf') != 'off' THEN
         context := jsonb_strip_nulls(jsonb_build_object(
-            'limit', init_limit,
+            'limit', _limit,
             'matched', total_count,
             'returned', coalesce(jsonb_array_length(out_records), 0)
         ));
     ELSE
         context := jsonb_strip_nulls(jsonb_build_object(
-            'limit', init_limit,
+            'limit', _limit,
             'returned', coalesce(jsonb_array_length(out_records), 0)
         ));
     END IF;
diff --git a/src/pgstac/sql/004_search.sql b/src/pgstac/sql/004_search.sql
@@ -779,22 +779,11 @@ DECLARE
     out_records jsonb;
     out_len int;
     _limit int := coalesce((_search->>'limit')::int, 10);
-    init_limit int := _limit;
     _querylimit int;
     _fields jsonb := coalesce(_search->'fields', '{}'::jsonb);
     has_prev boolean := FALSE;
     has_next boolean := FALSE;
-    items_cnt int := coalesce(jsonb_array_length(_search->'ids'),0);
 BEGIN
-    RAISE NOTICE 'Items Count: %', items_cnt;
-    IF items_cnt > 0 THEN
-        IF items_cnt <= _limit THEN
-            _limit := items_cnt - 1;
-        ELSE
-            _limit := items_cnt;
-        END IF;
-        RAISE NOTICE 'Items is set. Changing limit to %', items_cnt;
-    END IF;
     searches := search_query(_search);
     _where := searches._where;
     orderby := searches.orderby;
@@ -866,7 +855,7 @@ BEGIN
 
     out_len := jsonb_array_length(out_records);
 
-    IF out_len = init_limit + 1 THEN
+    IF out_len = _limit + 1 THEN
         IF token_prev THEN
             has_prev := TRUE;
             out_records := out_records - 0;
@@ -891,13 +880,13 @@ BEGIN
 
     IF context(_search->'conf') != 'off' THEN
         context := jsonb_strip_nulls(jsonb_build_object(
-            'limit', init_limit,
+            'limit', _limit,
             'matched', total_count,
             'returned', coalesce(jsonb_array_length(out_records), 0)
         ));
     ELSE
         context := jsonb_strip_nulls(jsonb_build_object(
-            'limit', init_limit,
+            'limit', _limit,
             'returned', coalesce(jsonb_array_length(out_records), 0)
         ));
     END IF;
diff --git a/src/pgstac/tests/pgtap.sql b/src/pgstac/tests/pgtap.sql
@@ -17,7 +17,7 @@ CREATE EXTENSION IF NOT EXISTS pgtap;
 SET SEARCH_PATH TO pgstac, pgtap, public;
 
 -- Plan the tests.
-SELECT plan(191);
+SELECT plan(194);
 --SELECT * FROM no_plan();
 
 -- Run the tests.
diff --git a/src/pgstac/tests/pgtap/004_search.sql b/src/pgstac/tests/pgtap/004_search.sql
@@ -650,3 +650,23 @@ SELECT * FROM pg_temp.testpaging('asc','asc');
 SELECT * FROM pg_temp.testpaging('asc','desc');
 SELECT * FROM pg_temp.testpaging('desc','desc');
 SELECT * FROM pg_temp.testpaging('desc','asc');
+
+\copy items_staging (content) FROM 'tests/testdata/items_duplicate_ids.ndjson'
+
+SELECT is(
+    (SELECT jsonb_array_length(search('{"ids": ["pgstac-test-item-duplicated"]}')->'features')),
+    '2',
+    'Make sure all matching items are returned when items with the same ID are in multiple collections, no collections specified. #192'
+);
+
+SELECT is(
+    (SELECT jsonb_array_length(search('{"ids": ["pgstac-test-item-duplicated"], "collections": ["pgstac-test-collection"]}')->'features')),
+    '1',
+    'Make sure all matching items are returned when items with the same ID are in multiple collections, some collections specified. #192'
+);
+
+SELECT is(
+    (SELECT jsonb_array_length(search('{"ids": ["pgstac-test-item-duplicated"], "collections": ["pgstac-test-collection", "pgstac-test-collection2"]}')->'features')),
+    '2',
+    'Make sure all matching items are returned when items with the same ID are in multiple collections, all collections specified. #192'
+);
diff --git a/src/pgstac/tests/testdata/items_duplicate_ids.ndjson b/src/pgstac/tests/testdata/items_duplicate_ids.ndjson
@@ -0,0 +1,2 @@
+{"id": "pgstac-test-item-duplicated", "bbox": [-87.816179, 30.496894, -87.74637, 30.565604], "type": "Feature", "links": [], "assets": {"image": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.tif", "type": "image/tiff; application=geotiff; profile=cloud-optimized", "roles": ["data"], "title": "RGBIR COG tile", "eo:bands": [{"name": "Red", "common_name": "red"}, {"name": "Green", "common_name": "green"}, {"name": "Blue", "common_name": "blue"}, {"name": "NIR", "common_name": "nir", "description": "near-infrared"}]}, "metadata": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_fgdc_2011/30087/m_3008726_se_16_1_20110731.txt", "type": "text/plain", "roles": ["metadata"], "title": "FGDC Metdata"}, "thumbnail": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.200.jpg", "type": "image/jpeg", "roles": ["thumbnail"], "title": "Thumbnail"}}, "geometry": {"type": "Polygon", "coordinates": [[[-87.74637, 30.497308], [-87.746892, 30.565604], [-87.816179, 30.565188], [-87.815608, 30.496894], [-87.74637, 30.497308]]]}, "collection": "pgstac-test-collection", "properties": {"gsd": 1, "datetime": "2011-07-31T00:00:00Z", "naip:year": 2013, "proj:bbox": [421730, 3374130, 428375, 3381699], "proj:epsg": 26916, "providers": [{"url": "https://www.fsa.usda.gov/programs-and-services/aerial-photography/imagery-programs/naip-imagery/", "name": "USDA Farm Service Agency", "roles": ["producer", "licensor"]}], "naip:state": "zz", "proj:shape": [7569, 6645], "eo:cloud_cover": 50, "proj:transform": [1, 0, 421730, 0, -1, 3381699, 0, 0, 1]}, "stac_version": "1.0.0-beta.2", "stac_extensions": ["eo", "projection"]}
+{"id": "pgstac-test-item-duplicated", "bbox": [-87.816179, 30.496894, -87.74637, 30.565604], "type": "Feature", "links": [], "assets": {"image": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.tif", "type": "image/tiff; application=geotiff; profile=cloud-optimized", "roles": ["data"], "title": "RGBIR COG tile", "eo:bands": [{"name": "Red", "common_name": "red"}, {"name": "Green", "common_name": "green"}, {"name": "Blue", "common_name": "blue"}, {"name": "NIR", "common_name": "nir", "description": "near-infrared"}]}, "metadata": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_fgdc_2011/30087/m_3008726_se_16_1_20110731.txt", "type": "text/plain", "roles": ["metadata"], "title": "FGDC Metdata"}, "thumbnail": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.200.jpg", "type": "image/jpeg", "roles": ["thumbnail"], "title": "Thumbnail"}}, "geometry": {"type": "Polygon", "coordinates": [[[-87.74637, 30.497308], [-87.746892, 30.565604], [-87.816179, 30.565188], [-87.815608, 30.496894], [-87.74637, 30.497308]]]}, "collection": "pgstac-test-collection2", "properties": {"gsd": 1, "datetime": "2011-07-31T00:00:00Z", "naip:year": 2013, "proj:bbox": [421730, 3374130, 428375, 3381699], "proj:epsg": 26916, "providers": [{"url": "https://www.fsa.usda.gov/programs-and-services/aerial-photography/imagery-programs/naip-imagery/", "name": "USDA Farm Service Agency", "roles": ["producer", "licensor"]}], "naip:state": "zz", "proj:shape": [7569, 6645], "eo:cloud_cover": 50, "proj:transform": [1, 0, 421730, 0, -1, 3381699, 0, 0, 1]}, "stac_version": "1.0.0-beta.2", "stac_extensions": ["eo", "projection"]}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+{"id": "pgstac-test-item-duplicated", "bbox": [-87.816179, 30.496894, -87.74637, 30.565604], "type": "Feature", "links": [], "assets": {"image": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.tif", "type": "image/tiff; application=geotiff; profile=cloud-optimized", "roles": ["data"], "title": "RGBIR COG tile", "eo:bands": [{"name": "Red", "common_name": "red"}, {"name": "Green", "common_name": "green"}, {"name": "Blue", "common_name": "blue"}, {"name": "NIR", "common_name": "nir", "description": "near-infrared"}]}, "metadata": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_fgdc_2011/30087/m_3008726_se_16_1_20110731.txt", "type": "text/plain", "roles": ["metadata"], "title": "FGDC Metdata"}, "thumbnail": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.200.jpg", "type": "image/jpeg", "roles": ["thumbnail"], "title": "Thumbnail"}}, "geometry": {"type": "Polygon", "coordinates": [[[-87.74637, 30.497308], [-87.746892, 30.565604], [-87.816179, 30.565188], [-87.815608, 30.496894], [-87.74637, 30.497308]]]}, "collection": "pgstac-test-collection", "properties": {"gsd": 1, "datetime": "2011-07-31T00:00:00Z", "naip:year": 2013, "proj:bbox": [421730, 3374130, 428375, 3381699], "proj:epsg": 26916, "providers": [{"url": "https://www.fsa.usda.gov/programs-and-services/aerial-photography/imagery-programs/naip-imagery/", "name": "USDA Farm Service Agency", "roles": ["producer", "licensor"]}], "naip:state": "zz", "proj:shape": [7569, 6645], "eo:cloud_cover": 50, "proj:transform": [1, 0, 421730, 0, -1, 3381699, 0, 0, 1]}, "stac_version": "1.0.0-beta.2", "stac_extensions": ["eo", "projection"]}
	`2`	+{"id": "pgstac-test-item-duplicated", "bbox": [-87.816179, 30.496894, -87.74637, 30.565604], "type": "Feature", "links": [], "assets": {"image": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.tif", "type": "image/tiff; application=geotiff; profile=cloud-optimized", "roles": ["data"], "title": "RGBIR COG tile", "eo:bands": [{"name": "Red", "common_name": "red"}, {"name": "Green", "common_name": "green"}, {"name": "Blue", "common_name": "blue"}, {"name": "NIR", "common_name": "nir", "description": "near-infrared"}]}, "metadata": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_fgdc_2011/30087/m_3008726_se_16_1_20110731.txt", "type": "text/plain", "roles": ["metadata"], "title": "FGDC Metdata"}, "thumbnail": {"href": "https://naipeuwest.blob.core.windows.net/naip/v002/al/2011/al_100cm_2011/30087/m_3008726_se_16_1_20110731.200.jpg", "type": "image/jpeg", "roles": ["thumbnail"], "title": "Thumbnail"}}, "geometry": {"type": "Polygon", "coordinates": [[[-87.74637, 30.497308], [-87.746892, 30.565604], [-87.816179, 30.565188], [-87.815608, 30.496894], [-87.74637, 30.497308]]]}, "collection": "pgstac-test-collection2", "properties": {"gsd": 1, "datetime": "2011-07-31T00:00:00Z", "naip:year": 2013, "proj:bbox": [421730, 3374130, 428375, 3381699], "proj:epsg": 26916, "providers": [{"url": "https://www.fsa.usda.gov/programs-and-services/aerial-photography/imagery-programs/naip-imagery/", "name": "USDA Farm Service Agency", "roles": ["producer", "licensor"]}], "naip:state": "zz", "proj:shape": [7569, 6645], "eo:cloud_cover": 50, "proj:transform": [1, 0, 421730, 0, -1, 3381699, 0, 0, 1]}, "stac_version": "1.0.0-beta.2", "stac_extensions": ["eo", "projection"]}