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

Patch endpoints #744

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

rhysrevans3
Copy link
Contributor

@rhysrevans3 rhysrevans3 commented Aug 21, 2024

Description:
Adds PATCH endpoints to transaction extension. Adds support for RFC 6902 and RFC 7396. Pivots on header Content-Type value.

Related pull request: stac-api-extensions/transaction#14

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@rhysrevans3 rhysrevans3 marked this pull request as ready for review August 22, 2024 08:37
@rhysrevans3
Copy link
Contributor Author

I've had to switch from stac-pydantic Item/Collection to Dict to allow for partial updates. Not sure if this is the best method or if you can switch off pydantic validation. Another option would be having separate models for partial items/collections in stac-pydantic where all attributes.

@vincentsarago
Copy link
Member

I've had to switch from stac-pydantic Item/Collection to Dict to allow for partial updates. Not sure if this is the best method or if you can switch off pydantic validation. Another option would be having separate models for partial items/collections in stac-pydantic where all attributes.

Would a typedDict make more sense?

Adding default for content_type.
Moving transaction types back to extensions.transaction.
Adding literals for content_type.
Removing sys check for TypedDict import.
@vincentsarago
Copy link
Member

@rhysrevans3 should we wait for stac-api-extensions/transaction#14 to be merged/published?

def __init__(self, *args, **kwargs):
"""Init function to add 'from' field."""
super().__init__(*args, **kwargs)
self.__setattr__("from", kwargs["from"])
Copy link
Member

Choose a reason for hiding this comment

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

because we're using pydantic model I think we can use Field

class PatchMoveCopy(BaseModel):
   """Move or Copy Operation."""

   path: str
   op: Literal["move", "copy"]
   fromurl: str = Field(alias="from")

but then the attribute is .fromurl not .from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I hadn't seen alias before that looks neater.

I've also added:

model_config = ConfigDict(populate_by_name=True)

which means you can do this:

patch_operation = PatchMoveCopy(**{"path": "hello", "from":"world", "op": "move"})

And overrode the model_dump method so by_alias defaults to True to use the alias in the model dump

patch_operation.model_dump()
#> {'path': 'hello', 'op': 'move', 'from': 'world'}

Not sure if this is the right thing to do or if we should expect people to use:

patch_operation.model_dump(by_alias=True)

@vincentsarago
Copy link
Member

@rhysrevans3 I'm trying to get a general sense of the patch operation. It's not clear to me what are the requirements that an API should implement. By default it seems that a client will have to support a lot of things

For example, I'm looking at https://docs.ogc.org/DRAFTS/20-002.html#update_clause and I don't see reference to the move/copy operations

@rhysrevans3
Copy link
Contributor Author

My understanding is RFC 5789, which the OGC doc references, say a PATCH endpoint should except a set of actions to modify the requested resource. But it doesn't specify what these actions should be.

RFC 6902 extends this and specify a set of allowed operations (add, remove, replace, move, copy, test) which is what I have suggested in stac-api-extensions/transaction#14.

Currently the README on the transaction extension suggests the PATCH endpoint follows RFC 7396 which accepts a partial json document which it merges with the existing document this might not be the best choice for STAC because of it's handling of null values. As mentioned in this issue stac-api-extensions/transaction#13

@djspstfc
Copy link

djspstfc commented Sep 3, 2024

Just to add to @rhysrevans3 's comments, the main issue with supporting only RFC7396 (or RFC7386) is the "magic" behaviour of setting attribute to null in combination with the elastic search backend:

  • RFC7396 treats explicit null as a command to delete that attribute (this is your "delete" patch command)
  • If you actually want to set a value to null, you can't
  • ElasticSearch on the other hand, has a different understanding of explicit null as "leave this in the document but don't index this value"

Without a fairly complicated scripting command on elastic search, it would not be possible for elastic search to return a _source document that meets the expected result of an explicit null being sent in a PATCH request.

RFC6902 gets around this by allowing you to explicitly say "that attribute there, delete it" and not "that attribute there, I'm going to tell you to set it to null but which I mean please write a script to tell elastic search to remove it from the source document and reindex".

@vincentsarago
Copy link
Member

@rhysrevans3 The overall looks great, just added one comment about using stac-pydantic base model.

As I've asked couple weeks ago, should we wait for stac-api-extensions/transaction#14 to be merged? do you know anyone who could help move this PR forward?

@rhysrevans3
Copy link
Contributor Author

@rhysrevans3 The overall looks great, just added one comment about using stac-pydantic base model.

As I've asked couple weeks ago, should we wait for stac-api-extensions/transaction#14 to be merged? do you know anyone who could help move this PR forward?

Ideally yes I think it would be better to have that PR merged first.

But my understanding is that RFC 6902 and RFC 7396 are implementations of RFC 5789, which is the current STAC/OGC specification. So this merging this pull still keeps within the current STAC specification.

But given @m-mohr comments on that pull request maybe it's best to wait for OGC's opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants