PgSTAC: Fields extension includes links even if not listed in includes

[lossyrob]

This test currently fails:

@pytest.mark.asyncio
async def test_field_extension_includes_only_id_and_collection(
    app_client, load_test_data, load_test_collection
):
    """Test POST search including id and collection"""
    test_item = load_test_data("test_item.json")
    resp = await app_client.post(
        f"/collections/{test_item['collection']}/items", json=test_item
    )
    assert resp.status_code == 200

    body = {"fields": {"include": ["id", "collection"]}}

    resp = await app_client.post("/search", json=body)
    resp_json = resp.json()
    assert set(resp_json["features"][0].keys()) == {"id", "collection"}

with

E       AssertionError: assert {'links', 'id', 'collection'} == {'id', 'collection'}
E         Extra items in the left set:
E         'links'
E         Full diff:
E         - {'id', 'collection'}
E         + {'links', 'id', 'collection'}
E         ?  +++++++++

Links shouldn't be included in the response if they aren't in the "include" list.

[philvarner]

This is legal behavior wrt the Fields Extension -- the list of fields is only expressing a client preference rather than a response requirement. (However, I do think it's reasonable to exclude the links if they're not in the fields list, since that can be a large field.)

[duckontheweb]

It looks like this was changed in stac-utils/stac-fastapi#397 so that links are not returned unless explicitly included in fields. Is that correct? If so, I believe this can be closed.

[duckontheweb]

It looks like this was changed in stac-utils/stac-fastapi#397 so that links are not returned unless explicitly included in fields. Is that correct? If so, I believe this can be closed.

I misinterpreted that PR, the test above still fails. However, that PR did add code to allow the client to specifically exclude "links", so using:

body = {"fields": {"include": ["id", "collection"], "exclude": ["links"]}}

gets the desired effect.

@lossyrob @philvarner Given that the field list is more like a preference than rules, does this seem to be sufficient?

[gadomski]

I've implemented a fix for this issue here, but after reading https://github.com/stac-api-extensions/fields#includeexclude-semantics, I think this issue is invalid. Combining

If fields attribute is specified with an empty object, or with both include and exclude set to null or an empty array, the recommended behavior is as if include was set to ["id", "type", "geometry", "bbox", "links", "assets", "properties.datetime"].

with

If only include is specified, these attributes are added to the default set of attributes (set union operation).

implies that links should be included in the response, unless explicitly excluded. Closing as wontfix, please reopen if you think my interpretation is incorrect.

[philvarner]

As I said before:

This is legal behavior wrt the Fields Extension -- the list of fields is only expressing a client preference rather than a response requirement.

So, this is not a bug because of STAC API conformance, but it may still be a desired change in the behavior of stac-fastapi.

[gadomski]

Understood.

it may still be a desired change in the behavior of stac-fastapi

I've opened stac-utils/stac-fastapi#524 to push towards a decision on desired behavior. Reopening this issue, linked to stac-utils/stac-fastapi#524.

[matthewhanson]

I agree with @mmcfarland here. that it's cumbersome to explicitly specify excludes in order to cancel out the default includes. It may require some experimentation since it may not be explicitly advertised what a particular implementation includes. I've never much cared for the default includes, if you specify includes or excludes then of course you understand that it may make items invalid. They seem like unnecessary guardrails, since you can override them anyway with excludes, why the extra step to try to protect users?

I realize we aren't talking about removing the default set of includes, but if we are to keep them then links should be included in it, because otherwise it defeats the purpose of the default set to begin with.

In addition to that, the fields extension is apparently out of date, because it should also include stac_version in order to make returned Items valid.

[gadomski]

I'm moving this issue out of the 2.4.4 milestone as it's taking a while to resolve, and I really would like to start working on the backend refactor (process outlined here: stac-utils/stac-fastapi#517). To reduce the PR clutter, I've closed the two relevant PRs, but left their branches around for reference:

• Remove links from default include list stac-fastapi#524
• Align fields extension behavior with extension suggestions stac-fastapi#527

Once stac-api-extensions/fields#5 is merged and released, we should align with those recommendations.