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

[BUG] Deserialization CreateIndexRequest is broken. #206

Open
alexisgayte opened this issue Aug 12, 2022 · 11 comments
Open

[BUG] Deserialization CreateIndexRequest is broken. #206

alexisgayte opened this issue Aug 12, 2022 · 11 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@alexisgayte
Copy link

alexisgayte commented Aug 12, 2022

What is the bug?

The deserialization of CreateIndexRequest is broken.

How can one reproduce the bug?

By deserializing CreateIndexRequest using the ObjectMapper provided.

What is the expected behavior?

Here, it is unclear what the behaviour should be. however it should not fail.

Do you have any additional context?

The serialization is specific to opensearch client therefore you need to use it. But somehow the "index" field is discarded as it is part of the URL for the API call.
The deserialization follows the same process as the serialization. However, "index" field is mandatory but it cannot be pass as property.

https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/opensearch/indices/CreateIndexRequest.java#L440-L446

https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/opensearch/indices/CreateIndexRequest.java#L97

@alexisgayte alexisgayte added bug Something isn't working untriaged labels Aug 12, 2022
@dblock
Copy link
Member

dblock commented Aug 12, 2022

Repro steps? Or maybe want to try to write a failing test for this?

@alexisgayte
Copy link
Author

Hi,
Thanks for the quick turn around.

I haven't found any test template,
However any CreateIndexRequest deserialization will fail.

Here a simple example of the issue:

      JacksonJsonpMapper mapper = (JacksonJsonpMapper) client._transport().jsonpMapper();

      JsonParser parser = Json.createParser(new StringReader("{\"index\":\"index\",\"mappings\":{\"properties\":{\"test\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\"}}}}},\"settings\":{\"number_of_shards\":\"1\",\"number_of_replicas\":\"1\"}}"));
      CreateIndexRequest deserialize = mapper.deserialize(parser, CreateIndexRequest.class);

      client.indices().create(deserialize);

You should get
Caused by: org.opensearch.client.util.MissingRequiredPropertyException: Missing required property 'CreateIndexRequest.index'.

@abhinav-nath
Copy link
Contributor

I am getting the same error. Any news on this bug?
Or, can someone suggest a way to create index using a JSON file?

@alexisgayte
Copy link
Author

I have created a builder for the builder using the internal Serialiser. If that can help.

@abhinav-nath
Copy link
Contributor

@alexisgayte - sure, can you please share it?

@owaiskazi19
Copy link
Member

owaiskazi19 commented Nov 8, 2022

@alexisgayte @abhinav-nath

This is how I have created index using the client. Let me know if this works for you

private String getAnomalyDetectorMappings() throws IOException {
        URL url = AnomalyDetectionIndices.class.getClassLoader().getResource(<enter json file>);
        return Resources.toString(url, Charsets.UTF_8);
    }

JsonpMapper mapper = javaClient._transport().jsonpMapper();
        JsonParser parser = null;
        try {
            parser = mapper
                .jsonProvider()
                .createParser(new ByteArrayInputStream(getAnomalyDetectorMappings().getBytes(StandardCharsets.UTF_8)));
        } catch (Exception e) {
            e.printStackTrace();
        }

        CreateIndexRequest request = null;
        try {
            request = new CreateIndexRequest.Builder()
                .index("index_name")
                .mappings(TypeMapping._DESERIALIZER.deserialize(parser, mapper))
                .build();
        } catch (Exception e) {
            e.printStackTrace();
        }

and for handling deserialization using ObjectMapper

OpenSearchTransport transport = new RestClientTransport(
            restClient,
            new JacksonJsonpMapper(new ObjectMapper().registerModule(new JavaTimeModule()))
        );

@alexisgayte
Copy link
Author

Yes, that is mainly the idea.
However in my case I had to build a builder for the builder :

@JsonpDeserializable
public final class CreateIndexRequestBuilder extends CreateIndexRequest.Builder {

    private CreateIndexRequestBuilder(Builder builder) {
        mappings(builder.mappings);
        settings(builder.settings);
    }

    public static class Builder extends ObjectBuilderBase implements ObjectBuilder<CreateIndexRequestBuilder> {

        @Nullable
        private TypeMapping mappings;
        @Nullable
        private IndexSettings settings;

        public final Builder mappings(@Nullable TypeMapping value) {
            this.mappings = value;
            return this;
        }

        public final Builder settings(@Nullable IndexSettings value) {
            this.settings = value;
            return this;
        }

        public CreateIndexRequestBuilder build() {
            _checkSingleUse();
            return new CreateIndexRequestBuilder(this);
        }
    }

    // ---------------------------------------------------------------------------------------------

    /**
     * Json deserializer for {@link CreateIndexRequestBuilder}
     */
    public static final JsonpDeserializer<CreateIndexRequestBuilder> _DESERIALIZER = ObjectBuilderDeserializer
            .lazy(Builder::new, CreateIndexRequestBuilder::setupCreateIndexRequestDeserializer);

    protected static void setupCreateIndexRequestDeserializer(ObjectDeserializer<CreateIndexRequestBuilder.Builder> op) {
        op.add(Builder::mappings, TypeMapping._DESERIALIZER, "mappings");
        op.add(Builder::settings, IndexSettings._DESERIALIZER, "settings");
    }

}

Then you can use it the way you want :

      JacksonJsonpMapper mapper = (JacksonJsonpMapper) client._transport().jsonpMapper();

      JsonParser parser = Json.createParser(new StringReader("{\"mappings\":{\"properties\":{\"test\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\"}}}}},\"settings\":{\"number_of_shards\":\"1\",\"number_of_replicas\":\"1\"}}"));
      CreateIndexRequestBuilder indexRequestBuilder = mapper.deserialize(parser, CreateIndexRequestBuilder.class);
      indexRequestBuilder.index(indexName);
      client.indices().create(indexRequestBuilder.build());

@dblock
Copy link
Member

dblock commented Nov 9, 2022

@alexisgayte Try to turn this into a failing spec?

@alexisgayte
Copy link
Author

I am not sure what you mean by that, I have done it 3 months ago and moved on, bear with me.

The main problem I had, is the definition cannot be auto populated without the index (which is not provided by the opensearch API call and the mapping we get from the API).

I am mainly using spring to wrap my object and auto mapping.

@dblock
Copy link
Member

dblock commented Nov 9, 2022

Thanks for sticking around @alexisgayte!

The original issue and the example above says "The deserialization of CreateIndexRequest is broken." and your code ends up with a missing required property error. Is it unexpected? What do we want to do in this project about it?

@alexisgayte
Copy link
Author

alexisgayte commented Nov 9, 2022

Yes of course, the property is part of the json.
What ever you do with the deserialization you will get this error. Basically you can't deserialised, if I recall.

@wbeckler wbeckler added the good first issue Good for newcomers label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants