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

indices client and xcontent/json support #143

Closed
UglyBarnacle opened this issue Jan 28, 2022 · 13 comments · Fixed by #200
Closed

indices client and xcontent/json support #143

UglyBarnacle opened this issue Jan 28, 2022 · 13 comments · Fixed by #200
Assignees
Labels
Area: Documentation Improvements or additions to documentation Category: Enhancement New feature or request Category: Question Not an issue but a question. May lead to enhancing docs

Comments

@UglyBarnacle
Copy link

Hello and I have a question:
I am trying out this new fluent client and have a bunch of projects that I wanted to migrate from the RHLC to the new API client. Now I am kinda stuck on an inconvenient spot, creating an index with settings and mapping. the builder and functional pattern are nice, though in our projects we put json-files for mappings and settings which are read and converted into an object/xContentBuilder so we do not have to implement complex settings/mappings into java code (and reuse without copy paste).

kinda like that

CreateIndexRequest createIndexRequest = new CreateIndexRequest(indexName)
				.settings(this.elasticsearchIndexService.createSettings(ELASTICSEARCH_INDEX_SETTINGS_FILE))
				.mapping(this.elasticsearchIndexService.createMapping(ELASTICSEARCH_INDEX_MAPPING_FILE,
					ELASTICSEARCH_INDEX_MAPPING_FOLDER, shop));
AcknowledgedResponse indexCreateResponse = this.restHighLevelClient.indices().create(createIndexRequest, RequestOptions.DEFAULT);

depending on the shop parameter different mappings are loaded.

Is there a similar and fluent way doing that in the new client or are there any plans supporting json/xcontent?

@adagios
Copy link

adagios commented Feb 1, 2022

I would argue that the same need exists for queries.

I have queries that are built based on json specified queries (easy to test, easy to comprehend) that previously could be composed using QueryBuilders.wrapperQuery(), and now I don't see a way to use them.

Some way to deserialize from JSON to Query (Or Query.Builder) would be most useful.

@sothawo
Copy link
Contributor

sothawo commented Feb 1, 2022

wrapper query is available now since 7.17.0

@sulaukad
Copy link

@sothawo
Do you have an example of how to use a wrapper query in the given case?

@adagios
Copy link

adagios commented Feb 11, 2022

