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.

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

Jump to bottom

Decouple NamedXContentRegistry from ElasticsearchException #29253

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Decouple NamedXContentRegistry from ElasticsearchException #29253

merged 5 commits into from
Mar 27, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Mar 26, 2018 •
edited
Loading

This commit decouples NamedXContentRegistry from using either
ElasticsearchException, ParsingException, or UnknownNamedObjectException.

This will allow us to move NamedXContentRegistry to its own lib as part of the
xcontent extraction work.

Relates to #28504

@dakrone
Decouple NamedXContentRegistry from ElasticsearchException
483803a 
This commit decouples `NamedXContentRegistry` from using either
`ElasticsearchException`, `ParsingException`, or `UnknownNamedObjectException`.

This will allow us to move NamedXContentRegistry to its own lib as part of the
xcontent extraction work.

Relates to elastic#28504
@dakrone dakrone added >non-issue :Core/Infra/Core Core issues without another label v7.0.0 v6.3.0 labels Mar 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

cbuescher
cbuescher approved these changes Mar 27, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

LGTM

nik9000
nik9000 reviewed Mar 27, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java Outdated
@@ -316,11 +317,11 @@ public static QueryBuilder parseInnerQueryBuilder(XContentParser parser) throws
QueryBuilder result;
try {
result = parser.namedObject(QueryBuilder.class, queryName, null);
} catch (UnknownNamedObjectException e) {
} catch (XContentParseException e) {
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to use this XContentParseException for anything other that unknown named objects we shouldn't catch it here. We need to be sure this is an unknown named object. Either we should rename the exception or make a subclass of it for unknown named objects, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's probably a good idea, for now there are no other uses of this exception, but if that changes in the future I will make a subclass of it for this.

nik9000
nik9000 reviewed Mar 27, 2018
View reviewed changes
server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java Outdated
@@ -1007,22 +1008,20 @@ public void testNamedObject() throws IOException {
{
p.nextToken();
assertEquals("test", p.namedObject(Object.class, "str", null));
UnknownNamedObjectException e = expectThrows(UnknownNamedObjectException.class,
XContentParseException e = expectThrows(XContentParseException.class,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be NamedObejectNotFoundException instead, right? It is more specific.

server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java Outdated
}
{
Exception e = expectThrows(ElasticsearchException.class, () -> p.namedObject(String.class, "doesn't matter", null));
assertEquals("Unknown namedObject category [java.lang.String]", e.getMessage());
Exception e = expectThrows(XContentParseException.class, () -> p.namedObject(String.class, "doesn't matter", null));
Copy link
Member

Choose a reason for hiding this comment

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

Here too

server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java Outdated
}
{
XContentParser emptyRegistryParser = xcontentType().xContent()
.createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, new byte[] {});
Exception e = expectThrows(ElasticsearchException.class,
Exception e = expectThrows(XContentParseException.class,
Copy link
Member

Choose a reason for hiding this comment

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

And here!

server/src/test/java/org/elasticsearch/search/rescore/QueryRescorerBuilderTests.java Outdated
@@ -220,8 +221,8 @@ public void testUnknownFieldsExpection() throws IOException {
"}\n";
{
XContentParser parser = createParser(rescoreElement);
Exception e = expectThrows(ParsingException.class, () -> RescorerBuilder.parseFromXContent(parser));
assertEquals("Unknown RescorerBuilder [bad_rescorer_name]", e.getMessage());
Exception e = expectThrows(XContentParseException.class, () -> RescorerBuilder.parseFromXContent(parser));
Copy link
Member

Choose a reason for hiding this comment

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

All of these should be NamedObjectNotFoundException I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I changed all the occurences, thanks

nik9000
nik9000 approved these changes Mar 27, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

👍

@dakrone dakrone merged commit eebda69 into elastic:master Mar 27, 2018
dakrone added a commit that referenced this pull request Mar 27, 2018
@dakrone
Decouple NamedXContentRegistry from ElasticsearchException (#29253)
adafb7e 
* Decouple NamedXContentRegistry from ElasticsearchException

This commit decouples `NamedXContentRegistry` from using either
`ElasticsearchException`, `ParsingException`, or `UnknownNamedObjectException`.

This will allow us to move NamedXContentRegistry to its own lib as part of the
xcontent extraction work.

Relates to #28504
martijnvg added a commit that referenced this pull request Mar 28, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
ffb5281 
* es/master: (22 commits)
  Fix building Javadoc JARs on JDK for client JARs (#29274)
  Require JDK 10 to build Elasticsearch (#29174)
  Decouple NamedXContentRegistry from ElasticsearchException (#29253)
  Docs: Update generating test coverage reports (#29255)
  [TEST] Fix issue with HttpInfo passed invalid parameter
  Remove all dependencies from XContentBuilder (#29225)
  Fix sporadic failure in CompositeValuesCollectorQueueTests
  Propagate ignore_unmapped to inner_hits (#29261)
  TEST: Increase timeout for testPrimaryReplicaResyncFailed
  REST client: hosts marked dead for the first time should not be immediately retried (#29230)
  TEST: Use different translog dir for a new engine
  Make SearchStats implement Writeable (#29258)
  [Docs] Spelling and grammar changes to reindex.asciidoc (#29232)
  Do not optimize append-only if seen normal op with higher seqno (#28787)
  [test] packaging: gradle tasks for groovy tests (#29046)
  Prune only gc deletes below local checkpoint (#28790)
  remove testUnassignedShardAndEmptyNodesInRoutingTable
  #28745: remove extra option in the composite rest tests
  Fold EngineDiskUtils into Store, for better lock semantics (#29156)
  Add file permissions checks to precommit task
  ...
martijnvg added a commit that referenced this pull request Mar 28, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
305083b 
* es/6.x:
  Fix building Javadoc JARs on JDK for client JARs (#29274)
  Require JDK 10 to build Elasticsearch (#29174)
  Decouple NamedXContentRegistry from ElasticsearchException (#29253)
  Docs: Update generating test coverage reports (#29255)
  [TEST] Fix issue with HttpInfo passed invalid parameter
  Remove all dependencies from XContentBuilder (#29225)
  Fix sporadic failure in CompositeValuesCollectorQueueTests
  Propagate ignore_unmapped to inner_hits (#29261)
  TEST: Increase timeout for testPrimaryReplicaResyncFailed
  REST client: hosts marked dead for the first time should not be immediately retried (#29230)
  Make SearchStats implement Writeable (#29258)
  [Docs] Spelling and grammar changes to reindex.asciidoc (#29232)
  [test] packaging: gradle tasks for groovy tests (#29046)
  remove testUnassignedShardAndEmptyNodesInRoutingTable
  Add file permissions checks to precommit task
  Remove execute mode bit from source files
  #28745: remove 7.x option in the composite rest tests.
  Optimize the composite aggregation for match_all and range queries (#28745)
  Clarify deprecation warning for auto_generate_phrase_query (#29204)
@dakrone dakrone deleted the xcontent-exceptions-of-dooooom branch April 19, 2018 14:44
@polyfractal polyfractal mentioned this pull request Mar 19, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

@cbuescher cbuescher cbuescher approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dakrone @elasticmachine @nik9000 @cbuescher @colings86