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

GEO: More robust handling of ignore_malformed in geoshape parsing #35603

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Nov 15, 2018

Adds a method to XContent parser to skip all children of a current
element in case of the parsing failure and applies this method to be
able to ignore the rest of the GeoJson shape if the parsing fails and
we need to ignore the geoshape due to the ignore malformed flag.

Supersedes #34498

Closes #34047

Adds a method to XContent parser to skip all children of a current
element in case of the parsing failure and applies this method to be
able to ignore the rest of the GeoJson shape if the parsing fails and
we need to ignore the geoshape due to the ignore malformed flag.

Supersedes elastic#34498

Closes elastic#34047
@imotov imotov added >bug :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 v6.6.0 labels Nov 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@imotov
Copy link
Contributor Author

imotov commented Nov 15, 2018

EDIT: wrong nik. sorry

@nik9000 github suggested you, but if you think somebody else is in a better position to review the XContentParser portion, please feel free to reassign.

@jpountz
Copy link
Contributor

jpountz commented Nov 16, 2018

I like the idea but at the same time I'd like to avoid adding more methods to XContentParser. I'm wondering whether we could do something similar with a wrapper instead that would count levels similarly to what you are doing in your Mark impl?

@imotov
Copy link
Contributor Author

imotov commented Nov 16, 2018

@jpountz that sounds like a great idea! Let me try that.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

This looks great, I just left a question.

public XContentSubParser(XContentParser parser) {
this.parser = parser;
if (parser.currentToken() != Token.START_OBJECT) {
throw new IllegalStateException("The sub parser has to be created on the start of an object");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


@Override
public boolean isClosed() {
return parser.isClosed();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether the fact that this would still return false after close has been called could be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! I missed this one. Good catch! I will fix that.

@imotov imotov merged commit 39789d0 into elastic:master Nov 21, 2018
imotov added a commit that referenced this pull request Nov 22, 2018
…5603)

Adds an XContent sub parser class that can to wrap another
XContent parser at the beginning of an object and allow skiping
all children in case of the parsing failure. It also uses this
subparser to ignore the rest of the GeoJson shape if the
parsing fails and we need to ignore the geoshape due to the
ignore_malformed flag.

Supersedes #34498

Closes #34047
@imotov imotov deleted the issue-34047-more-robust-error-handling-in-geojson-parser-ii branch May 1, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >bug v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants