stac-utils / pystac

Python library for working with any SpatioTemporal Asset Catalog (STAC)
https://pystac.readthedocs.io
Other
358 stars 120 forks source link

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

Closed duckontheweb closed 3 years ago

duckontheweb commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.