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 Clarifications #312

Draft
wants to merge 3 commits into
base: IDTA-01002-3-1_preparation
Choose a base branch
from

Conversation

sebbader-sap
Copy link
Contributor

* do not use for "normal" -> deprecated
* PUT semantic for /$value and /$metadata
@sebbader-sap sebbader-sap added this to the 3.1 milestone Aug 30, 2024
Copy link
Collaborator

@mjacoby mjacoby left a comment

Choose a reason for hiding this comment

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

Is this available in rendered form? It is quite difficult to review OpenAPI in yaml form and pasting it into an OpenAPI editor does only work to some degree because of dependencies to the model.

I have the following issues with the proposal

  1. PATCH with /$value does not work, at least not the way described here, because it allows to add elements and the required type information cannot be inferfed from the valueOnly serialization (e.g. value of 42 could be int, integer, double, float, short, ...). Prohibiting to add new values does not fully resolve this problem, as existing values could change datatype, e.g. a DateTime property with new value 42 - would this be allowed? According to the current state of the document I would say yes. However, it is unclear what the expected result is.
  2. PATCH operations still have level parameter - that does not make sense?
  3. PATCH with /$metadata takes type concrete types as input (e.g. Submodel, SubmodelElement). This means one has to send all mandatory fields, i.e., even the ones unchanged. How is that different from classical PUT behavior?

My proposal would be to keep things simple and RFC conform: just allow PATCH on submodel ands submodelElements without any modifiers like /$value or /$metadata according to RFC 7386 JSON Merge Patch as this covers all the functionality we want and is already well known by many developers.


For /$metadata: +
The serialised submodel in the request body must not contain any SubmodelElements. +
The server must overwrite all metadata-relevant Submodel fields with the received fields. This means in particular that previously used fields of the Submodel instance that are missing in the request body indicate a deletion of them. It is not possible to change single items in contained arrays. In particular, a client must therefore send the complete content of arrays (e.g. for `description` or `displayName`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that classical PUT behavior? How does this differ from PUT?

The server must overwrite all metadata-relevant Submodel fields with the received fields. This means in particular that previously used fields of the Submodel instance that are missing in the request body indicate a deletion of them. It is not possible to change single items in contained arrays. In particular, a client must therefore send the complete content of arrays (e.g. for `description` or `displayName`).

For /$value: +
The server must overwrite all ValueOnly-relevant Submodel fields with the received values. This means in particular that previously used fields of the Submodel instance that are missing in the request body indicate a deletion of them. It is not possible to change single items in contained arrays, for instance, for SubmodelElementLists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be a 1:1 copy from other sections but I think it would be better to write something different and more specific to modifying the value of a submodel. E.g., this text mentions SubmodelElementLists which are not relevant here because we are talking about modifying an array of SubmodelElements without the need to know what the elements are.

More important, this won't work becaue valueOnly serialization does not contain all information to "create" new elements. Imaging a submodel without any child elements. Through PATCH on /$value we now send something like {"foo": 42}. What is the result? We could deduct that this is a property called foo, but we cannot infer the type of foo (double, int, short, ???) as the valueOnly serialization does not hold the necessary type information.
As a consequence, adding the elements via valueOnly must not be allowed which would require quite complex processing to check because of arbitrary nesting depth and also because type checking would need to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I think it would be better to write something different

Always open for better proposals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More important, this won't work because valueOnly serialization does not contain all information to "create" new elements.

It was the outcome of the last group call to define it like that. Please raise the topic again there, together with a respective formulation, to cover also this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not an issue of how to phrase things but a technical issue that this approach will not work.
In the REST API WG calls I did propose to follow standard JSON Merge Patch behavior and drop support for PATCH on /$metadata and /$value; a proposal that does not suffer from the problem we are talking about.
This problem will always exist when doing a PATCH on /$value because there is no type information in the valueOnly serialization. From my perspective there are only two "fixes": change the valueOnly serialization to include type information (bad idea) or to not allow PATCH on /$value.

I don't know what else to tell you or what else to present besides the simple solution I already proposed which is following best practices for REST APIs and just go with RFC 7386 JSON Merge Patch


URL-encoded IdShortPath

For /$metadata: +
The serialised SubmodelElement in the request body must not contain any values. +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typical PUT behavior?

The serialised SubmodelElement in the request body must not contain any values. +
The server must overwrite all metadata-relevant SubmodelElement fields with the received fields. This means in particular that previously used fields of the SubmodelElement instance that are missing in the request body indicate a deletion of them. It is not possible to change single items in contained arrays. In particular, a client must therefore send the complete content of arrays (e.g. for `description` or `displayName`).

For /$value: +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to PATCH /submodel/$value, however, for certain cases this potentially might work, e.g. if limited to only work on elements of type Property. Even this would still require defining additional semantics such as what happens when having a property of type DateTime and sending a numeric value from which we cannot infer the concrete type from the valueOnly serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a broader topic. We currently do not require the server to check whether the value value actually fits to the format claimed in valueType.

@@ -521,7 +534,19 @@ base64url-encoded identifier

|PostSubmodel |POST |/submodels |
|PutSubmodelById |PUT |/submodels/\{submodelIdentifier} |base64url-encoded identifier
|PatchSubmodelById |PATCH |/submodels/\{submodelIdentifier} |path-suffix=$metadata/$value or no suffix for normal
|PatchSubmodelById |PATCH |/submodels/\{submodelIdentifier} |path-suffix=/$metadata or /$value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issues as for PatchSubmodel

@sebbader-sap
Copy link
Contributor Author

Is this available in rendered form?

No, with the current pipeline we have this is not possible. You can view it in a local IDE that has an OpenAPI plugin, though.

@sebbader-sap
Copy link
Contributor Author

  1. PATCH operations still have level parameter - that does not make sense?

I think you have a point here. We had already an issue on Patch and Level. Please help me to find all occurrences and adjust them.

@sebbader-sap
Copy link
Contributor Author

PATCH with /$metadata takes type concrete types as input (e.g. Submodel, SubmodelElement). This means one has to send all mandatory fields, i.e., even the ones unchanged. How is that different from classical PUT behavior?

The difference is what we regard as the resource. As the normal representation is the original serialisation, and metadata or value are only projection of it, we decided to use PATCH here.

@waltersve
Copy link

waltersve commented Sep 19, 2024

From my perspective, PATCH will also work with “/$value” (basically any modification) without compromising any guarantees related to correctness. Of course the validation is enforced in a different way. Given that you have a submodel descriptor referencing a submodel template (a semanticId defining the unique identifier of the model supported by the submodel e.g. urn:samm:io.catenax.battery.battery_pass:6.0.0#BatteryPass) which is for example based on SAMM, the backend has all necessary information to verify the request payload. This can be done using generated artifacts such as JSON Schema, OpenAPI specifications, DTOs with bean annotations related to JSR 380 and the once from Jackson, loading ttl files at runtime and access the semantic information via native interfaces like Java, TypeScript, .. or even generate the database schemas. This approach is also is very efficient for both the submodel (backend) and the API consumer, resulting in smaller request payloads.

@mjacoby
Copy link
Collaborator

mjacoby commented Sep 19, 2024

@waltersve I get what you are saying but this only works under two assumptions

  1. SubmodelTemplates are used
  2. Submodels that are instances of SubmodelTemplates are not allowed to have additional elements not defined in the SMT

The first assumption definitely does not hold in general as it is optional to use SMTs. In fact, SMTs are only a special case and the API must be based on the generic metamodel without any knowledge of any SMTs.

About the section assumption I am not sure if this holds or not. At least I am not aware of any constraint defined in the AAS specification that submodels that use a SubmodelTemplate are not allowed to have properties besides the ones defined in the SubmodelTemplate. If there is such a constraint, please provide a citation. My personal understanding is that SMT specific a set of elements a submodel can/must have but an instance of that SMT is free to contain additional elements.

To summarize, I do not see how PATCH on /$value can work safely besides the very narrow special case you mentioned that might not even be 100% compliant with the specification, i.e., the problem remains unsolved.

@sebbader-sap
Copy link
Contributor Author

I have also the opinion that the whole behaviour must work even though the server has no knowledge of the used SubmodelTemplates (at the moment it receives the message).

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.

3 participants