Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow resolved self links #555

Merged
merged 4 commits into from
Jul 16, 2021
Merged

Allow resolved self links #555

merged 4 commits into from
Jul 16, 2021

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Jul 13, 2021

Related Issue(s): #499.

Description: When getting a STAC object's self href, there was a cast that assumed that the object's self link's target was a string, which was not always the case (the self link's target could be a string or another STAC object). This commit removes that cast by splitting a link's target into two separate "private" attributes, _target_href and _target_object.

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

gadomski added 2 commits July 13, 2021 09:51
When getting a STAC object's self href, there was a cast that assumed
that the object's self link's target was a string, which was not always
the case (the self link's target could be a string or another STAC
object). This commit removes that cast by splitting a link's target into
two separate "private" attributes, `_target_href` and `_target_object`.

Fixes #499.
@lossyrob
Copy link
Member

What is the situation that causes a STACObject's self link to be a STACObject itself?

Should it be the case that a STACObject's self link is either set to a href, or None? Having a resolved self link seems like it's a bit recursive, and I'm unclear there'd be any benefit to having a self link with the target as an object or if there's any potential issues that might arise by allowing that (which we do now, but perhaps mistakenly?)

If we shouldn't block self links from being resolved, then I'm +1 on this change.

@gadomski
Copy link
Member Author

What is the situation that causes a STACObject's self link to be a STACObject itself?

I ran into it when naively resolving all links as a part of validation: https://github.com/stac-utils/stactools/pull/151/files#diff-209f36e38a8b60f0a8a376a7bf6b1d00f00885b56aff923de59cacaa73b552c8R66-R75.

Should it be the case that a STACObject's self link is either set to a href, or None? Having a resolved self link seems like it's a bit recursive, and I'm unclear there'd be any benefit to having a self link with the target as an object or if there's any potential issues that might arise by allowing that (which we do now, but perhaps mistakenly?)

Yup, this would be another way to solve the problem. Policing it might be a bit trickier (put STACObject.links behind a getter/setter?) but not too difficult.

Zooming out to how links work, it seems strange (to me at least) to "erase" the href information when resolving a link:

self.target = obj

This PR keeps the href information after resolution, which could theoretically be useful for debugging or some other use case down the road?

pystac/link.py Outdated
self._target_object = target

def get_target_str(self) -> Optional[str]:
"""Returns this link's target as a string."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be worded differently - if you create a Link with the target that is a STACObject, this would imply to me that it would return the str representation of the link, i.e. call get_self_href on the target; it doesn't and would return None.

Or perhaps this should, and if _target_href is None and there's a _target_object, to return _target_object.get_self_href?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 3561a5c. I think someone could infinitely recurse themselves if they created a self link with only a target object (no string href) but that'd be a weird thing to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added recursion check in 21feb88.

@codecov-commenter
Copy link

Codecov Report

Merging #555 (3561a5c) into main (e4059c8) will increase coverage by 0.00%.
The diff coverage is 79.62%.

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #555    +/-   ##
========================================
  Coverage   94.35%   94.35%            
========================================
  Files          71       71            
  Lines       10409    10649   +240     
  Branches     1089     1099    +10     
========================================
+ Hits         9821    10048   +227     
- Misses        419      427     +8     
- Partials      169      174     +5     
Impacted Files Coverage Δ
pystac/link.py 86.79% <73.17%> (-5.69%) ⬇️
pystac/stac_object.py 90.86% <100.00%> (ø)
tests/test_link.py 100.00% <100.00%> (ø)
pystac/extensions/sar.py 98.60% <0.00%> (-0.37%) ⬇️
pystac/extensions/eo.py 94.41% <0.00%> (ø)
tests/extensions/test_sar.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4059c8...3561a5c. Read the comment docs.

@duckontheweb duckontheweb merged commit 0ba1a1d into stac-utils:main Jul 16, 2021
@gadomski gadomski deleted the issues/499-resolving-self-href branch July 20, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants