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

Added Put Mapping API to high-level Rest client (#27205) #27869

Merged
merged 10 commits into from Jan 23, 2018

Conversation

catalin-ursachi
Copy link
Contributor

Adds the Put Mapping API (part of #27205).

Still have to write the docs.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -165,6 +166,18 @@ static Request createIndex(CreateIndexRequest createIndexRequest) throws IOExcep
return new Request(HttpPut.METHOD_NAME, endpoint, parameters.getParams(), entity);
}

static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
String endpoint = endpoint(putMappingRequest.indices(), "_mapping", putMappingRequest.type());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to support putMappingRequest.getConcreteIndex() here? If so, how/when should it be used? Does it make sense to support in client-specific code?

Copy link
Member

Choose a reason for hiding this comment

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

no that's an internal concept leaked to our external API. Maybe check in RestHighLevelClient that concreteIndex is not set, as it will be ignored.

@@ -89,6 +91,29 @@ public void createIndexAsync(CreateIndexRequest createIndexRequest, ActionListen
listener, Collections.emptySet(), headers);
}

/**
* Creates an index using the Put Mapping API
Copy link
Contributor

Choose a reason for hiding this comment

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

put mapping adds a type or fields to an existing index?

@dadoonet
Copy link
Member

IMO this is one of the API we should not add to the REST Client. See discussion at #15613 (comment).

If we merge it, we should may be merge it as @Deprecated... ?

But I'd prefer that others comment as well.

@catalin-ursachi
Copy link
Contributor Author

Fwiw, would it not be simplest to merge this as is, and modify it together with the corresponding endpoint (be it PUT /_settings or something else) if and when necessary?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna javanna self-requested a review January 16, 2018 16:08
@javanna javanna self-assigned this Jan 16, 2018
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

this looks pretty good @catalin-ursachi thanks a lot! Besides the few comments that I left, I think this only needs docs. If you could work on that, I would be happy to merge this in. Sorry it took me a while to look at it.

@@ -165,6 +166,18 @@ static Request createIndex(CreateIndexRequest createIndexRequest) throws IOExcep
return new Request(HttpPut.METHOD_NAME, endpoint, parameters.getParams(), entity);
}

static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
String endpoint = endpoint(putMappingRequest.indices(), "_mapping", putMappingRequest.type());
Copy link
Member

Choose a reason for hiding this comment

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

no that's an internal concept leaked to our external API. Maybe check in RestHighLevelClient that concreteIndex is not set, as it will be ignored.

if (source != null) {
builder.rawValue(new BytesArray(source), XContentType.JSON);
} else {
builder.startObject().endObject();
Copy link
Member

Choose a reason for hiding this comment

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

is it ok here to only end the object? should we not start it too? and above, does rawValue print out a valid complete object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the first question; we are both starting and ending it, aren't we?

And yes, as source has to be a complete object, rawValue applied by itself to the builder also produces one.

mapping.endObject();
mapping.endObject();
mapping.endObject();
request.source(mapping);
Copy link
Member

Choose a reason for hiding this comment

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

do we need to test the case where source is not set?

return request;
}

private static XContentBuilder randomMapping() throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javanna - These two methods are duplicated in CreateIndexRequestTests; what would be a sensible place to commonise them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Although fwiw randomMapping does differ slightly between the two classes)

Copy link
Member

Choose a reason for hiding this comment

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

you can make one of them public and call it from both tests, I don't mind

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

thanks for the update @catalin-ursachi I added a few more comments, nothing major though, and thanks a lot for adding the docs. Once you address my latest comments this should be ready to be merged.

@@ -179,6 +180,21 @@ static Request createIndex(CreateIndexRequest createIndexRequest) throws IOExcep
return new Request(HttpPut.METHOD_NAME, endpoint, parameters.getParams(), entity);
}

static Request putMapping(PutMappingRequest putMappingRequest) throws IOException {
// The concreteIndex is an internal concept, not applicable to requests made over the REST API.
assert (putMappingRequest.getConcreteIndex() == null);
Copy link
Member

Choose a reason for hiding this comment

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

given that a request is provided by users, this should rather be an exception that gets thrown. Also this assertion may make the user's application crash. Throw IllegalArgumentException instead?

Params parameters = Params.builder();
parameters.withTimeout(putMappingRequest.timeout());
parameters.withMasterTimeout(putMappingRequest.masterNodeTimeout());
parameters.withUpdateAllTypes(putMappingRequest.updateAllTypes());
Copy link
Member

Choose a reason for hiding this comment

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

this parameter has been removed from master, I think that you may see that once you merge master in. I need to remember to add it back though when backporting to 6.x.

PutMappingRequest putMappingRequest = new PutMappingRequest(indexName);

putMappingRequest.type("type_name");

Copy link
Member

Choose a reason for hiding this comment

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

nit: can you remove this empty line and the one above please?

assertTrue(putMappingResponse.isAcknowledged());

Map<String, Object> indexMetaData = getIndexMetadata(indexName);

Copy link
Member

Choose a reason for hiding this comment

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

nit: can you remove this empty line?

" }\n" +
" }\n" +
" }\n" +
"}", // <2>
Copy link
Member

Choose a reason for hiding this comment

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

was this required? Maybe you can revert this small part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IDE indented the lines; will revert.

Would you like me to add the two extra spaces per line within the string back in? They seemed superfluous.


String expectedRequestBody = "{}";

assertEquals(expectedRequestBody, actualRequestBody);
Copy link
Member

Choose a reason for hiding this comment

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

here too can you remove some of these empty lines please?

return request;
}

private static XContentBuilder randomMapping() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

you can make one of them public and call it from both tests, I don't mind

mapping.endObject();
mapping.endObject();
mapping.endObject();
request.source(mapping);
Copy link
Member

Choose a reason for hiding this comment

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

I had left a comment on this, do we need to test also the case where the source is not set hence it's null? We do cover it in PutMappingRequest#toXContent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's the new testToXContentWithEmptySource below.

(Sorry, all comments got hidden because the merge with master moved all these files to server/...)

Copy link
Member

Choose a reason for hiding this comment

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

nice, sorry I missed it.

--------------------------------------------------
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[put-mapping-request-source]
--------------------------------------------------
<1> The mapping source
Copy link
Member

Choose a reason for hiding this comment

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

should we cover the different way to provide the source like we did in other docs pages (e.g. search API)? I see that we didn't do it for the create index API, but maybe we should have. Shall we have a common page on how to provide a source and link to it? What do you think? Maybe we could also do it as a followup so this PR doesn't get too big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does make sense; but it does rather seem like there could be some more user-friendly mapping-building tools, abstracting out the boilerplate. E.g. the ones in Elastic4s:

    createIndex("artists").mappings(
      mapping("modern").fields(
        textField("name")
      )
    )

(https://github.com/sksamuel/elastic4s)

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 anyway; as things are now, I think a separate doc makes sense, so as not to clutter the Create Index and Put Mapping pages too much?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should do better, but for now we are only porting what the transport client supports to the new client. I think that having a separate page would be good so we don't repeat the same content in different pages, I assume that the content would be the same, it can probably already be extracted from the search docs page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool; I am tempted to suggest doing it as a follow-up PR, then, since I'm not sure I've got the time to do it just now. If you think it's a blocker for this one, though, then that's fair enough.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, fine as a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

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

@catalin-ursachi let me know if you want to take this, otherwise I can take care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By all means feel free to take it; otherwise I'll pick it up when I next set aside some time for OS work, but I can't promise when that is. 🙂

<2> The type to create (or update)

==== Mapping source
A description of the fields to create on the mapping; if not defined, the mapping will default to empty.
Copy link
Member

Choose a reason for hiding this comment

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

should we list this under the optional arguments? After all it is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Technically it's optional; but a PutMapping request without a source just adds a new type; which, in the ES 6.x+ world, is basically meaningless.

@catalin-ursachi
Copy link
Contributor Author

Made all the requested changes, save for the two outstanding questions.

@javanna javanna merged commit cf61d79 into elastic:master Jan 23, 2018
@javanna
Copy link
Member

javanna commented Jan 23, 2018

thanks @catalin-ursachi !

javanna pushed a commit that referenced this pull request Jan 23, 2018
javanna added a commit that referenced this pull request Jan 23, 2018
martijnvg added a commit that referenced this pull request Jan 24, 2018
* es/master:
  Remove redundant argument for buildConfiguration of s3 plugin (#28281)
  Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (#28329)
  Fix spelling error
  Reindex: Wait for deletion in test
  Reindex: log more on rare test failure
  Ensure we protect Collections obtained from scripts from self-referencing (#28335)
  [Docs] Fix asciidoc style in composite agg docs
  Adds the ability to specify a format on composite date_histogram source (#28310)
  Provide a better error message for the case when all shards failed (#28333)
  [Test] Re-Add integer_range and date_range field types for query builder tests (#28171)
  Added Put Mapping API to high-level Rest client (#27869)
  Revert change that does not return all indices if a specific alias is requested via get alias api. (#28294)
  Painless: Replace Painless Type with Java Class during Casts (#27847)
  Notify affixMap settings when any under the registered prefix matches (#28317)
martijnvg added a commit that referenced this pull request Jan 24, 2018
* es/6.x:
  Remove redundant argument for buildConfiguration of s3 plugin (#28281)
  Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (#28329)
  Fix spelling error
  Reindex: Wait for deletion in test
  Reindex: log more on rare test failure
  Ensure we protect Collections obtained from scripts from self-referencing (#28335)
  Provide a better error message for the case when all shards failed (#28333)
  REST high-level client: add support for update_all_types option to put mapping API
  Added Put Mapping API to high-level Rest client (#27869)
  [Test] Re-Add integer_range and date_range field types for query builder tests (#28171)
  Revert change that does not return all indices if a specific alias is requested via get alias api. (#28294)
  Painless: Replace Painless Type with Java Class during Casts (#27847)
  Notify affixMap settings when any under the registered prefix matches (#28317)
jasontedor added a commit to matarrese/elasticsearch that referenced this pull request Jan 24, 2018
* master: (94 commits)
  Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (elastic#28329)
  Fix spelling error
  Reindex: Wait for deletion in test
  Reindex: log more on rare test failure
  Ensure we protect Collections obtained from scripts from self-referencing (elastic#28335)
  [Docs] Fix asciidoc style in composite agg docs
  Adds the ability to specify a format on composite date_histogram source (elastic#28310)
  Provide a better error message for the case when all shards failed (elastic#28333)
  [Test] Re-Add integer_range and date_range field types for query builder tests (elastic#28171)
  Added Put Mapping API to high-level Rest client (elastic#27869)
  Revert change that does not return all indices if a specific alias is requested via get alias api. (elastic#28294)
  Painless: Replace Painless Type with Java Class during Casts (elastic#27847)
  Notify affixMap settings when any under the registered prefix matches (elastic#28317)
  Trim down usages of `ShardOperationFailedException` interface (elastic#28312)
  Do not return all indices if a specific alias is requested via get aliases api.
  [Test] Lower bwc version for rank-eval rest tests
  CountedBitSet doesn't need to extend BitSet. (elastic#28239)
  Calculate sum in Kahan summation algorithm in aggregations (elastic#27807) (elastic#27848)
  Remove the `update_all_types` option. (elastic#28288)
  Add information when master node left to DiscoveryNodes' shortSummary() (elastic#28197)
  ...
@KnowledgeGarden
Copy link

The preliminary doc https://www.elastic.co/guide/en/elasticsearch/client/java-rest/6.x/java-rest-high-put-mapping.html shows how to use putMapping. When attempted on 6.1.2, the api is missing; documentation to set mappings seems incomplete.

@KnowledgeGarden
Copy link

Documentation on CreateIndexRequest -- a class in the codebase for 6.1.3 says to fire that request to make a CreateIndexResponse with client.indices().create(...) and, of course, that API is missing. There must be a way to create index/mappings pairs well documented somewhere online.

@javanna
Copy link
Member

javanna commented Feb 2, 2018

@KnowledgeGarden to know when something was/will be released, you can check the labels of the PR. This one is marked 6.3.0, meaning that it will be released with 6.3.0. It is true that the request is already there, as it is part of the transport client, but the high-level REST client doesn't support it yet in version 6.1.3.

@KnowledgeGarden
Copy link

@javanna Thanks! Timeline for release of 6.3.0?

@catalin-ursachi
Copy link
Contributor Author

Hi @KnowledgeGarden; you can of course use the low-level rest client for creating an index and adding a mapping:

// Create index
client().performRequest("PUT", "my_new_index");

// Put mapping
HttpEntity mappingSource = new NStringEntity(
    "{\n" +
        "  \"properties\": {\n" +
        "    \"name\": {\n" +
        "      \"type\": \"text\"\n" +
        "    }\n" +
        "  }\n" +
        "}\n",
    ContentType.APPLICATION_JSON);
client().performRequest("PUT", "my_new_index/_mapping/my_new_type", Collections.emptyMap(), mappingSource);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants