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

fields added to top_hits aggregation #2209

Closed
wants to merge 1 commit into from

Conversation

semd
Copy link

@semd semd commented Apr 8, 2024

The following query works and returns the fields information requested in the top_hits aggregation. However, the typescript definition for top hits aggregations (AggregationsTopHitsAggregation) is missing the fields entry.

{
  "aggregations": {
    "theAggName": {
      "top_hits": {
        "fields": [
          {
            "field": "@timestamp",
            "format": "strict_date_optional_time"
          },
          "_index"
        ]
      }
    }
  }
}

So typescript throws the following error:
Object literal may only specify known properties, but 'fields' does not exist in type 'AggregationsTopHitsAggregation'. Did you mean to write 'field'?

This PR adds the fields property to the AggregationsTopHitsAggregation type with:

fields?: (QueryDslFieldAndFormat | Field)[]

I've tested it works using the QueryDslFieldAndFormat and string types, and the array is required.

@semd semd added the typescript label Apr 8, 2024
@semd semd self-assigned this Apr 8, 2024
@semd
Copy link
Author

semd commented Apr 8, 2024

fwiw, this comes from a Kibana PR comment elastic/kibana#178893 (comment)

@JoshMock
Copy link
Member

JoshMock commented Apr 8, 2024

@semd Thanks for this. It's best we look at the underlying specification to fix missing types like this, so that all clients benefit from the fix.

I'll see about fixing the spec soon, unless you happen to get to it before I can.

@semd
Copy link
Author

semd commented Apr 9, 2024

@JoshMock That sounds good. I didn't know about this process, I just saw the type missing and jumped into the fix.
We can follow the conventional way, I already created a local type to solve the problem in Kibana.
Please let me know when the spec is fixed so I can update our side. Thanks!

@JoshMock
Copy link
Member

I've opened elastic/elasticsearch-specification#2522 to solve this in the spec. Code generation should pick it up and include it in the next release.

@JoshMock JoshMock closed this Apr 18, 2024
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.

2 participants