fix: Prevent metadata duplication

[Jimlinz]

Pystac normalize_hrefs resolves all links and mutates the entire tree, causing <item>.json and collection.json for all datasets within the catalog to update. save_object only saves the particular entity within the catalog, preventing unnecessary metadata duplication.

Reference

stac-utils/pystac#284
stac-utils/pystac#294
stac-utils/pystac#90

[billgeo]

We do want to normalize hrefs across the Geostore (hopefully we have a test to enforce that). Have you checked that we have code elswehere for that and/or a test for this?

[billgeo]

We definitely don't need to normalize every href every time we add a dataset though, just need to do the catalog.json and all the STAC files in the child / dataset that is being added. Can ignore all the sibling dataset stac files.

[Jimlinz]

This was addressed in stac-utils/pystac#294 (if I read the PR correctly). Nevertheless, tests are failing which means more work is needed to figure out what is needed to fix this issue.

[Jimlinz]

Changing PR to draft since tests are failing. save_object appears to be a suggestion to work around #1818; however, this is causing tests to fail. More work is needed to figure out why the tests are failing and if there is a better solution for the problem above.

[l0b0]

Looking through the test failures:

• should_update_root_catalog_with_new_version_catalog [dataset version links] fails because an optional title is missing. Possible solutions:
  • Ignore the irrelevant field in the test. This is probably the simplest, but means the test might have to change again if PySTAC changes.
  • Change the test to only check the URLs rather than the whole link array. More robust, but also more work and less coverage.
• should_update_root_catalog_with_new_version_collection [item links] fails for two separate reasons:
  • Missing title, as above.
  • Wrong root link. This is an actual issue, and needs fixing.
• should_successfully_run_dataset_version_creation_process_and_again_with_partial_upload fails because of a missing/wrong root link (expected_link_object is never in (load(smart_open.open(f"{storage_bucket_prefix}{dataset_title}/{item_metadata_filename}", mode="rb")))[STAC_LINKS_KEY]). Also needs fixing.

In summary, to make this work ASAP we could:

1. Modify tests to remove the title from the relevant expected links entries.
2. Change the code which writes metadata files (geostore.import_metadata_file.task.importer) to modify the root URL as it's currently specified in the tests.

[l0b0]

Rather than deleting the title from the actual state you could simply remove the non-existent title from the expected state. That should result in less code overall, and very slightly more coverage.

[Jimlinz]

Do you mean something like this bb4fd62?
It was my initial attempt, but it didn't work. Maybe I have missed something?

[l0b0]

Something like bb4fd62, except some of the titles should still be there.

[l0b0]

Isn't there some missing code to actually replace the root href?

>               assert item_json[STAC_LINKS_KEY] == expected_item_links
E               AssertionError: assert [{'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n  'rel': 'root',\n  'type': 'application/json'},\n {'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n  'rel': 'parent',\n  'type': 'application/json'}] == [{'href': '../catalog.json', 'rel': 'root', 'type': 'application/json'},\n {'href': './CCRrWAn9FPPOyNFd1Q6I.json',\n  'rel': 'parent',\n  'type': 'application/json'}]
E                 At index 0 diff: {'rel': 'root', 'href': './CCRrWAn9FPPOyNFd1Q6I.json', 'type': 'application/json'} != {'rel': 'root', 'href': '../catalog.json', 'type': 'application/json'}
E                 Full diff:
E                   [
E                 -  {'href': '../catalog.json',
E                 +  {'href': './CCRrWAn9FPPOyNFd1Q6I.json',
E                     'rel': 'root',
E                     'type': 'application/json'},
E                    {'href': './CCRrWAn9FPPOyNFd1Q6I.json',
E                     'rel': 'parent',
E                     'type': 'application/json'},
E                   ]

[Jimlinz]

Took a while for me to trace this down. There are two options to tackle this bug:
root_catalog.save_object() or root_catalog.save(catalog_type=CatalogType.SELF_CONTAINED) see populate_catalog/task.py

The first option save_object is the workaround suggested by Pystac (see stac-utils/pystac#284). This would be the ideal implementation as none of the metadata gets duplicated during dataset version creation (at least in in my own tests); however, from what I can tell, this options eludes importer from being called in import_metadata_file/task.py (which is why you are seeing the error message above - something I tried in the early commit). Why this may be the case exactly, I am still slightly unclear. I have tried tracing this down but with no success thus far. Perhaps you might have a better understanding of how Geostore utilizes Pystac to shine some light on why this may be? I will continue to explore and will update if I am able to work this out.

The second option root_catalog.save(catalog_type=CatalogType.SELF_CONTAINED) is the option I went with in this PR. This appears to only be a partial fix. It prevents item.json from being updated with every new dataset version creation; however, it doesn't prevent collection.json from being updated every time a new dataset is added or updated.

[l0b0]

This constant already exists in geostore/populate_catalog/task.py, so it should probably be moved to a file in the geostore directory (so that it's shared between subdirectories at deployment time) and imported.

[Jimlinz]

Have changed this to reference geostore/populate_catalog/task.py (for now). Good idea to move this to another file, but a quick look shows it is reference in quite a few places, so I might move this change to another PR.

[Jimlinz]

Spoke too soon. The value needs to be accessible via the lambda function. Changing this back to ../catalog.json for now and will create another PR to address the above.