Skip to content

Raise exception on attempt to extend object not implementing that extension #370

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

Closed
duckontheweb opened this issue May 27, 2021 · 7 comments · Fixed by #450
Closed

Raise exception on attempt to extend object not implementing that extension #370

duckontheweb opened this issue May 27, 2021 · 7 comments · Fixed by #450
Assignees
Milestone

Comments

@duckontheweb
Copy link
Contributor

Calling, for instance, pystac.extensions.eo.EOExtension.ext on an Item that does not have the EO schema URI in its stac_extensions list will succeed and return an ItemEOExtension instance. You can then read and write fields like eo:cloud_cover even though the Item doesn't actually implement that extension.

It seems like the desired behavior here would be to raise an exception when using ext on an object that does not implement that extension for 2 reasons:

  1. When using PySTAC to read existing STAC Items, this could mask a problem with an invalid object
  2. This will interfere with the edge case where a user has custom fields with a prefix that overlaps with one of the extensions (maybe because the custom fields predate the extension). In this case, the fields should not be treated as if they are defined by the extension.

Originally raised as part of this PR comment in stac-utils/stactools

@duckontheweb duckontheweb changed the title Raise exception when attempting to extend an object that does not implement that extension Raise exception on attempt to extend object not implementing that extension May 27, 2021
@duckontheweb duckontheweb modified the milestones: 1.0.0, 1.0.0-rc.1 May 28, 2021
@duckontheweb duckontheweb self-assigned this Jun 8, 2021
@duckontheweb duckontheweb modified the milestones: 1.0.0-rc.1, 1.0.0 Jun 11, 2021
@duckontheweb
Copy link
Contributor Author

I think I could use some input on how to implement this for Asset instances with no owner. Since Assets do not have a stac_extension list, the only way to know if an Asset officially "implements" an extension is if the owning Item or Collection includes the schema URI. In the case where we are calling *Extension.ext on an Asset without an owner, should the call raise an exception or succeed? If the first, then we would be requiring developers to attach the Asset to an Item/Collection before adding extension properties. If the second, then we run the risk of adding those extension properties to the asset, attaching it to an Item later, but not adding the schema URI to that Item (unless there's another mechanism for checking this that I've missed, or we build one).

@gadomski
Copy link
Member

gadomski commented Jun 16, 2021

I haven't seen many any cases where an Asset exists before the Item does. My instinct is to raise an exception when trying to extend an un-parented Asset. If folks really need to work around that exception, they can modify the properties directly.

@lossyrob
Copy link
Member

lossyrob commented Jun 16, 2021

My preference would be for an exception not to be thrown if the Asset doesn't have an owner, and perhaps give a warning in the documentation. This way users can build up assets before attaching them to items legitimately if that is better for their workflow. I'd rather be trusted by the library to do the right thing than to be blocked by an exception for a legitimate usage.

If we do want to raise, I would say an argument to avoid raising that the user could explicitly state to work around this issue would be ideal if possible.

@gadomski
Copy link
Member

@lossyrob how should the attachment work in this case? Would you want the Item to "inherit" extensions from the Asset? Or is it the dev's responsibility to turn on the extension in the Item?

@lossyrob
Copy link
Member

lossyrob commented Jun 16, 2021

The latter case - if you use an *Extension object to access properties on the Asset that doesn't have an Item, then it's on the user to make sure the Item it is eventually associated with has the proper extensions enabled.

@duckontheweb
Copy link
Contributor Author

The other option I considered was to make the Asset class "extension-aware" in some way. Then we could do a check when the asset is added to an Item and add extension URIs as necessary.

I haven't thought through the implementation in detail, but one possibility would be to extract the stac_extensions property out of STACObject and into a Extensible base class that both STACObject and Asset could inherit from. This might allow us to make the ExtensionManagementMixin generic over Extensible instead of STACObject, and then the Asset would be responsible for maintaining its own internal list of extension URIs if it is not owned or updating the owning Items's list if it is owned. Definitely some nuance to work out there, though.

@duckontheweb
Copy link
Contributor Author

I can start by having the call succeed for un-owned and then we can always add a feature/fix to update the owning Item when adding an Asset. The breaking change is really raising the exception, so I wanted to make sure we had that ironed out for 1.0.

duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 16, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 16, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 16, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 16, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
duckontheweb added a commit to duckontheweb/pystac that referenced this issue Jun 17, 2021
@duckontheweb duckontheweb linked a pull request Jun 17, 2021 that will close this issue
4 tasks
duckontheweb pushed a commit that referenced this issue Jun 17, 2021
Raise exception when extending object that does not have schema URI
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 a pull request may close this issue.

3 participants