Remove link type #290
Remove link type #290
Conversation
|
Discussed this with Matt in a video chat; thinking through these and related changes, I think this will be a big improvement and the assumptions are sound. 👍 |
|
In updating the tests I came across a test case that is using a relative link for a hierarchical link, which would no longer be supported: While moving assets along with the metadata might be a common case (and relative links for assets still work), I don't think using relative hrefs in a link should be a recommended practice. Relative links for an asset that's in the same location as the metadata makes sense, but a source link for labels points to another Item which could be in some random part of the tree and having a relative link to that is rather awkward. I'm fine with not supporting this use case right now. Alternately we could just keep all non-hierarchical links as is when renormalizing....since a renormalization is typically going to be over the whole catalog and the relative link will still be valid. |
…ELF_CONTAINED cats
…f setting catalog_type on root
|
@matthewhanson using relative links to go to different parts of a Catalog tree is necessary for self-contained ML datasets. Otherwise you'll have to use absolute links to link a Label Item to it's source imagery, and then you can no longer copy the catalog around and have it be valid. I think this is a use case we need to support. This is the sort of use case that causes cycles in the graph that is otherwise a tree - an annoying one to support but one I think is worth utilizing if we consider STAC to be something that can allow referencing of Items in a deeper way then parent->child (e.g., a derived from link). Is there a way to avoid Link Type and also allow for these types of relative links? Where does the problem? I don't follow where in the normalization process this breaks things. |
|
Good point, basically we don't want to convert relative non-hierarchical links to absolute URLs. If we just leave relative links the way they are, if you normalize_hrefs those relative links will still be valid. It's a problem if you wanted to move the item from one place in the catalog to another, but that wouldn't work now would it? You would use Or if you remove the item linked to or move it, but again, that also would cause the same issue currently. |
|
Leaving the links alone actually is the same way that relative assets are treated, so I think we should keep them consistent. |
|
If you are normalizing HREFs, it will resolve links recursively via resolve_links, which uses _object_links, which can be contributed to by extensions, a la the label extension specifying that the "source" link is an object link that should be resolved. So for example you are normalizing a catalog One way the traversal could happen is with
Another traversal reads IMAGES first, but similarly resolves SourceItem, and when the "source" link of the LabelItem is read, it resolves to the cached item, and they are still refering to the same object. It seems like your changes continue to resolve the object links, so I think the above would still apply here? The link for any of the _object_link links should not be updated by the linking object, because those are pending modification (or are already resolved and modified) by the normalize_href call in another part of the catalog. I might be confusing myself, and others with this, but at least I think the "objects can get resolved via non-hierarchical links" is applicable to your idea of using resovled-ness as a flag determining whether or not to save something |
|
I've updated this to handle all links defined by extensions |
Codecov Report
@@ Coverage Diff @@
## develop #290 +/- ##
===========================================
- Coverage 93.68% 93.65% -0.04%
===========================================
Files 33 33
Lines 4057 4004 -53
===========================================
- Hits 3801 3750 -51
+ Misses 256 254 -2
Continue to review full report at Codecov.
|
|
We should probably cut a new release with this change and the coupe of fixes - the last 1.0.0-beta.2 release. Want to set up a time to pair on that so there's two of us who can release? |
Related Issue(s): #
#286 , also resolves #285
Description:
This PR removes the link_type field and the LinkType class, along with the need to loop through all links to set them as RELATIVE or ABSOLUTE.
Instead, when a link href is requested it returns an absolute link unless all the following are true:
When all are true the link relative to the owner is returned.
There are some additional implications and related changes:
Catalog.catalog_type = Catalog.Type.X...subsequent saves will generate relative links for all saved objectsnormalize_and_saveandsaveis no longer needed, in fact it should be deprecated. It's been left in for backward compatibility. The catalog_type of the root should be favored.I've not yet updated tests, instead I'm putting this WIP PR up for discussion since it's a big internal change. Afterward I can update tests
PR Checklist:
scripts/format)scripts/test)