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.

Sign up for GitHub

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

Set 'id' field when deserializing a PutPipelineRequest #790

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set 'id' field when deserializing a PutPipelineRequest #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ This section is for maintaining a changelog for all breaking changes for the cli
- Fix PutTemplateRequest field deserialization ([#723](https://github.com/opensearch-project/opensearch-java/pull/723))
- Fix PutIndexTemplateRequest field deserialization ([#765](https://github.com/opensearch-project/opensearch-java/pull/765))
- Fix InnerHits storedFields deserialization/serialization ([#781](https://github.com/opensearch-project/opensearch-java/pull/781)
- Fix PutPipelineRequest field deserialization ([#789](https://github.com/opensearch-project/opensearch-java/issues/789)

### Security

Expand Down Expand Up @@ -258,4 +259,4 @@ This section is for maintaining a changelog for all breaking changes for the cli
[2.5.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.4.0...v2.5.0
[2.4.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.3.0...v2.4.0
[2.3.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.2.0...v2.3.0
[2.2.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.1.0...v2.2.0
[2.2.0]: https://github.com/opensearch-project/opensearch-java/compare/v2.1.0...v2.2.0
1 change: 1 addition & 0 deletions java-client/src/main/java/org/opensearch/client/opensearch/ingest/PutPipelineRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor Author

@patschl patschl Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@reta reta Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

op.add(Builder::onFailure, JsonpDeserializer.arrayDeserializer(Processor._DESERIALIZER), "on_failure");
op.add(Builder::processors, JsonpDeserializer.arrayDeserializer(Processor._DESERIALIZER), "processors");
op.add(Builder::version, JsonpDeserializer.longDeserializer(), "version");
Expand Down
27 changes: 27 additions & 0 deletions java-client/src/test/java/org/opensearch/client/opensearch/experiments/ParsingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
import org.opensearch.client.opensearch.indices.IndexSettingsSearch;
import org.opensearch.client.opensearch.indices.Translog;
import org.opensearch.client.opensearch.indices.get_field_mapping.TypeFieldMappings;
import org.opensearch.client.opensearch.ingest.ConvertType;
import org.opensearch.client.opensearch.ingest.PutPipelineRequest;
import org.opensearch.client.opensearch.model.ModelTestCase;

public class ParsingTests extends ModelTestCase {
Expand Down Expand Up @@ -357,4 +359,29 @@ public void testIndexSettingsSearch() {
assertEquals(search.idle().after().time(), deserialized.idle().after().time());

}

@Test
public void testPutPipelineRequestDeserialization() {
var putPipelineRequest = PutPipelineRequest.of(
b -> b.id("test-pipeline")
.description("pipeline desc")
.processors(p -> p.convert(c -> c.field("age").targetField("age").type(ConvertType.Integer)))
);

var input = "{\"id\":\"test-pipeline\",\"description\":\"pipeline desc\","
+ "\"processors\":[{\"convert\":{\"field\":\"age\",\"target_field\":\"age\",\"type\":\"integer\"}}]}";

var deserialized = fromJson(input, PutPipelineRequest._DESERIALIZER);

assertEquals(putPipelineRequest.id(), deserialized.id());
assertEquals(putPipelineRequest.description(), deserialized.description());
assertEquals(putPipelineRequest.processors().size(), deserialized.processors().size());

var processor = putPipelineRequest.processors().get(0);
var deserializedProcessor = deserialized.processors().get(0);
assertEquals(processor._kind(), deserializedProcessor._kind());
assertEquals(processor.convert().field(), deserializedProcessor.convert().field());
assertEquals(processor.convert().targetField(), deserializedProcessor.convert().targetField());
assertEquals(processor.convert().type(), deserializedProcessor.convert().type());
}
}
Loading