Skip to content

more_like_this query to throw an error if the like fields is not provided #40632

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
Apr 18, 2019
Merged

more_like_this query to throw an error if the like fields is not provided #40632

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0657593
more_like_this query to throw an error if the like fields is not prov…
jimczi Mar 29, 2019
8e0be9e
fix test to not create invalid builder
jimczi Mar 29, 2019
186f6df
fix IT
jimczi Apr 1, 2019
621b161
Merge branch 'master' into more_like_this_default_field
jimczi Apr 4, 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
7 changes: 7 additions & 0 deletions server/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,13 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
List<String> moreLikeFields = new ArrayList<>();
if (useDefaultField) {
moreLikeFields = context.defaultFields();
if (moreLikeFields.size() == 1
&& moreLikeFields.get(0).equals("*")
&& (likeTexts.length > 0 || unlikeTexts.length > 0)) {
throw new IllegalArgumentException("[more_like_this] query cannot infer the field to analyze the free text, " +
Copy link
Member

Choose a reason for hiding this comment

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

The error seems to be breaking MoreLikeThisQueryBuilderTests#testToQuery and also occasionally #testMustRewrite. Probably something in the test setup needs to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks good catch, I changed the test to not create invalid builders now that we have this restriction.

"you should update the [index.query.default_field] index setting to a field that exists in the mapping or " +
"set the [fields] option in the query.");
}
} else {
for (String field : fields) {
MappedFieldType fieldType = context.fieldMapper(field);
Expand Down
46 changes: 46 additions & 0 deletions server/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,26 @@

import org.apache.lucene.analysis.core.WhitespaceAnalyzer;
import org.apache.lucene.index.Fields;
import org.apache.lucene.index.Term;
import org.apache.lucene.index.memory.MemoryIndex;
import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.BoostQuery;
import org.apache.lucene.search.DisjunctionMaxQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.TermQuery;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.termvectors.MultiTermVectorsItemResponse;
import org.elasticsearch.action.termvectors.MultiTermVectorsRequest;
import org.elasticsearch.action.termvectors.MultiTermVectorsResponse;
import org.elasticsearch.action.termvectors.TermVectorsRequest;
import org.elasticsearch.action.termvectors.TermVectorsResponse;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.lucene.search.MoreLikeThisQuery;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
Expand Down Expand Up @@ -305,6 +311,39 @@ public void testUnsupportedFields() throws IOException {
assertThat(e.getMessage(), containsString("more_like_this only supports text/keyword fields"));
}

public void testDefaultField() throws IOException {
QueryShardContext context = createShardContext();

{
MoreLikeThisQueryBuilder builder =
new MoreLikeThisQueryBuilder(new String[]{"hello world"}, null);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> builder.toQuery(context));
assertThat(e.getMessage(), containsString("[more_like_this] query cannot infer"));
}

{
context.getIndexSettings().updateIndexMetaData(
newIndexMeta("index",
context.getIndexSettings().getSettings(),
Settings.builder().putList("index.query.default_field", STRING_FIELD_NAME).build()
)
);
try {
MoreLikeThisQueryBuilder builder = new MoreLikeThisQueryBuilder(new String[]{"hello world"}, null);
builder.toQuery(context);
} finally {
// Reset the default value
context.getIndexSettings().updateIndexMetaData(
newIndexMeta("index",
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity: why does the default need to get changed back? I was under the impression the context doesn't get reused? between different tests? If that is not the case, maybe the context should contain a copy of the index settings so changes to it don't affect other tests. This might be something to adress in another issue though if adressing it here is too involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We update the settings for the created index that is shared among all tests in this class so we need to reset the value at the end. The query shard context is created for each test method but the index is static.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I think it would be good to change the test setup so this is something we don't have to worry about when writing tests. Probably worth a separate issue though.

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 think it's ok to share the same index on all tests. We can maybe restore the default value of the index settings after each test but it's very uncommon to change a setting in a query builder test so I don't think it's worth the change.

context.getIndexSettings().getSettings(),
Settings.builder().putList("index.query.default_field", "*").build()
)
);
}
}
}

public void testMoreLikeThisBuilder() throws Exception {
Query parsedQuery =
parseQuery(moreLikeThisQuery(new String[]{"name.first", "name.last"}, new String[]{"something"}, null)
Expand Down Expand Up @@ -390,4 +429,11 @@ protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
}
return query;
}

private static IndexMetaData newIndexMeta(String name, Settings oldIndexSettings, Settings indexSettings) {
Settings build = Settings.builder().put(oldIndexSettings)
.put(indexSettings)
.build();
return IndexMetaData.builder(name).settings(build).build();
}
}