Set 'id' field when deserializing a PutPipelineRequest #790
Conversation
patschl
requested review from
reta,
Bukhtawar,
dblock,
szczepanczykd,
madhusudhankonda,
saratvemulapalli and
VachaShah
as code owners
patschl
force-pushed
the
put_pipeline_request_id_deserialization
branch
from
b6c6d4b to
b2e75f4
Compare
Signed-off-by: Patrick Schlindwein <[email protected]> Signed-off-by: Patrick Schlindwein <[email protected]>
patschl
force-pushed
the
put_pipeline_request_id_deserialization
branch
from
b2e75f4 to
ab53a74
Compare
| @@ -481,6 +481,7 @@ protected static void setupPutPipelineRequestDeserializer(ObjectDeserializer<Put | |||
|
|
|||
| op.add(Builder::meta, JsonpDeserializer.stringMapDeserializer(JsonData._DESERIALIZER), "_meta"); | |||
| op.add(Builder::description, JsonpDeserializer.stringDeserializer(), "description"); | |||
| op.add(Builder::id, JsonpDeserializer.stringDeserializer(), "id"); | |||
There was a problem hiding this comment.
@patschl the PutPipelineRequest does not include id as the payload - it is part of the URL and passed as the URL path component. Under which circumstances it is returned in the request payload? Could you please share the example? (sadly the linked issue has no payload sample).
There was a problem hiding this comment.
Thanks @reta!
I'm not quite sure I understand the purpose of this deserializer correctly if it should strictly deal with the payload of a request.
id is a required field for the PutPipelineRequest, so deserializing will currently fail every time, since the deserializer of course does not set it. Wouldn't this make the deserializer unnecessary if it won't work anyway?
We had a similar issue with the name property in the PutIndexTemplateRequest, but managed to work around it.
We persist such index template and pipeline definitions as json in our VCS and create the requests using the objects _DESERIALIZER implementations, this is the reason for this PR.
There was a problem hiding this comment.
Thanks @patschl
I'm not quite sure I understand the purpose of this deserializer correctly if it should strictly deal with the payload of a request.
The deserializer mirrors serializer: in this particular case, the id is not part of the serialized JSON (payload) and as such, should not be deserialized from the payload (since it should not be there in the first place).
There was a problem hiding this comment.
@reta Okay I see, I will close this PR then. Thank you for clearing that up.
There is no way to create this object from a json using this deserializer, but I guess this was never the use case anyway?
There was a problem hiding this comment.
There is no way to create this object from a json using this deserializer, but I guess this was never the use case anyway?
Thank you @patschl, I think the question is under which circumstances it should happen, this is a request which naturally only uses one way serialization (when send to the service). But in general - yes, it seems like some models may need more context (URI components) to deserialize completely.
Description
Currently deserializing a PutPipelineRequest it not possible due to the
idfield not being set but also being required during deserialization.This PR adds support for the
idfield.Issues Resolved
Fixes #789
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.