(don't think this is the right place for this, but trying to help)

@sulaukad to build a query you could use:

        WrapperQuery query = new WrapperQuery.Builder()
                .query("{some json query}")
                .build();

or in a builder:

        SearchRequest request = new SearchRequest.Builder()
                .query(_0 -> _0.wrapper(_1 -> _1
                        .query("{some json query}")))
                .build();

@adagios
Copy link

adagios commented Feb 16, 2022

There is a way to deserialize JSON strings to a Request. But I don't know if it's something that will keep working.

At this moment, it's possible to use the _DESERIALIZER that's a static member of every Request.
Something like:

JsonpMapper mapper = new JacksonJsonpMapper();
CreateIndexRequest request = CreateIndexRequest._DESERIALIZER.deserialize(
          mapper.jsonProvider().createParser(
                  new ByteArrayInputStream(stringWithJsonRequest.getBytes(StandardCharsets.UTF_8))
          ),
          mapper
 );

@sothawo do you thinks this is a valid use?

@sothawo
Copy link
Contributor

sothawo commented Feb 17, 2022

Don|t ask me, I am not maintaining this project, I am only using it, and yes, I use the _DESERIALIZER as well in some places.

@swallez
Copy link
Member

swallez commented Feb 17, 2022

Sorry for the late answer. The client's jsonp mapper provides an easy way to achieve that without having to deal with the intricacies of _DESERIALIZER(which should be considered internal, but has to be made public for cross-package visibility reasons).

There are some "to" and "from" string methods in the tests:

public static <T> String toJson(T value, JsonpMapper mapper) {
StringWriter sw = new StringWriter();
JsonProvider provider = mapper.jsonProvider();
JsonGenerator generator = provider.createGenerator(sw);
mapper.serialize(value, generator);
generator.close();
return sw.toString();
}
public static <T> T fromJson(String json, Class<T> clazz, JsonpMapper mapper) {
JsonParser parser = mapper.jsonProvider().createParser(new StringReader(json));
return mapper.deserialize(parser, clazz);
}

And you can get the mapper from a client using client._transport().jsonpMapper().

Hope this answers the question.

@swallez swallez self-assigned this Feb 17, 2022
@swallez swallez added Area: Documentation Improvements or additions to documentation Category: Question Not an issue but a question. May lead to enhancing docs labels Feb 17, 2022
@adagios
Copy link

adagios commented Feb 22, 2022

@swallez Is there any difference between obtaining a mapper from the transport versus just instantiating one?

Is client._transport().jsonMapper() better in some way than just new JsonMapper()?

@swallez
Copy link
Member

swallez commented Mar 4, 2022

@adagios by new JsonMapper() you mean com.fasterxml.jackson.databind.json.JsonMapper?

The Java API client deserialization relies on its own co.elastic.clients.json.JsonpMapper interface that abstracts the actual JSON mapper used (we have implementations based on Jackson and Json-b).

So client._transport().jsonMapper() is of the correct type, although you can of course also use new co.elastic.clients.json.jackson.JacksonJsonpMapper(new com.fasterxml.jackson.databind.json.JsonMapper()).

@swallez swallez added the Category: Enhancement New feature or request label Mar 14, 2022
@swallez
Copy link
Member

swallez commented Mar 17, 2022

We've added support for loading JSON directly from object builders with a new withJson(someJson) method in PR #200.

This is backwards compatible and will be in the next 7.17 and 8.1 patch releases.

The documentation for this new feature is already up for the main branch.

@sbrunot
Copy link

sbrunot commented Mar 18, 2022

@swallez a quick question about using deserialize (either with _DESERIALIZER or mapper.deserialize) for class CreateIndexRequest: I can get it working because an index name is mandatory when creating a CreateIndexRequest instance and there seem no way to provide it in the JSON data (it is simply ignored by the parser). I endup with the following exception:

co.elastic.clients.util.MissingRequiredPropertyException: Missing required property 'CreateIndexRequest.index'
	at co.elastic.clients.util.ApiTypeHelper.requireNonNull(ApiTypeHelper.java:76)
	at co.elastic.clients.elasticsearch.indices.CreateIndexRequest.<init>(CreateIndexRequest.java:90)
	at co.elastic.clients.elasticsearch.indices.CreateIndexRequest.<init>(CreateIndexRequest.java:61)
	at co.elastic.clients.elasticsearch.indices.CreateIndexRequest$Builder.build(CreateIndexRequest.java:407)
	at co.elastic.clients.elasticsearch.indices.CreateIndexRequest$Builder.build(CreateIndexRequest.java:223)
	at co.elastic.clients.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:80)
	at co.elastic.clients.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:43)
	at co.elastic.clients.json.JsonpMapperBase.deserialize(JsonpMapperBase.java:38)

Can you confirm that deserializing a CreateIndexRequest is indeed not supported (the deserialization only deserialize the request payload, not the other requests parameters) ?

@swallez
Copy link
Member

swallez commented Mar 18, 2022

@sbrunot indeed, many requests have required properties that are part of the URL path, and this is typically the case for CreateIndexRequest's index property. Since they can't be expressed as JSON, there's no way they can be be initialized by the deserializer, which is then causing this error when the deserializer calls build() to create the object before returning it.

The new withJson feature solves this by separating builder creation from its deserialization. This exact use case is in the docs examples:

InputStream input = this.getClass()
    .getResourceAsStream("some-index.json"); 

CreateIndexRequest req = CreateIndexRequest.of(b -> b
    .index("some-index")
    .withJson(input) 
);

This feature is already available in the 7.17.2 and 8.1.1 snapshots.

@swallez
Copy link
Member

swallez commented Mar 18, 2022

Also, as I mentioned previously, _DESERIALIZER should be considered internal although it has to be made public for cross-package visibility reasons. Hopefully with the withJson feature the use cases mentioned in this issue (and more, actually) are covered and _DESERIALIZER will go back to where it belongs: a low level concern that users should not be concerned with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Category: Enhancement New feature or request Category: Question Not an issue but a question. May lead to enhancing docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants