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

Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. #6928

Merged

Conversation

Projects
None yet
3 participants
@martijnvg
Copy link
Member

martijnvg commented Jul 18, 2014

The percolator and filter parsing for aliases should forcefully enforce strict query parsing.

Strict parsing for percolator and filter alias parsing is only enforced on indices created after to upgrade to 1.4.0

PR for #6664

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/IndexQueryParserService.java Outdated
* @return The lowest node version in the cluster when the index was created or <code>null</code> if that was unknown
*/
public Version getIndexCreatedVersion() {
return indexSettings.getAsVersion(IndexMetaData.SETTING_VERSION_CREATED, null);

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 21, 2014

Contributor

Or rather Version.indexCreated(indexSettings) which has additional asserts about the version?

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/IndexQueryParserService.java Outdated
@@ -117,7 +119,7 @@ public IndexQueryParserService(Index index, @IndexSettings Settings indexSetting

this.defaultField = indexSettings.get("index.query.default_field", AllFieldMapper.NAME);
this.queryStringLenient = indexSettings.getAsBoolean("index.query_string.lenient", false);
this.strict = indexSettings.getAsBoolean("index.query.parse.strict", false);
this.defaultStrict = indexSettings.getAsBoolean("index.query.parse.strict", false);

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 21, 2014

Contributor

I don't think it is ok to use the same configuration parameter for

  • throwing an error on unmapped fields
  • throwing an error on deprecated options.

I think we should have a new setting.

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/IndexQueryParserService.java Outdated
}

public ParsedQuery parse(QueryParseContext context, BytesReference source) throws ElasticsearchException {
public ParsedQuery parse(QueryParseContext context, BytesReference source, Boolean forceStrictParsing) throws ElasticsearchException {

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 21, 2014

Contributor

Instead of passing an additional Boolean on all parsing methods, should this rather be a flag on the context?

@jpountz jpountz removed the review label Jul 21, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Jul 21, 2014

@martijnvg Left some comments.

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Jul 21, 2014

Thanks @jpountz for the comments, I updated the PR.

@jpountz

View changes

src/main/java/org/elasticsearch/index/percolator/PercolatorQueriesRegistry.java Outdated
@@ -101,6 +104,7 @@ public PercolatorQueriesRegistry(ShardId shardId, @IndexSettings Settings indexS
this.indexCache = indexCache;
this.indexFieldDataService = indexFieldDataService;
this.shardPercolateService = shardPercolateService;
this.disallowUnmappedFields = indexSettings.getAsBoolean("index.percolator.disallow.unmapped.fields", true);

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 25, 2014

Contributor

Can you make it a constant so that tests can use it?

Also, maybe it should be index.percolator.allow_unmapped_fields instead? (removing the double negation when it is set to false and using underscores for the last part, what do you think?)

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 25, 2014

Contributor

Also should we disallow unmapped fields when the index has been created on or after 1.4.0?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jul 28, 2014

Author Member

This is enforced later on in QueryParseContext#failIfFieldMappingNotFound

XContentParser parser = null;
try {
parser = XContentHelper.createParser(source);
try (XContentParser sourceParser = XContentHelper.createParser(source)) {

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 25, 2014

Contributor

nice

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/QueryParseContext.java Outdated
@@ -110,6 +113,7 @@ public void parseFlags(EnumSet<ParseField.Flag> parseFlags) {
}

public void reset(XContentParser jp) {
disallowUnmappedFields = null;

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 25, 2014

Contributor

Should it be a boolean instead and reset to indexQueryParser.defaultDisallowUnmappedFields()?

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/QueryParseContext.java Outdated
boolean disallowUnmappedFields = this.disallowUnmappedFields != null ? this.disallowUnmappedFields : indexQueryParser.defaultDisallowUnmappedFields();
if (disallowUnmappedFields) {
Version indexCreatedVersion = indexQueryParser.getIndexCreatedVersion();
if (fieldMapping == null && indexCreatedVersion != null && indexCreatedVersion.onOrAfter(Version.V_1_4_0)) {

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 25, 2014

Contributor

indexCreatedVersion should never be null?

@martijnvg martijnvg changed the title Core: The `index.query.parse.strict` setting also fails if queries refer to unmapped fields. Core: Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. Jul 28, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Jul 28, 2014

@jpountz Thanks for your review. I updated the PR.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 12, 2014

LGTM

@jpountz jpountz removed the review label Aug 12, 2014

@martijnvg martijnvg added v1.5.0 and removed v1.4.0.Beta1 labels Sep 8, 2014

@clintongormley clintongormley added v1.4.0.Beta1 and removed v1.5.0 labels Sep 8, 2014

@martijnvg martijnvg force-pushed the martijnvg:improvements/strict_query_parsing branch from ec7a7ed Sep 8, 2014

@clintongormley clintongormley changed the title Core: Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. Settings: Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. Sep 8, 2014

@martijnvg martijnvg force-pushed the martijnvg:improvements/strict_query_parsing branch 2 times, most recently Sep 9, 2014

Core: Added the `index.query.parse.allow_unmapped_fields` setting to …
…fail queries if they refer to unmapped fields.

The percolator and filters in aliases by default enforce strict query parsing.

Closes #7335

@martijnvg martijnvg force-pushed the martijnvg:improvements/strict_query_parsing branch to 52f1ab6 Sep 9, 2014

@martijnvg martijnvg merged commit 52f1ab6 into elastic:master Sep 9, 2014

@martijnvg martijnvg deleted the martijnvg:improvements/strict_query_parsing branch May 18, 2015

@clintongormley clintongormley changed the title Settings: Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. Add a `index.query.parse.allow_unmapped_fields` setting that fails if queries refer to unmapped fields. Jun 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.