fix extension free-text advanced GET-q query not split by comma

[fmigneault]

Related Issue(s):

• n/a

Description:

Unlike its basic counterpart, the q parameter of advanced free-text extension was passed as is (plain string) to the downstream client. This lead to invalid searches.

This PR fixes this by applying the same transform, which leads to the expected behavior.

Note

Both "test_search_free_text_advanced" tests fail currently, but it doesn't seem to make much sense that the literal inputs are returned as is in response.json() == "+ocean,-coast", given that the client expects q: List[str] (https://github.com/stac-utils/stac-fastapi-pgstac/blob/d33b0f3d1d509e16fe0dfc765dc47aea6613aac8/stac_fastapi/pgstac/core.py#L57). Should those tests be adjusted?

stac_fastapi/extensions/tests/test_free_text.py:302: AssertionError
======================================================================================================================================= short test summary info =======================================================================================================================================
FAILED stac_fastapi/extensions/tests/test_free_text.py::test_search_free_text_search_advanced - AssertionError: assert ['+ocean', '-coast'] == '+ocean,-coast'
FAILED stac_fastapi/extensions/tests/test_free_text.py::test_search_free_text_advanced_complete - AssertionError: assert ['ocean', 'coast'] == 'ocean,coast'

PR Checklist:

• pre-commit hooks pass locally
• Tests pass (run make test)
• Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
• Changes are added to the CHANGELOG.

[vincentsarago]

This was made on purpose see #655 (comment)

advanced might get removed anyway stac-api-extensions/freetext-search#13

cc @rhysrevans3

[fmigneault]

@vincentsarago
Thanks for the heads up!
Is this removal sure to make it in? I will remove it and revert to basic free-text in our implementation if so.

What would be the recommended approach to do the +/- of Advanced FreeText-Search instead? For users, this seemed like a convenient way to filter-out items without involving "more complex" CQL2, which is why we were considering enabling it.

[vincentsarago]

What would be the recommended approach to do the +/- of Advanced FreeText-Search instead? For users, this seemed like a convenient way to filter-out items without involving "more complex" CQL2, which is why we were considering enabling it.

I can see the appeal for advanced free-text but this is lacking an official (and complete) specification (on OGC or STAC side)

going back to the original issue, in stac-fastapi-pgstac we use string anyway https://github.com/stac-utils/stac-fastapi-pgstac/blob/main/stac_fastapi/pgstac/core.py#L598-L599 👀

FYI: I also think that free-text is not recommended for items search in pgstac

[fmigneault]

going back to the original issue, in stac-fastapi-pgstac we use string anyway https://github.com/stac-utils/stac-fastapi-pgstac/blob/main/stac_fastapi/pgstac/core.py#L598-L599

Indeed, but the " OR ".join(q) expects that its input is an Iterable[str]. If it is a str directly, which is itself an Iterable[str], it ends up in the result shown in my example (i.e. "EuroSAT" becomes "E OR u OR r OR o OR S OR A OR T"). A pre-wrapping as ["EuroSAT"] is needed for correct interpretation of the word.

In the case of basic free-text, this list wrapping is applied using the converter here:

stac-fastapi/stac_fastapi/extensions/stac_fastapi/extensions/core/free_text/request.py

Lines 25 to 26 in 2c8c071

	if val:
	return val.split(",")

However, the advanced free-text that goes through the same code-path did not use the converter, so it ends up as plain string in the " OR ".join(q) call.

[rhysrevans3]

My plan was to move rather than remove advanced search mainly to give the user more clarity on the expected behaviour of the extension. @fmigneault it would be good to get the opinion of someone who is using the advanced search on where it should be moved to (if it is moved) stac-api-extensions/freetext-search#13

[vincentsarago]

@fmigneault I see

So I think the issue is that FreeTextAdvancedExtensionGetRequest should return a str (not List[str]) so we should remove the converter.

Then we will need a fix in stac-fastapi-pgstac to handle str not only List[str]

[fmigneault]

@rhysrevans3
Since there is a q parameter conflict between the basic/advanced, I think it is actually clearer if both are together as currently defined. This makes it clear that implementer have to pick one or the other, and users can better understand their differences. I think that moving it will increase the chances of user being unaware of the advanced variant if they land only on the basic description, or they might get lost amongst all other STAC extensions and conformance links.

@vincentsarago
Yes, that works also.
I think part of the issue is that it is not clear at which level the parsing should occur right now.
Currently, comma-word splitting happens while validating the parameter, but OR joining happens in pgstac. If the str is returned directly, does that mean the comma-word split would happen in pgstac as well for basic search, or that means pgstac will deal with List[str] | str directly for basic/advanced respectively?

[vincentsarago]

FYI: stac-utils/stac-fastapi-pgstac#263

if/when merged, I'll back port the change and release a 5.0.3 with this change