Skip to content

Remove types from internal GetFieldMappings request/response objects #48815

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

Merged
merged 15 commits into from
Nov 27, 2019
Merged

Remove types from internal GetFieldMappings request/response objects #48815

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
59ddbd5
Remove types from internal GetFieldMappings request/response objects
romseygeek Nov 1, 2019
f660bf3
feedback; tests
romseygeek Nov 11, 2019
c7ce92a
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 11, 2019
0e910dc
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 12, 2019
cc2af67
Remove special-case when no fields are found
romseygeek Nov 12, 2019
7e8a493
better tests for null mappings
romseygeek Nov 13, 2019
86c90ad
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 13, 2019
5791352
cleanup
romseygeek Nov 14, 2019
6b06af6
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 14, 2019
e25bc12
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 18, 2019
c6ece6c
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 22, 2019
6a3847e
Remove special-case logic for existing index but non-existing field
romseygeek Nov 22, 2019
50bb694
plugin compilation
romseygeek Nov 22, 2019
0f137dd
watcher test
romseygeek Nov 22, 2019
2f96021
Merge remote-tracking branch 'origin/master' into types-removal/getfi…
romseygeek Nov 27, 2019
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 ...spec/src/main/resources/rest-api-spec/test/indices.get_field_mapping/20_missing_field.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
"Return empty object if field doesn't exist, but type and index do":
"Return empty object if field doesn't exist, but index does":

- do:
indices.create:
Expand All @@ -17,3 +17,4 @@
fields: not_existent

- match: { 'test_index.mappings': {}}
Copy link
Contributor

@jtibshirani jtibshirani Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just realized this doesn't seem right -- why did the response change here? It seems like an API break to start returning a completely empty response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This threw me as well, but if you look at the history it's actually restoring the behaviour that was there before include_type_name was added - see #37210. There's specific logic in RestGetFieldMappingAction to return an empty response if the index exists but the field doesn't, but it wasn't being tripped beforehand because no types parameter was included in the request. Now that types is gone entirely it has reverted back to the previous behaviour, but I agree it seems a bit odd - maybe we should just remove this extra logic entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now, thanks for the context! Looks like I subtly changed the behavior there without an explicit discussion.

I also think we should remove the extra logic. Given that we already used this new response format for a whole major release, I think it'd be confusing to change this format again in 8.0. It's also a breaking change that the user can't opt out of.

I noticed that with the current behavior we only return an empty object when a single field is provided. If the request contains two fields that are both non-existent, then we still return the format { "index": { "mappings": {} }. This is true both of the current PR and the old 6.x logic. This seems quite inconsistent, and suggests to me that it's not so bad to keep the { "index": { "mappings": {} } format everywhere.

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'll remove that special case and re-test


32 changes: 15 additions & 17 deletions ...java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsIndexRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,39 @@

package org.elasticsearch.action.admin.indices.mapping.get;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.OriginalIndices;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.action.support.single.shard.SingleShardRequest;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;

import java.io.IOException;

public class GetFieldMappingsIndexRequest extends SingleShardRequest<GetFieldMappingsIndexRequest> {

private final boolean probablySingleFieldRequest;
private final boolean includeDefaults;
private final String[] fields;
private final String[] types;

private OriginalIndices originalIndices;

GetFieldMappingsIndexRequest(StreamInput in) throws IOException {
super(in);
types = in.readStringArray();
if (in.getVersion().before(Version.V_8_0_0)) {
in.readStringArray();
}
fields = in.readStringArray();
includeDefaults = in.readBoolean();
probablySingleFieldRequest = in.readBoolean();
if (in.getVersion().before(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small comment, we could add a comment here explaining this backwards-compatibility logic.

in.readBoolean();
}
originalIndices = OriginalIndices.readOriginalIndices(in);
}

GetFieldMappingsIndexRequest(GetFieldMappingsRequest other, String index, boolean probablySingleFieldRequest) {
this.probablySingleFieldRequest = probablySingleFieldRequest;
GetFieldMappingsIndexRequest(GetFieldMappingsRequest other, String index) {
this.includeDefaults = other.includeDefaults();
this.types = other.types();
this.fields = other.fields();
assert index != null;
this.index(index);
Expand All @@ -61,18 +63,10 @@ public ActionRequestValidationException validate() {
return null;
}

public String[] types() {
return types;
}

public String[] fields() {
return fields;
}

public boolean probablySingleFieldRequest() {
return probablySingleFieldRequest;
}

public boolean includeDefaults() {
return includeDefaults;
}
Expand All @@ -90,10 +84,14 @@ public IndicesOptions indicesOptions() {
@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeStringArray(types);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeStringArray(Strings.EMPTY_ARRAY);
}
out.writeStringArray(fields);
out.writeBoolean(includeDefaults);
out.writeBoolean(probablySingleFieldRequest);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeBoolean(false);
}
OriginalIndices.writeOriginalIndices(originalIndices, out);
}

Expand Down
23 changes: 11 additions & 12 deletions ...main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.action.admin.indices.mapping.get;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.IndicesRequest;
Expand All @@ -28,6 +29,7 @@
import org.elasticsearch.common.io.stream.StreamOutput;

import java.io.IOException;
import java.util.Arrays;

/**
* Request the mappings of specific fields
Expand All @@ -44,7 +46,6 @@ public class GetFieldMappingsRequest extends ActionRequest implements IndicesReq
private boolean includeDefaults = false;

private String[] indices = Strings.EMPTY_ARRAY;
private String[] types = Strings.EMPTY_ARRAY;

private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen();

Expand All @@ -53,7 +54,12 @@ public GetFieldMappingsRequest() {}
public GetFieldMappingsRequest(StreamInput in) throws IOException {
super(in);
indices = in.readStringArray();
types = in.readStringArray();
if (in.getVersion().before(Version.V_8_0_0)) {
String[] types = in.readStringArray();
if (types != Strings.EMPTY_ARRAY) {
throw new IllegalArgumentException("Expected empty type array but received [" + Arrays.toString(types) + "]");
}
}
indicesOptions = IndicesOptions.readIndicesOptions(in);
local = in.readBoolean();
fields = in.readStringArray();
Expand All @@ -79,11 +85,6 @@ public GetFieldMappingsRequest indices(String... indices) {
return this;
}

public GetFieldMappingsRequest types(String... types) {
this.types = types;
return this;
}

public GetFieldMappingsRequest indicesOptions(IndicesOptions indicesOptions) {
this.indicesOptions = indicesOptions;
return this;
Expand All @@ -94,10 +95,6 @@ public String[] indices() {
return indices;
}

public String[] types() {
return types;
}

@Override
public IndicesOptions indicesOptions() {
return indicesOptions;
Expand Down Expand Up @@ -132,7 +129,9 @@ public ActionRequestValidationException validate() {
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeStringArray(indices);
out.writeStringArray(types);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeStringArray(Strings.EMPTY_ARRAY);
}
indicesOptions.writeIndicesOptions(out);
out.writeBoolean(local);
out.writeStringArray(fields);
Expand Down
10 changes: 0 additions & 10 deletions ...va/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,6 @@ public GetFieldMappingsRequestBuilder addIndices(String... indices) {
return this;
}

public GetFieldMappingsRequestBuilder setTypes(String... types) {
request.types(types);
return this;
}

public GetFieldMappingsRequestBuilder addTypes(String... types) {
request.types(ArrayUtils.concat(request.types(), types));
return this;
}

public GetFieldMappingsRequestBuilder setIndicesOptions(IndicesOptions indicesOptions) {
request.indicesOptions(indicesOptions);
return this;
Expand Down
110 changes: 34 additions & 76 deletions ...ain/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@

package org.elasticsearch.action.admin.indices.mapping.get;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
Expand All @@ -34,6 +34,7 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.mapper.Mapper;
import org.elasticsearch.index.mapper.MapperService;

import java.io.IOException;
import java.io.InputStream;
Expand All @@ -55,64 +56,35 @@ public class GetFieldMappingsResponse extends ActionResponse implements ToXConte

private static final ParseField MAPPINGS = new ParseField("mappings");

private static final ObjectParser<Map<String, Map<String, FieldMappingMetaData>>, String> PARSER =
new ObjectParser<>(MAPPINGS.getPreferredName(), true, HashMap::new);

static {
PARSER.declareField((p, typeMappings, index) -> {
p.nextToken();
while (p.currentToken() == XContentParser.Token.FIELD_NAME) {
final String typeName = p.currentName();

if (p.nextToken() == XContentParser.Token.START_OBJECT) {
final Map<String, FieldMappingMetaData> typeMapping = new HashMap<>();
typeMappings.put(typeName, typeMapping);

while (p.nextToken() == XContentParser.Token.FIELD_NAME) {
final String fieldName = p.currentName();
final FieldMappingMetaData fieldMappingMetaData = FieldMappingMetaData.fromXContent(p);
typeMapping.put(fieldName, fieldMappingMetaData);
}
} else {
p.skipChildren();
}
p.nextToken();
}
}, MAPPINGS, ObjectParser.ValueType.OBJECT);
}

// TODO remove the middle `type` level of this
private final Map<String, Map<String, Map<String, FieldMappingMetaData>>> mappings;
private final Map<String, Map<String, FieldMappingMetaData>> mappings;

GetFieldMappingsResponse(Map<String, Map<String, Map<String, FieldMappingMetaData>>> mappings) {
GetFieldMappingsResponse(Map<String, Map<String, FieldMappingMetaData>> mappings) {
this.mappings = mappings;
}

GetFieldMappingsResponse(StreamInput in) throws IOException {
super(in);
int size = in.readVInt();
Map<String, Map<String, Map<String, FieldMappingMetaData>>> indexMapBuilder = new HashMap<>(size);
Map<String, Map<String, FieldMappingMetaData>> indexMapBuilder = new HashMap<>(size);
for (int i = 0; i < size; i++) {
String index = in.readString();
int typesSize = in.readVInt();
Map<String, Map<String, FieldMappingMetaData>> typeMapBuilder = new HashMap<>(typesSize);
for (int j = 0; j < typesSize; j++) {
String type = in.readString();
int fieldSize = in.readVInt();
Map<String, FieldMappingMetaData> fieldMapBuilder = new HashMap<>(fieldSize);
for (int k = 0; k < fieldSize; k++) {
fieldMapBuilder.put(in.readString(), new FieldMappingMetaData(in.readString(), in.readBytesReference()));
}
typeMapBuilder.put(type, unmodifiableMap(fieldMapBuilder));
if (in.getVersion().before(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small comment, we could structure this as

if (in.getVersion().before(Version.V_8_0_0)) {
    int typesSize = in.readVInt();
    assert typesSize <= 1;
    in.readString(); // type
}

This would allow for unifying the following code and also for keeping the map pre-sizing (new HashMap<>(fieldSize);).

int typesSize = in.readVInt();
assert typesSize == 1;
in.readString(); // type
}
int fieldSize = in.readVInt();
Map<String, FieldMappingMetaData> fieldMapBuilder = new HashMap<>(fieldSize);
for (int k = 0; k < fieldSize; k++) {
fieldMapBuilder.put(in.readString(), new FieldMappingMetaData(in.readString(), in.readBytesReference()));
}
indexMapBuilder.put(index, unmodifiableMap(typeMapBuilder));
indexMapBuilder.put(index, unmodifiableMap(fieldMapBuilder));
}
mappings = unmodifiableMap(indexMapBuilder);

}

/** returns the retrieved field mapping. The return map keys are index, type, field (as specified in the request). */
public Map<String, Map<String, Map<String, FieldMappingMetaData>>> mappings() {
/** returns the retrieved field mapping. The return map keys are index, field (as specified in the request). */
public Map<String, Map<String, FieldMappingMetaData>> mappings() {
return mappings;
}

Expand All @@ -122,32 +94,23 @@ public Map<String, Map<String, Map<String, FieldMappingMetaData>>> mappings() {
* @param field field name as specified in the {@link GetFieldMappingsRequest}
* @return FieldMappingMetaData for the requested field or null if not found.
*/
public FieldMappingMetaData fieldMappings(String index, String type, String field) {
Map<String, Map<String, FieldMappingMetaData>> indexMapping = mappings.get(index);
public FieldMappingMetaData fieldMappings(String index, String field) {
Map<String, FieldMappingMetaData> indexMapping = mappings.get(index);
if (indexMapping == null) {
return null;
}
Map<String, FieldMappingMetaData> typeMapping = indexMapping.get(type);
if (typeMapping == null) {
return null;
}
return typeMapping.get(field);
return indexMapping.get(field);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
for (Map.Entry<String, Map<String, Map<String, FieldMappingMetaData>>> indexEntry : mappings.entrySet()) {
builder.startObject();
for (Map.Entry<String, Map<String, FieldMappingMetaData>> indexEntry : mappings.entrySet()) {
builder.startObject(indexEntry.getKey());
builder.startObject(MAPPINGS.getPreferredName());

Map<String, FieldMappingMetaData> mappings = null;
for (Map.Entry<String, Map<String, FieldMappingMetaData>> typeEntry : indexEntry.getValue().entrySet()) {
assert mappings == null;
mappings = typeEntry.getValue();
}
if (mappings != null) {
addFieldMappingsToBuilder(builder, params, mappings);
if (indexEntry.getValue() != null) {
addFieldMappingsToBuilder(builder, params, indexEntry.getValue());
}

builder.endObject();
Expand All @@ -168,7 +131,6 @@ private void addFieldMappingsToBuilder(XContentBuilder builder,
}

public static class FieldMappingMetaData implements ToXContentFragment {
public static final FieldMappingMetaData NULL = new FieldMappingMetaData("", BytesArray.EMPTY);

private static final ParseField FULL_NAME = new ParseField("full_name");
private static final ParseField MAPPING = new ParseField("mapping");
Expand Down Expand Up @@ -206,10 +168,6 @@ public Map<String, Object> sourceAsMap() {
return XContentHelper.convertToMap(source, true, XContentType.JSON).v2();
}

public boolean isNull() {
return NULL.fullName().equals(fullName) && NULL.source.length() == source.length();
}

//pkg-private for testing
BytesReference getSource() {
return source;
Expand Down Expand Up @@ -255,18 +213,18 @@ public int hashCode() {
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeVInt(mappings.size());
for (Map.Entry<String, Map<String, Map<String, FieldMappingMetaData>>> indexEntry : mappings.entrySet()) {
for (Map.Entry<String, Map<String, FieldMappingMetaData>> indexEntry : mappings.entrySet()) {
out.writeString(indexEntry.getKey());
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeVInt(1);
out.writeString(MapperService.SINGLE_MAPPING_NAME);
}
out.writeVInt(indexEntry.getValue().size());
for (Map.Entry<String, Map<String, FieldMappingMetaData>> typeEntry : indexEntry.getValue().entrySet()) {
out.writeString(typeEntry.getKey());
out.writeVInt(typeEntry.getValue().size());
for (Map.Entry<String, FieldMappingMetaData> fieldEntry : typeEntry.getValue().entrySet()) {
out.writeString(fieldEntry.getKey());
FieldMappingMetaData fieldMapping = fieldEntry.getValue();
out.writeString(fieldMapping.fullName());
out.writeBytesReference(fieldMapping.source);
}
for (Map.Entry<String, FieldMappingMetaData> fieldEntry : indexEntry.getValue().entrySet()) {
out.writeString(fieldEntry.getKey());
FieldMappingMetaData fieldMapping = fieldEntry.getValue();
out.writeString(fieldMapping.fullName());
out.writeBytesReference(fieldMapping.source);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ public void writeTo(StreamOutput out) throws IOException {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
for (final ObjectObjectCursor<String, MappingMetaData> indexEntry : getMappings()) {
builder.startObject(indexEntry.key);
if (indexEntry.value != null) {
builder.startObject(indexEntry.key);
builder.field(MAPPINGS.getPreferredName(), indexEntry.value.sourceAsMap());
builder.endObject();
} else {
builder.startObject(MAPPINGS.getPreferredName()).endObject();
}
builder.endObject();
}
return builder;
}
Expand Down
Loading