Skip to content

[Rest Api Compatibility] Ignore use_field_mapping option for docvalue #74435

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 4 commits into from
Jun 24, 2021
Merged

[Rest Api Compatibility] Ignore use_field_mapping option for docvalue #74435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
019291a
[Rest Api Compatibility] Ignore use_field_mapping option for docvalue
pgomulka Jun 22, 2021
32698c3
use constant in custom field format parser
pgomulka Jun 22, 2021
837acb0
revert accidental test unmute
pgomulka Jun 22, 2021
44e4148
Merge remote-tracking branch 'upstream/master' into compat/use_field_…
pgomulka Jun 23, 2021
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
1 change: 0 additions & 1 deletion rest-api-spec/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ tasks.named("yamlRestCompatTest").configure {
'mtermvectors/11_basic_with_types/Basic tests for multi termvector get',
'mtermvectors/21_deprecated_with_types/Deprecated camel case and _ parameters should fail in Term Vectors query',
'mtermvectors/30_mix_typeless_typeful/mtermvectors without types on an index that has types',
'search/10_source_filtering/docvalue_fields with default format', //use_field_mapping change
Copy link
Contributor Author

@pgomulka pgomulka Jun 22, 2021

Choose a reason for hiding this comment

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

the 7.x test asserts about the deprecation warning and the right format (declared on mapping) being used

'search/150_rewrite_on_coordinator/Ensure that we fetch the document only once', //terms_lookup
'search/171_terms_query_with_types/Terms Query with No.of terms exceeding index.max_terms_count should FAIL', //bulk
'search/260_parameter_validation/test size=-1 is deprecated', //size=-1 change
Expand Down
39 changes: 36 additions & 3 deletions server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,35 @@
package org.elasticsearch.search.fetch.subphase;

import org.elasticsearch.Version;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.common.xcontent.ParseField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.ParseField;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.core.CheckedFunction;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.RestApiVersion;

import java.io.IOException;
import java.util.Objects;

import static org.elasticsearch.core.RestApiVersion.equalTo;
import static org.elasticsearch.core.RestApiVersion.onOrAfter;

/**
* Wrapper around a field name and the format that should be used to
* display values of this field.
*/
public final class FieldAndFormat implements Writeable, ToXContentObject {
private static final String USE_DEFAULT_FORMAT = "use_field_mapping";
Copy link
Contributor

@jtibshirani jtibshirani Jun 22, 2021

Choose a reason for hiding this comment

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

This class is also used by the fields option, in addition to docvalue_fields, which means we're allowing use_field_mapping to be set on 7.x on the fields option, even though it was never allowed before. This is unfortunate, but seems like an okay compromise to me.

Copy link
Contributor Author

@pgomulka pgomulka Jun 22, 2021

Choose a reason for hiding this comment

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

good point, I was not aware of this.. I don't think the parser has the information about the parent of the current key/value..
The only alternative would be to use two different parsers, but I am not sure if this is not an overdo.
Especially because this is meant to be for rest backwards compatibility. It would be surprising to see someone start using "use_field_mapping" with rest api compatibility for the fields when it was not allowed in 7.x..
Also it would not do anything anyway.

Copy link
Contributor

@jtibshirani jtibshirani Jun 23, 2021

Choose a reason for hiding this comment

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

I also feel like this is the right trade-off to keep the code simple.

private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(FetchDocValuesPhase.class);

private static final ParseField FIELD_FIELD = new ParseField("field");
private static final ParseField FORMAT_FIELD = new ParseField("format");
private static final ParseField INCLUDE_UNMAPPED_FIELD = new ParseField("include_unmapped");
Expand All @@ -38,10 +48,33 @@ public final class FieldAndFormat implements Writeable, ToXContentObject {

static {
PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELD_FIELD);
PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FORMAT_FIELD);
PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(),
FORMAT_FIELD.forRestApiVersion(onOrAfter(RestApiVersion.V_8)));
PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(),
ignoreUseFieldMappingStringParser(),
FORMAT_FIELD.forRestApiVersion(equalTo(RestApiVersion.V_7)),
ObjectParser.ValueType.STRING_OR_NULL);
PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), INCLUDE_UNMAPPED_FIELD);
}

private static CheckedFunction<XContentParser, String, IOException> ignoreUseFieldMappingStringParser() {
return (p) -> {
if (p.currentToken() == XContentParser.Token.VALUE_NULL) {
return null;
} else {
String text = p.text();
if (text.equals(USE_DEFAULT_FORMAT)) {
DEPRECATION_LOGGER.compatibleApiWarning("explicit_default_format",
"[" + USE_DEFAULT_FORMAT + "] is a special format that was only used to " +
"ease the transition to 7.x. It has become the default and shouldn't be set explicitly anymore.");
return null;
} else {
return text;
}
}
};
}

/**
* Parse a {@link FieldAndFormat} from some {@link XContent}.
*/
Expand Down