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

Remove deprecated 'body' key from Request interfaces #116102

Open
mshustov opened this issue Oct 25, 2021 · 9 comments
Open

Remove deprecated 'body' key from Request interfaces #116102

mshustov opened this issue Oct 25, 2021 · 9 comments
Labels
Feature:elasticsearch impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

elasticsearch-js client deprecated body key usage in request interfaces. body key support will be removed in the following major release.
Kibana codebase should be upgraded accordingly to prepare for this breaking change.

// from
const response = await client.search({
  index: 'test',
  body: {
    query: {
      match_all: {}
    }
  }
})

// to
const response = await client.search({
  index: 'test',
  query: {
    match_all: {}
  }
})

Right now it affects DX of elasticsearch service mocks. Since consumers of the mocks have to declare a compatible interface explicitly.

(esClient.bulk.mock.calls[0][0] as estypes.BulkRequest)?.body
@mshustov mshustov added Feature:elasticsearch Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Oct 25, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Nov 1, 2021
@mshustov mshustov added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated and removed loe:small Small Level of Effort labels Nov 2, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:needs-research This issue requires some research before it can be worked on or estimated impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Nov 2, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:large Large Level of Effort and removed loe:small Small Level of Effort labels Nov 30, 2021
@pgayvallet
Copy link
Contributor

@elastic/clients-team do you know if removing the body key from the client's signature is still something planned?

@JoshMock
Copy link
Member

JoshMock commented Jul 8, 2024

@pgayvallet I would like to include that as part of 9.0. It would significantly simplify the type definitions for each API.

@TinaHeiligers
Copy link
Contributor

@JoshMock any updates on plans that we need to know about?

@JoshMock
Copy link
Member

The plan is tentative, primarily because I wanted to check back in with you all on how you're feeling about it. 🙂 What's the overall sentiment?

While it does make sense to remove something that's deprecated (albeit, deprecated before my time as maintainer), automated codegen does all the work to maintain the second set of type definitions, so the effort isn't any higher. The only pain it really presents, then, is the readability of the generated API function code (e.g. T.CatAliasesRequest | TB.CatAliasesRequest would become T.CatAliasesRequest for all functions). On the whole, that's a pretty small annoyance, just multiplied by a few hundred functions.

In other words, it's a nice-to-have, but only for aesthetic/readability purposes.

@afharo
Copy link
Member

afharo commented Sep 24, 2024

I'm ++ to having them removed and simplify the types. However, the hardest part for us to to identify all usages of body in the Kibana codebase. I wonder if there's any "easy" way to identify all usages so that we can tackle them. body is a widely used word in many different scopes, so a binary search doesn't really help 😅

@JoshMock
Copy link
Member

@afharo When I get to some other 9.0 changes I hope to make, I can publish an alpha release to npm that drops body types. That way you'd be able to run your test suite and get an idea of how widespread it will be.

@afharo
Copy link
Member

afharo commented Sep 25, 2024

@JoshMock, that sounds like a great approach. I wonder how many usages are missing tests (and implemented in .js so types won't complain). But we can hope for the best and for QA in beta versions to highlight any remaining uncaught usage.

cc @elastic/kibana-core, how does that sound?

@JoshMock
Copy link
Member

I wonder how many usages are missing tests (and implemented in .js so types won't complain)

If I totally remove body support, you might know at run time if the request bodies are validated during your tests. If not, I could push a branch to Github that also throws an error if a body property is used. Then you could clone, build and install that branch and test Kibana against that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:elasticsearch impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants