Upgrade ES client to 9.0.0-alpha.4#213375
Conversation
afharo
added
Feature:elasticsearch
Team:Core
There was a problem hiding this comment.
@JoshMock, here's a missing query parameter in the spec.
There was a problem hiding this comment.
Thanks, I opened elastic/elasticsearch-specification#3954 to add it (plus a bunch of others that were also missing).
There was a problem hiding this comment.
require_alias is accepted by the IndexRequest but not the CreateRequest, so I went with the querystring passthrough for both here.
| }` | ||
| ); | ||
| const result = (await originalEqlSearch.call(opts.esClient, params, { | ||
| const result = (await originalEqlSearch.call(opts.esClient.eql, params, { |
There was a problem hiding this comment.
Seems weird that this would have worked before!
There was a problem hiding this comment.
yes... I think that the internals changed to address the body vs. query parameters... but maybe @JoshMock can provide a more accurate reason.
There was a problem hiding this comment.
This one's a bit tricky. 🤔 It may be because, in 8.x, That is effectively the same thing as Pick<Client, "transport">, whereas in 9.0 That also holds the acceptable parameters so it is no longer a subset of properties from Client.
I wasn't thinking about that side effect when making this change, but now I'm wondering if there's a better pattern to avoid this being a breaking change.
| }) ?? {}; | ||
| const byTaskType = ((aggregations.byTaskType.buckets as OverdueTaskAggBucket[]) ?? []).reduce( | ||
| (acc: Record<string, number>, bucket: OverdueTaskAggBucket) => { | ||
| // @ts-expect-error there's no way that buckets (array) matches `number` |
There was a problem hiding this comment.
Isn't the acc var typed wrong here? Seems like it should be an Record<string, something[]>? So seems like this may require a more involved fix, not clear what it would be though ...
There was a problem hiding this comment.
++ I think that the fact that FieldValue used to be any didn't highlight this wrongly typed reduce.
I didn't know what's the goal of this reduce, so I thought I'd flag it and leave it to the owners to fix it 😇
afharo
added
Team:Search
Team:Threat Hunting
|
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
baileycash-elastic
left a comment
baileycash-elastic
left a comment
There was a problem hiding this comment.
Reviewed on behalf of obs-ux-mgmt:
investigate_app
slo
observability-server (?)
changes make sense to me
| query?: estypes.QueryDslQueryContainer; | ||
| sort?: estypes.SortOptions[]; | ||
| _source?: string[] | false; | ||
| search_after?: Array<string | number>; |
There was a problem hiding this comment.
IIRC, because this way we reduce the actual definition of search_after (which includes boolean and null 🤷 )
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
cc @afharo |
20 participants
Summary
Upgrading the ES client to v9.0.0-alpha.4 to test the changes mentioned in elastic/elasticsearch-js#2584
This new version introduces some type changes, most notably, the
FieldValueisstring | number | boolean | null | undefinedinstead ofany, leading to some new type checks to be implemented (like on aggregation resultsbucket.key,search_after, andsortoptions).On top of that, it adds the new behavior where unknown properties are placed in the
body(when the request has a body). If they must be in as a query parameter, they should be placed under thequerystringoption.cc @JoshMock
TODO:
querystring: {})I will address
// @ts-expect-error elasticsearch@9.0.0 https://github.com/elastic/elasticsearch-js/issues/2584in a separate PR to reduce noise.Related #208776