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

Declared Object in ConstructingObjectParser is ignored in case target object is already built. #34351

Closed
bizybot opened this issue Oct 8, 2018 · 6 comments
Labels
>bug :Core/Infra/Core Core issues without another label

Comments

@bizybot
Copy link
Contributor

bizybot commented Oct 8, 2018

We found a somewhat unexpected behavior of the ConstructingObjectParser.
While parsing JSON object { "role_mapping" : { "created" : "true" } }, if we declare
inner field "create" as a constructorArg along with the field "role_mapping" for which the parser is a no-op, the target object is still constructed.

static {
        PARSER.declareBoolean(constructorArg(), new ParseField("created"));
        PARSER.declareObject((a,b) -> {}, (parser, context) -> null, new ParseField("role_mapping"));
}

We continue to sub parse the object for role_mapping and once we encounter constructorArg we build the target object. The field role_mapping object is queued but later ignored.

The behaviour seems to be similar to what we do for unknown fields or optional constructor args, but
not sure about it.

@bizybot bizybot added :Core/Infra/Core Core issues without another label team-discuss labels Oct 8, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@bizybot
Copy link
Contributor Author

bizybot commented Oct 8, 2018

Adding @nik9000 for more insights. Thank you.

@bizybot bizybot changed the title Declared Object in ConstructingObjectParser is ignored Declared Object in ConstructingObjectParser is ignored in case target object is already built. Oct 8, 2018
@nik9000
Copy link
Member

nik9000 commented Oct 16, 2018

I think I understand what is up here. I'll open up something to detect it.

@nik9000 nik9000 added >bug and removed team-discuss labels Oct 16, 2018
@nik9000
Copy link
Member

nik9000 commented Oct 17, 2018

Ok - the issue is that any parser that attempts to parse an object or a list is obligated to move to the end of that object or list. If it doesn't then all kinds of crazy things happen. You can work around this with (parser, context) -> parser.skipChildren().

@nik9000
Copy link
Member

nik9000 commented Oct 17, 2018

I'm most of the way through a change to detect "bad parsers" but fighting with some test failures.

@nik9000
Copy link
Member

nik9000 commented Oct 17, 2018

Ah! I see what you are doing now. I'll include a fix when i open a PR.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Oct 17, 2018
Adds checks for misbehaving parsers.

Closes elastic#34351
nik9000 added a commit that referenced this issue Oct 25, 2018
Adds checks for misbehaving parsers. The checks aren't perfect at all but
they are simple and fast enough that we can do them all the time so
they'll catch most badly behaving parsers.

Closes #34351
nik9000 added a commit that referenced this issue Oct 26, 2018
Adds checks for misbehaving parsers. The checks aren't perfect at all but
they are simple and fast enough that we can do them all the time so
they'll catch most badly behaving parsers.

Closes #34351
kcm pushed a commit that referenced this issue Oct 30, 2018
Adds checks for misbehaving parsers. The checks aren't perfect at all but
they are simple and fast enough that we can do them all the time so
they'll catch most badly behaving parsers.

Closes #34351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label
Projects
None yet
Development

No branches or pull requests

3 participants