Add Get Aliases API to the high-level REST client #28799
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.client; | ||
|
|
||
| import org.elasticsearch.ElasticsearchException; | ||
| import org.elasticsearch.action.ActionResponse; | ||
| import org.elasticsearch.cluster.metadata.AliasMetaData; | ||
| import org.elasticsearch.common.xcontent.StatusToXContentObject; | ||
| import org.elasticsearch.common.xcontent.ToXContent; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
| import org.elasticsearch.common.xcontent.XContentParser.Token; | ||
| import org.elasticsearch.rest.RestStatus; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
||
| import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken; | ||
|
|
||
| public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject { | ||
|
|
||
| private final RestStatus status; | ||
|
There was a problem hiding this comment. It feels weird to have the status here. Everything else doesn't use it unless they get an exception. There was a problem hiding this comment. I see that we do this because we don't throw exceptions on 404s because they could contain partial results. I think we should have documentation for this in the asciidoc because it is super uncommon for our APIs to do this. There was a problem hiding this comment. See #30536. Man when I think of documenting this I think we better spend time on changing it asap :) |
||
| private final String errorMessage; | ||
| private final Map<String, Set<AliasMetaData>> aliases; | ||
|
|
||
| public GetAliasesResponse(RestStatus status, String errorMessage, Map<String, Set<AliasMetaData>> aliases) { | ||
| this.status = status; | ||
| this.errorMessage = errorMessage; | ||
| this.aliases = aliases; | ||
| } | ||
|
|
||
| @Override | ||
| public RestStatus status() { | ||
| return status; | ||
| } | ||
|
|
||
| public String getErrorMessage() { | ||
| return errorMessage; | ||
| } | ||
|
|
||
| public Map<String, Set<AliasMetaData>> getAliases() { | ||
| return aliases; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| GetAliasesResponse that = (GetAliasesResponse) o; | ||
| return status == that.status && | ||
| Objects.equals(errorMessage, that.errorMessage) && | ||
| Objects.equals(aliases, that.aliases); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(status, errorMessage, aliases); | ||
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
|
There was a problem hiding this comment. Can we bind this to the rest action buildResponse ? Otherwise it would be easy to get out of sync if anything changes There was a problem hiding this comment. this is here only for testing, it is not really needed as the response only needs to be parsed back. I do see how there is some duplication, but this toXContent is much more straight-forward than what the REST action currently does, so I am not sure what to do. how did you mean to bind the two? There was a problem hiding this comment. I meant that if code is shared between There was a problem hiding this comment. If we wanted to we could move this class to the server and use it to build the response that we return to users. It is a good idea but I prefer not to move this thing into the server. |
||
| builder.startObject(); | ||
| { | ||
| if (status != RestStatus.OK) { | ||
| builder.field("error", errorMessage); | ||
| builder.field("status", status.getStatus()); | ||
| } | ||
|
|
||
| for (Map.Entry<String, Set<AliasMetaData>> entry : aliases.entrySet()) { | ||
| builder.startObject(entry.getKey()); | ||
| { | ||
| builder.startObject("aliases"); | ||
| { | ||
| for (final AliasMetaData alias : entry.getValue()) { | ||
| AliasMetaData.Builder.toXContent(alias, builder, ToXContent.EMPTY_PARAMS); | ||
| } | ||
| } | ||
| builder.endObject(); | ||
| } | ||
| builder.endObject(); | ||
| } | ||
| } | ||
| builder.endObject(); | ||
| return builder; | ||
| } | ||
|
|
||
| public static GetAliasesResponse fromXContent(XContentParser parser) throws IOException { | ||
| if (parser.currentToken() == null) { | ||
| parser.nextToken(); | ||
| } | ||
| ensureExpectedToken(Token.START_OBJECT, parser.currentToken(), parser::getTokenLocation); | ||
| Map<String, Set<AliasMetaData>> aliases = new HashMap<>(); | ||
|
|
||
| String currentFieldName; | ||
| Token token; | ||
| String exceptionMessage = null; | ||
| RestStatus status = RestStatus.OK; | ||
|
|
||
| while (parser.nextToken() != Token.END_OBJECT) { | ||
| if (parser.currentToken() == Token.FIELD_NAME) { | ||
| currentFieldName = parser.currentName(); | ||
|
|
||
| if ("status".equals(currentFieldName)) { | ||
| if ((token = parser.nextToken()) != Token.FIELD_NAME) { | ||
| ensureExpectedToken(Token.VALUE_NUMBER, token, parser::getTokenLocation); | ||
| status = RestStatus.fromCode(parser.intValue()); | ||
| } | ||
|
There was a problem hiding this comment. I see why you can't use Could you add error handling for some of this? Like, we should throw an exception if the There was a problem hiding this comment. I don't get what you mean, can you elaborate? There was a problem hiding this comment. I think this if statement confused me: I find myself asking "how can this be a field name and, if it is, what will we do?" Or, "what happens if someone sends and There was a problem hiding this comment. ok I see what you mean, in the high-level REST client we are rather lenient, in this case we should either throw or most likely skipChildren if we find a start_array |
||
| } else if ("error".equals(currentFieldName)) { | ||
| if ((token = parser.nextToken()) != Token.FIELD_NAME) { | ||
| if (token == Token.VALUE_STRING) { | ||
| exceptionMessage = parser.text(); | ||
| } else if (token == Token.START_OBJECT) { | ||
| parser.nextToken(); | ||
| exceptionMessage = ElasticsearchException.innerFromXContent(parser, true).getMessage(); | ||
|
There was a problem hiding this comment. We don't want the whole exception? There was a problem hiding this comment. it would be good to throw, but the problem is that this is in reality a 404, so for which 404s do we throw and for which we don't? We went for never throwing then, but the response doesn't have a place to hold the whole exception. Shall we add that at this point? This response is probably going to look even weirder. There was a problem hiding this comment. one alternative is what @olcbean tried before, that I didn't like at first: throw exception in the fromXContent when we find the complete exception, that is the only way to differentiate between the different 404 responses I think. Let me know what you prefer. There was a problem hiding this comment. We could save the exception as a member variable. It is weird but the API is weird. There was a problem hiding this comment. then you would either have the message or the exception? |
||
| } | ||
| } | ||
| } else { | ||
| String indexName = parser.currentName(); | ||
| if (parser.nextToken() == Token.START_OBJECT) { | ||
| Set<AliasMetaData> parseInside = parseAliases(parser); | ||
| aliases.put(indexName, parseInside); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return new GetAliasesResponse(status, exceptionMessage, aliases); | ||
| } | ||
|
|
||
| private static Set<AliasMetaData> parseAliases(XContentParser parser) throws IOException { | ||
| Set<AliasMetaData> aliases = new HashSet<>(); | ||
| Token token; | ||
| String currentFieldName = null; | ||
| while ((token = parser.nextToken()) != Token.END_OBJECT) { | ||
| if (token == Token.FIELD_NAME) { | ||
| currentFieldName = parser.currentName(); | ||
| } else if (token == Token.START_OBJECT) { | ||
| if ("aliases".equals(currentFieldName)) { | ||
| while (parser.nextToken() != Token.END_OBJECT) { | ||
| AliasMetaData fromXContent = AliasMetaData.Builder.fromXContent(parser); | ||
| aliases.add(fromXContent); | ||
| } | ||
| } else { | ||
| parser.skipChildren(); | ||
| } | ||
| } else if (token == Token.START_ARRAY) { | ||
| parser.skipChildren(); | ||
| } | ||
| } | ||
| return aliases; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about extending the
GetAliasesResponse? I know it does not bring anything but could mark a dependency ( if something is changed on the server side, the REST client needs to be modified as well ... )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that on the very long term the client will not depend on es core anymore, I was wondering if I should even extend ActionResponse, I would prefer not to extend the original
GetAliasesResponse. We do need to figure out how to manage changes, hopefully tests fail if the REST response changes though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid that the tests will not catch a change. The parsing of a response is lenient : if there are unknown fields, no errors ;). What about making the tests stricter (
supportsUnknownFields = false) ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, even integration tests would work fine if we add fields. We have to address this (not necessarily in this PR though), I guess we have the same with synced flush which is also implemented with a client specific response. Let's see what Nik thinks about it.