Geo Point parse error fix

[henningandersen]

When geo point parsing threw a parse exception, it did not consume
remaining tokens from the parser. This in turn meant that
indexing documents with malformed geo points into mappings with
ignore_malformed=true would fail in some cases, since DocumentParser
expects geo_point parsing to end on the END_OBJECT token.

Related to #17617

[elasticmachine]

Pinging @elastic/es-analytics-geo

[imotov]

We had the same issues on geoshape side and used a slightly different approach in geoshape parsing. We now have a special wrapper for XContentParser that allows skipping children in case of a parse error regardless on where the error occurred. It allows us to have one catch all section instead of thinking about every single path that things can go wrong. It also guarantees that we will never consume more tokens than we are supposed to or leave some hanging around. Would you be interested in trying this approach instead?

[henningandersen]

Thanks for the tip on the sub parser, @imotov. I adjusted to use that approach in e4791a9.

I changed the sub parser to also support arrays (since they have the same fundamental issue) and chose to also support non-object/non-arrays (this makes it much simpler to use at least for the geo point case).

Forgot to fix XContentParserTests, will look into that tomorrow morning.

[henningandersen]

@imotov this should now be ready for another review. A few notable changes:

1. sub parser: currentToken is now the same value as last call to nextToken (including null) and fixed all methods to mimick end of doc behaviour.
2. as mentioned above: allowing sub parser on both arrays and values (ended up allowing any token) is desirable at least in the geo points case.

Thinking more about this, it could be desirable to let the DocumentParser create the sub parser, ensuring that individual mappers do not have to consider this. I chose not to pursue this for now (larger effort).

[imotov]

allowing sub parser on both arrays and values (ended up allowing any token) is desirable at least in the geo points case.

Could you explain the motivation behind supporting values a bit more? The original intent of the subparser was to create a parsing context that the parser cannot escape or underconsume in case of an error. In case of the object the context is the entire object until the corresponding closing bracket. I can clearly see how this logic can be extended to arrays, where the context is all elements until the closing ]. However, what is the context in case of the value? It seems like it is just this value and the parser wrapper seems to be a bit overkill here since you already have the value and you cannot really underconsume or overconsume it accidently.

[henningandersen]

There are two primary motivations for also supporting value:

1. It makes it possible to wrap any "object", regardless of string, number or complex object. This in turn means that any parsing functionality can easily be wrapped in a sub parser. This is used in GeoUtils here: https://github.com/elastic/elasticsearch/pull/40447/files#diff-ac59342091d31c8a6b2b5a349712bac6R434 since parsing also supports string here: https://github.com/elastic/elasticsearch/pull/40447/files#diff-ac59342091d31c8a6b2b5a349712bac6R526 . The alternative would be to selectively construct the sub parser only for object and array, which makes the codd less clean.
2. The subparser really does two things:

• In case of errors close will find matching END token. (the original motivation).
• It disallows parsing beyond the current object. This part is also desirable for values, to ensure that parsing a value does not call nextToken (exactly like we expect an object/array to stop on END).

[imotov]

I feel like considering the amount of complexity that supporting a single value adds, it might be easier to just wrap after we encountered an object or an array instead of blindly wrapping every time on the outside. Basically just wrap here and here. This case we can get away with all the complexity of handling values.

[henningandersen]

I feel like considering the amount of complexity that supporting a single value adds,

I am not sure I follow you on this, the change to support values was really a one liner (initialize level to 1 or 0 depending on current token).

The rest of the checks were added to support that whatever comes out of nextToken() will also come out of currentToken(). Before this, you could forward the subparser beyond the END, get null back from nextToken() and then call currentToken() or any of the other accessor methods to get the value. This is an existing issue before this PR, but now that we start using the sub parser more it made sense to me to ensure this property holds also for the sub parser.

I do not mind following your suggestion, but I wanted to clarify above first to ensure this was included in your evaluation before I do the changes.

[imotov]

Before this, you could forward the subparser beyond the END

I agree that we should fix currentToken and currentName, but I think all other accessors method become only the real issue if you allow stopping on values not on the end of object or array. Otherwise, you are calling them on the "}" or "]", they return some nonsense, but I don't think we have to make them consistent with nonsense that is returned when you go beyond the end with normal parser.

In any case I would prefer not to wrap the values, since it gives too much runtime overhead for too little value in my opinion.

[henningandersen]

@imotov, reverted my previous changes and made a minimalistic change to fix the issue reported. Please have another look.

[imotov]

LGTM! Thanks for doing this! Really appreciate it.

[ywelsch]

Talked to @imotov and we think this should be backported up to 6.7.

[henningandersen]

Backported to 7.x, 7.0 and 6.7.