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 should throw parse exception if no field or script is specified #48824

Closed
polyfractal opened this issue Nov 1, 2019 · 4 comments · Fixed by #49661
Closed

Aggregations should throw parse exception if no field or script is specified #48824

polyfractal opened this issue Nov 1, 2019 · 4 comments · Fixed by #49661
Assignees

Comments

@polyfractal
Copy link
Contributor

Today if you specify an aggregation with no field or script like so:

{
  "size": 0,
  "aggs": {
    "the_terms": {
      "terms": {
        "size": 10
      }
    }
  }
}

We throw an exception:

{
    "type": "illegal_state_exception",
    "reason": "value source config is invalid; must have either a field context or a script or marked as unwrapped"
}

The problem is that the exception is generated from what is an otherwise invalid ValuesSourceConfig object. E.g. when resolving we identify that there is no script or field, generate an invalid VSConfig, then later at the factory layer check to see if the VSConfig is valid and throw an exception.

Besides being confusing from a code perspective, it also means we don't throw until we've already broadcast the request to the cluster. We should instead fail during parsing because there is no sensible reason to omit both field and script (I don't think?)

Note: There are a few instances (e.g. ParentAggregationBuilder) that use the mutability of VSConfig which may be why we ended up in this situation. But even if we don't fix that part of VSConfig, we can head off these scenarios at the parse layer first.

Related to #42949

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@ejmiers
Copy link
Contributor

ejmiers commented Nov 2, 2019

Hi, I'm fairly new to contributing to es. Is this something I could work on?

@polyfractal
Copy link
Contributor Author

Hi @ejmiers, sorry for the delay! Been traveling for work lately and my inbox has been a mess.

You can certainly work on this issue if you like! It shouldn't be too difficult, but also not the easiest first issue either. The actual change is relatively small but it requires knowing how our static json parsers work which can be a little tricky.

If you'd like to work on it I'm happy to help. You might also want to skim through the list of issues tagged with help needed or good first issue which have been specifically called out as items which are good for first time contributors

@ejmiers
Copy link
Contributor

ejmiers commented Nov 18, 2019

Ok, I think maybe I'll leave this one for someone else to tackle. Thanks for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants