Skip to content

Limit the number of nested documents #27405

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
Nov 22, 2017
Merged

Limit the number of nested documents #27405

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
d59543a
Limit the number of nested documents
mayya-sharipova Nov 15, 2017
a433f7d
Limit the number of nested documents
mayya-sharipova Nov 20, 2017
e45f7bb
Limit the number of nested documents
mayya-sharipova Nov 21, 2017
5d42738
Limit the number of nested documents
mayya-sharipova Nov 22, 2017
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
33 changes: 20 additions & 13 deletions core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,35 +525,42 @@ public void testParentObjectMapperAreNested() throws Exception {
}

public void testLimitNestedDocsDefaultSettings() throws Exception{
MapperService mapperService = createIndex("test1", Settings.builder().build()).mapperService();
Settings settings = Settings.builder().build();
MapperService mapperService = createIndex("test1", settings).mapperService();
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties")
.startObject("nested1").field("type", "nested").endObject()
.endObject().endObject().endObject().string();
DocumentMapper docMapper = mapperService.documentMapperParser().parse("type", new CompressedXContent(mapping));
long defaultMaxNoNestedDocs = MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.get(settings);

// parsing a doc with 3 nested objects succeeds
// parsing a doc with No. nested objects > defaultMaxNoNestedDocs fails
XContentBuilder docBuilder = XContentFactory.jsonBuilder();
docBuilder.startObject();
{
docBuilder.startArray("nested1");
{
docBuilder.startObject().field("field1", "11").field("field2", "21").endObject();
docBuilder.startObject().field("field1", "12").field("field2", "22").endObject();
docBuilder.startObject().field("field1", "13").field("field2", "23").endObject();
for(int i=0; i<=defaultMaxNoNestedDocs; i++) {
Copy link
Member

@cbuescher cbuescher Nov 22, 2017

Choose a reason for hiding this comment

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

nit: some whitespaces around the operators etc... would be nice

docBuilder.startObject().field("f", i).endObject();
Copy link
Member

@cbuescher cbuescher Nov 22, 2017

Choose a reason for hiding this comment

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

I'm suprised this triggers the no. fields warnings since "f" is not the field thats defined with the "nested" type above? I would have expected this to not trigger the limit but the exception tested below seems to indicate it triggers the setting. What am I missing?

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Nov 22, 2017

Choose a reason for hiding this comment

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

@cbuescher Please correct me if I misunderstood your previous comment: "could you change this to that the the document has more than the default number of nested docs (simple loop) and gets rejected".
This is what this new test is about.
We have a parent nested field nested1 under which we have many json objects with a field f, and since the number of these json objects is more than the default number, we get an error.

Copy link
Member

@cbuescher cbuescher Nov 22, 2017

Choose a reason for hiding this comment

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

Thanks, I probably read the test to quickly and missed the startArray() part. You are absolutely right, this is how I expected the test. Sorry for the noise.

}
}
docBuilder.endArray();
}
docBuilder.endObject();
SourceToParse source1 = SourceToParse.source("test1", "type", "1", docBuilder.bytes(), XContentType.JSON);
ParsedDocument doc = docMapper.parse(source1);
assertThat(doc.docs().size(), equalTo(4));
MapperParsingException e = expectThrows(MapperParsingException.class, () -> docMapper.parse(source1));
assertEquals(
"The number of nested documents has exceeded the allowed limit of [" + defaultMaxNoNestedDocs
+ "]. This limit can be set by changing the [" + MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey()
+ "] index level setting.",
e.getMessage()
);
}

public void testLimitNestedDocs() throws Exception{
// setting limit to allow only two nested objects in the whole doc
long nested_docs_limit = 2L;
long maxNoNestedDocs = 2L;
Copy link
Member

@cbuescher cbuescher Nov 22, 2017

Choose a reason for hiding this comment

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

just as feedback, nothing to change really, but I liked the previous variable name better ;-)

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Nov 22, 2017

Choose a reason for hiding this comment

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

@cbuescher thanks for the feedback, I thought in the previous variable names I did not follow Java camel case standards for variable names (was more of python style)

Copy link
Member

@cbuescher cbuescher Nov 22, 2017

Choose a reason for hiding this comment

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

I see, and you are right, camel case is preferred. I probably misread the "NoNestedDocs" part of the name as "no nested docs" and that confused me for a second, but either way is fine.

Copy link
Contributor Author

@mayya-sharipova mayya-sharipova Nov 22, 2017

Choose a reason for hiding this comment

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

@cbuescher thanks Christoph, you are rightNoNestedDocs is confusing, I will avoid using No for "number of" in the future.

MapperService mapperService = createIndex("test1", Settings.builder()
.put(MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey(), nested_docs_limit).build()).mapperService();
.put(MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey(), maxNoNestedDocs).build()).mapperService();
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties")
.startObject("nested1").field("type", "nested").endObject()
.endObject().endObject().endObject().string();
Expand Down Expand Up @@ -591,7 +598,7 @@ public void testLimitNestedDocs() throws Exception{
SourceToParse source2 = SourceToParse.source("test1", "type", "2", docBuilder2.bytes(), XContentType.JSON);
MapperParsingException e = expectThrows(MapperParsingException.class, () -> docMapper.parse(source2));
assertEquals(
"The number of nested documents has exceeded the allowed limit of [" + nested_docs_limit
"The number of nested documents has exceeded the allowed limit of [" + maxNoNestedDocs
+ "]. This limit can be set by changing the [" + MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey()
+ "] index level setting.",
e.getMessage()
Expand All @@ -600,9 +607,9 @@ public void testLimitNestedDocs() throws Exception{

public void testLimitNestedDocsMultipleNestedFields() throws Exception{
Copy link
Member

@cbuescher cbuescher Nov 17, 2017

Choose a reason for hiding this comment

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

This is interesting (and a great test by the way), because I just realized that the maximum number of nested docs check is applied across all nested fields in the document. I didn't immediately get that from the desciption in #26962 or the PR so far, maybe worth adding to the migration notest and docs as well.

// setting limit to allow only two nested objects in the whole doc
long nested_docs_limit = 2L;
long maxNoNestedDocs = 2L;
MapperService mapperService = createIndex("test1", Settings.builder()
.put(MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey(), nested_docs_limit).build()).mapperService();
.put(MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey(), maxNoNestedDocs).build()).mapperService();
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties")
.startObject("nested1").field("type", "nested").endObject()
.startObject("nested2").field("type", "nested").endObject()
Expand Down Expand Up @@ -650,7 +657,7 @@ public void testLimitNestedDocsMultipleNestedFields() throws Exception{
SourceToParse source2 = SourceToParse.source("test1", "type", "2", docBuilder2.bytes(), XContentType.JSON);
MapperParsingException e = expectThrows(MapperParsingException.class, () -> docMapper.parse(source2));
assertEquals(
"The number of nested documents has exceeded the allowed limit of [" + nested_docs_limit
"The number of nested documents has exceeded the allowed limit of [" + maxNoNestedDocs
+ "]. This limit can be set by changing the [" + MapperService.INDEX_MAPPING_NESTED_DOCS_LIMIT_SETTING.getKey()
+ "] index level setting.",
e.getMessage()
Expand Down
4 changes: 2 additions & 2 deletions rest-api-spec/src/main/resources/rest-api-spec/test/create/70_nested.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ setup:
"Indexing a doc with No. nested objects less or equal to index.mapping.nested_objects.limit should succeed":
- skip:
version: " - 6.99.99"
reason: index.mapping.nested_objects setting was introduced from this version
reason: index.mapping.nested_objects setting has been added in 7.0.0
- do:
create:
index: test_1
Expand All @@ -30,7 +30,7 @@ setup:
"Indexing a doc with No. nested objects more than index.mapping.nested_objects.limit should fail":
- skip:
version: " - 6.99.99"
reason: index.mapping.nested_objects setting was introduced from this version
reason: index.mapping.nested_objects setting has been added in 7.0.0
- do:
catch: /The number of nested documents has exceeded the allowed limit of \[2\]. This limit can be set by changing the \[index.mapping.nested_objects.limit\] index level setting\./
create:
Expand Down