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

Aggregations parsing is too lenient. #5837

Closed
wants to merge 1 commit into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 16, 2014

Our parser currently accepts invalid aggregations definitions, this might trigger unexpected results like #5827.

Close #5827

@s1monw
Copy link
Contributor

s1monw commented Apr 16, 2014

it looks cleaner now and good to me but I guess I'll let @uboness take a look at well!

@polyfractal
Copy link
Contributor

Would be great if this could be expanded to include the usual parsing problems, like doubling up fields. For example:

{
  "aggs" : {
    "my_agg" : {
       "terms" : {
          "field" : "a",
          "field" : "b"
       }
    }
  }
}

This agg will only use the b field when building the terms bucket. The a field is silently ignored and no exception is thrown, which is very confusing for users unfamiliar with pull parsing.

@jpountz
Copy link
Contributor Author

jpountz commented Apr 24, 2014

I agree this should not fail silently. I think the best way to fix this issue would be to wrap the parser in order to make sure that objects don't contain duplicate keys.

@s1monw
Copy link
Contributor

s1monw commented Apr 24, 2014

@jpountz I like the idea of having a parser wrapper that ensures we have unique keys. Yet, that is a general thing and should be a different issue?

@jpountz
Copy link
Contributor Author

jpountz commented Apr 24, 2014

+1 on a different issue

@uboness
Copy link
Contributor

uboness commented Apr 24, 2014

been thinking about it (dup keys that is)... not sure about it... wrapping the parser to check that in a generic manner comes with a cost (keeping track of all keys)... I'm wondering if it's something we should just keep simple and as cheap as possible at the price of leniency

.endObject()
.endObject()).execute().actionGet();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add a test for:

  • agg type is missing
  • 2 agg types definitions
  • 2 "aggs" definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a test for "2 agg types", I'll add the 2 other ideas.

@uboness
Copy link
Contributor

uboness commented Apr 28, 2014

left a comment on missing tests... other than that, LGTM

@jpountz jpountz closed this Apr 29, 2014
@jpountz jpountz deleted the fix/aggregation_parsing branch April 29, 2014 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cardinality not working as expected
5 participants