From 4cb41f65bb3d78c0d7ee71fc1916bc2089479d27 Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 15:21:59 +0800 Subject: [PATCH 1/7] use minimum should match --- .../sfeos_helpers/filter/transform.py | 9 +++- stac_fastapi/tests/extensions/test_filter.py | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py index c78b19e5..c0265979 100644 --- a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py +++ b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py @@ -41,13 +41,20 @@ def to_es(queryables_mapping: Dict[str, Any], query: Dict[str, Any]) -> Dict[str LogicalOp.OR: "should", LogicalOp.NOT: "must_not", }[query["op"]] - return { + + # For OR conditions, we need to ensure at least one condition must match + bool_query: Dict[str, Any] = { "bool": { bool_type: [ to_es(queryables_mapping, sub_query) for sub_query in query["args"] ] } } + # Add minimum_should_match for OR conditions + if query["op"] == LogicalOp.OR: + bool_query["bool"]["minimum_should_match"] = 1 + + return bool_query elif query["op"] in [ ComparisonOp.EQ, diff --git a/stac_fastapi/tests/extensions/test_filter.py b/stac_fastapi/tests/extensions/test_filter.py index e54d198e..6ed78ac0 100644 --- a/stac_fastapi/tests/extensions/test_filter.py +++ b/stac_fastapi/tests/extensions/test_filter.py @@ -674,3 +674,57 @@ async def test_queryables_enum_platform( # Clean up r = await app_client.delete(f"/collections/{collection_id}") r.raise_for_status() + + +@pytest.mark.asyncio +async def test_search_filter_ext_or_condition(app_client, ctx): + """Test that OR conditions work correctly in filters.""" + # This test verifies that OR conditions work by checking for items that match + # either of two conditions where only one should match the test item + params = { + "filter": { + "op": "or", + "args": [ + { + "op": "<", + "args": [ + {"property": "eo:cloud_cover"}, + 0, # This condition should NOT match (cloud_cover is 0, not < 0) + ], + }, + { + "op": "=", + "args": [ + {"property": "properties.proj:epsg"}, + 32756, # This condition SHOULD match + ], + }, + ], + } + } + + # First verify the test item matches exactly one of the conditions + props = ctx.item.get("properties", {}) + matches = [ + props.get("eo:cloud_cover", 100) < 0, # Should be False + props.get("proj:epsg") == 32756, # Should be True + ] + assert sum(matches) == 1, "Test item should match exactly one condition" + + # Now test the API + resp = await app_client.post("/search", json=params) + assert resp.status_code == 200 + resp_json = resp.json() + + # Should find at least the test item + assert len(resp_json["features"]) >= 1 + + # Verify at least one feature matches one of the conditions + matched = False + for feature in resp_json["features"]: + props = feature.get("properties", {}) + if props.get("eo:cloud_cover", 100) < 0 or props.get("proj:epsg") == 32756: + matched = True + break + + assert matched, "No features matched the OR condition" From 206c57cc4153e38ba2fc2dd851dbe6598c88a046 Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 15:30:33 +0800 Subject: [PATCH 2/7] revert min should match --- .../stac_fastapi/sfeos_helpers/filter/transform.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py index c0265979..4caaf95b 100644 --- a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py +++ b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py @@ -50,9 +50,9 @@ def to_es(queryables_mapping: Dict[str, Any], query: Dict[str, Any]) -> Dict[str ] } } - # Add minimum_should_match for OR conditions - if query["op"] == LogicalOp.OR: - bool_query["bool"]["minimum_should_match"] = 1 + # # Add minimum_should_match for OR conditions + # if query["op"] == LogicalOp.OR: + # bool_query["bool"]["minimum_should_match"] = 1 return bool_query From 7a60ce9d765ca653e877d35ab0ef5fe783d06c5e Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 15:40:16 +0800 Subject: [PATCH 3/7] use must clause in test --- stac_fastapi/tests/extensions/test_filter.py | 53 +++++++------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/stac_fastapi/tests/extensions/test_filter.py b/stac_fastapi/tests/extensions/test_filter.py index 6ed78ac0..2cda1913 100644 --- a/stac_fastapi/tests/extensions/test_filter.py +++ b/stac_fastapi/tests/extensions/test_filter.py @@ -677,54 +677,39 @@ async def test_queryables_enum_platform( @pytest.mark.asyncio -async def test_search_filter_ext_or_condition(app_client, ctx): - """Test that OR conditions work correctly in filters.""" - # This test verifies that OR conditions work by checking for items that match - # either of two conditions where only one should match the test item +async def test_search_filter_ext_or_with_must_condition(app_client, ctx): + """Test that OR conditions work correctly when combined with MUST conditions.""" + # This test verifies that when combining MUST and SHOULD clauses, + # we still require at least one SHOULD condition to match params = { "filter": { - "op": "or", + "op": "and", "args": [ + # MUST condition (matches all test items) + {"op": ">=", "args": [{"property": "eo:cloud_cover"}, 0]}, + # OR condition (only some items match) { - "op": "<", - "args": [ - {"property": "eo:cloud_cover"}, - 0, # This condition should NOT match (cloud_cover is 0, not < 0) - ], - }, - { - "op": "=", + "op": "or", "args": [ - {"property": "properties.proj:epsg"}, - 32756, # This condition SHOULD match + {"op": "<", "args": [{"property": "eo:cloud_cover"}, 10]}, + { + "op": "=", + "args": [{"property": "properties.proj:epsg"}, 99999], + }, ], }, ], } } - # First verify the test item matches exactly one of the conditions - props = ctx.item.get("properties", {}) - matches = [ - props.get("eo:cloud_cover", 100) < 0, # Should be False - props.get("proj:epsg") == 32756, # Should be True - ] - assert sum(matches) == 1, "Test item should match exactly one condition" - - # Now test the API + # Test the API resp = await app_client.post("/search", json=params) assert resp.status_code == 200 resp_json = resp.json() - # Should find at least the test item - assert len(resp_json["features"]) >= 1 - - # Verify at least one feature matches one of the conditions - matched = False + # Should only find items where cloud_cover < 10 (second condition is false for all) for feature in resp_json["features"]: props = feature.get("properties", {}) - if props.get("eo:cloud_cover", 100) < 0 or props.get("proj:epsg") == 32756: - matched = True - break - - assert matched, "No features matched the OR condition" + assert ( + props.get("eo:cloud_cover", 100) < 10 + ), "Items should only be returned if they match at least one OR condition" From 45053d0a70cf94712e5226b9474e81141ee79743 Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 15:56:38 +0800 Subject: [PATCH 4/7] update test --- stac_fastapi/tests/extensions/test_filter.py | 48 +++++++++++++------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/stac_fastapi/tests/extensions/test_filter.py b/stac_fastapi/tests/extensions/test_filter.py index 2cda1913..b97b7fd6 100644 --- a/stac_fastapi/tests/extensions/test_filter.py +++ b/stac_fastapi/tests/extensions/test_filter.py @@ -678,38 +678,52 @@ async def test_queryables_enum_platform( @pytest.mark.asyncio async def test_search_filter_ext_or_with_must_condition(app_client, ctx): - """Test that OR conditions work correctly when combined with MUST conditions.""" - # This test verifies that when combining MUST and SHOULD clauses, - # we still require at least one SHOULD condition to match + """ + Test that OR conditions require at least one match when combined with MUST. + This test will fail if minimum_should_match=1 is not set in the ES query. + """ + # Case 1: At least one OR condition matches (should return the item) params = { "filter": { "op": "and", "args": [ - # MUST condition (matches all test items) - {"op": ">=", "args": [{"property": "eo:cloud_cover"}, 0]}, - # OR condition (only some items match) + { + "op": ">=", + "args": [{"property": "eo:cloud_cover"}, 0], + }, # True for test item (cloud_cover=0) { "op": "or", "args": [ - {"op": "<", "args": [{"property": "eo:cloud_cover"}, 10]}, + { + "op": "<", + "args": [{"property": "eo:cloud_cover"}, 1], + }, # True for test item (cloud_cover=0) { "op": "=", "args": [{"property": "properties.proj:epsg"}, 99999], - }, + }, # False ], }, ], } } - - # Test the API resp = await app_client.post("/search", json=params) assert resp.status_code == 200 resp_json = resp.json() - - # Should only find items where cloud_cover < 10 (second condition is false for all) - for feature in resp_json["features"]: - props = feature.get("properties", {}) - assert ( - props.get("eo:cloud_cover", 100) < 10 - ), "Items should only be returned if they match at least one OR condition" + assert any( + f["properties"].get("eo:cloud_cover") == 0 for f in resp_json["features"] + ), "Should return the test item when at least one OR condition matches" + + # Case 2: No OR conditions match (should NOT return the item if minimum_should_match=1 is set) + params["filter"]["args"][1]["args"][0]["args"][ + 1 + ] = -1 # Now: cloud_cover < -1 (False) + params["filter"]["args"][1]["args"][1]["args"][ + 1 + ] = 99998 # Now: proj:epsg == 99998 (False) + resp = await app_client.post("/search", json=params) + assert resp.status_code == 200 + resp_json = resp.json() + assert all( + f["properties"].get("eo:cloud_cover") != 0 for f in resp_json["features"] + ), "Should NOT return the test item when no OR conditions match (requires minimum_should_match=1)" From 9ac446515483af89be554ab8e76ddf8abb4319be Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 16:11:03 +0800 Subject: [PATCH 5/7] update test --- stac_fastapi/tests/extensions/test_filter.py | 75 ++++++++++++++------ 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/stac_fastapi/tests/extensions/test_filter.py b/stac_fastapi/tests/extensions/test_filter.py index b97b7fd6..e222a452 100644 --- a/stac_fastapi/tests/extensions/test_filter.py +++ b/stac_fastapi/tests/extensions/test_filter.py @@ -677,31 +677,58 @@ async def test_queryables_enum_platform( @pytest.mark.asyncio -async def test_search_filter_ext_or_with_must_condition(app_client, ctx): +async def test_search_filter_ext_or_with_must_condition(app_client, load_test_data): """ Test that OR conditions require at least one match when combined with MUST. - This test will fail if minimum_should_match=1 is not set in the ES query. + This test will fail if minimum_should_match=1 is not set in the ES/OS query. """ - # Case 1: At least one OR condition matches (should return the item) + # Arrange: Create a unique collection for this test + collection_data = load_test_data("test_collection.json") + collection_id = collection_data["id"] = f"or-test-collection-{uuid.uuid4()}" + r = await app_client.post("/collections", json=collection_data) + r.raise_for_status() + + # Add three items: + # 1. Matches both must and should + # 2. Matches must but not should + # 3. Matches neither + items = [ + { + "eo:cloud_cover": 0, + "proj:epsg": 32756, + }, # Should be returned when should matches + { + "eo:cloud_cover": 5, + "proj:epsg": 88888, + }, # Should NOT be returned if min_should_match=1 + {"eo:cloud_cover": -5, "proj:epsg": 99999}, # Should not be returned at all + ] + for idx, props in enumerate(items): + item_data = load_test_data("test_item.json") + item_data["id"] = f"or-test-item-{idx}" + item_data["collection"] = collection_id + item_data["properties"]["eo:cloud_cover"] = props["eo:cloud_cover"] + item_data["properties"]["proj:epsg"] = props["proj:epsg"] + r = await app_client.post(f"/collections/{collection_id}/items", json=item_data) + r.raise_for_status() + + # Case 1: At least one OR condition matches (should return only the first item) params = { "filter": { "op": "and", "args": [ - { - "op": ">=", - "args": [{"property": "eo:cloud_cover"}, 0], - }, # True for test item (cloud_cover=0) + {"op": ">=", "args": [{"property": "eo:cloud_cover"}, 0]}, { "op": "or", "args": [ { "op": "<", "args": [{"property": "eo:cloud_cover"}, 1], - }, # True for test item (cloud_cover=0) + }, # Only first item matches { "op": "=", - "args": [{"property": "properties.proj:epsg"}, 99999], - }, # False + "args": [{"property": "proj:epsg"}, 32756], + }, # Only first item matches ], }, ], @@ -709,21 +736,29 @@ async def test_search_filter_ext_or_with_must_condition(app_client, ctx): } resp = await app_client.post("/search", json=params) assert resp.status_code == 200 - resp_json = resp.json() + features = resp.json()["features"] assert any( - f["properties"].get("eo:cloud_cover") == 0 for f in resp_json["features"] - ), "Should return the test item when at least one OR condition matches" + f["properties"].get("eo:cloud_cover") == 0 for f in features + ), "Should return the item where at least one OR condition matches" + assert all( + f["properties"].get("eo:cloud_cover") == 0 for f in features + ), "Should only return the item matching the should clause" - # Case 2: No OR conditions match (should NOT return the item if minimum_should_match=1 is set) + # Case 2: No OR conditions match (should NOT return the second item if minimum_should_match=1 is set) params["filter"]["args"][1]["args"][0]["args"][ 1 - ] = -1 # Now: cloud_cover < -1 (False) + ] = -10 # cloud_cover < -10 (false for all) params["filter"]["args"][1]["args"][1]["args"][ 1 - ] = 99998 # Now: proj:epsg == 99998 (False) + ] = 12345 # proj:epsg == 12345 (false for all) resp = await app_client.post("/search", json=params) assert resp.status_code == 200 - resp_json = resp.json() - assert all( - f["properties"].get("eo:cloud_cover") != 0 for f in resp_json["features"] - ), "Should NOT return the test item when no OR conditions match (requires minimum_should_match=1)" + features = resp.json()["features"] + assert len(features) == 0, ( + "Should NOT return items that match only the must clause when no OR conditions match " + "(requires minimum_should_match=1)" + ) + + # Clean up + r = await app_client.delete(f"/collections/{collection_id}") + r.raise_for_status() From a4720d22533e503a747c76615aecbeec9eb1dd2c Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 16:25:46 +0800 Subject: [PATCH 6/7] min should match, refresh --- .../stac_fastapi/sfeos_helpers/filter/transform.py | 6 +++--- stac_fastapi/tests/extensions/test_filter.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py index 4caaf95b..c0265979 100644 --- a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py +++ b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py @@ -50,9 +50,9 @@ def to_es(queryables_mapping: Dict[str, Any], query: Dict[str, Any]) -> Dict[str ] } } - # # Add minimum_should_match for OR conditions - # if query["op"] == LogicalOp.OR: - # bool_query["bool"]["minimum_should_match"] = 1 + # Add minimum_should_match for OR conditions + if query["op"] == LogicalOp.OR: + bool_query["bool"]["minimum_should_match"] = 1 return bool_query diff --git a/stac_fastapi/tests/extensions/test_filter.py b/stac_fastapi/tests/extensions/test_filter.py index e222a452..4a09f29c 100644 --- a/stac_fastapi/tests/extensions/test_filter.py +++ b/stac_fastapi/tests/extensions/test_filter.py @@ -677,11 +677,21 @@ async def test_queryables_enum_platform( @pytest.mark.asyncio -async def test_search_filter_ext_or_with_must_condition(app_client, load_test_data): +async def test_search_filter_ext_or_with_must_condition( + app_client, + load_test_data, + monkeypatch: pytest.MonkeyPatch, +): """ Test that OR conditions require at least one match when combined with MUST. This test will fail if minimum_should_match=1 is not set in the ES/OS query. """ + + # Arrange + # Enforce instant database refresh + # TODO: Is there a better way to do this? + monkeypatch.setenv("DATABASE_REFRESH", "true") + # Arrange: Create a unique collection for this test collection_data = load_test_data("test_collection.json") collection_id = collection_data["id"] = f"or-test-collection-{uuid.uuid4()}" From 8f21ddbce49a09ad61a0b9e2a6f3a3382032f634 Mon Sep 17 00:00:00 2001 From: jonhealy1 Date: Mon, 9 Jun 2025 19:04:10 +0800 Subject: [PATCH 7/7] re revert --- .../stac_fastapi/sfeos_helpers/filter/transform.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py index c0265979..4caaf95b 100644 --- a/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py +++ b/stac_fastapi/sfeos_helpers/stac_fastapi/sfeos_helpers/filter/transform.py @@ -50,9 +50,9 @@ def to_es(queryables_mapping: Dict[str, Any], query: Dict[str, Any]) -> Dict[str ] } } - # Add minimum_should_match for OR conditions - if query["op"] == LogicalOp.OR: - bool_query["bool"]["minimum_should_match"] = 1 + # # Add minimum_should_match for OR conditions + # if query["op"] == LogicalOp.OR: + # bool_query["bool"]["minimum_should_match"] = 1 return bool_query