Added Request/Response types definitions #970

delvedor · 2019-09-18T15:51:45Z

With this pr we introduce the response type definitions for the following APIs:

Index
Create
Update
Delete
Search
MSearch
Bulk

For now, we will not support more APIs because we are studying a more structured approach to be able to generate all the response definitions automatically.

Closes: #933

wylieconlon · 2019-09-18T15:58:48Z

api/ResponseTypes.ts

+    total: {
+      value: number;
+      relation: string;
+    };


This type is only correct when the server is running Elasticsearch 7.0 and above, and only when the request did not set the property rest_total_hits_as_int:

rest_total_hits_as_int (Optional, boolean) Indicates whether hits.total should be rendered as an integer or an object in the rest search response. Defaults to false.

For now, this type definitions will land only for 7.x and above, so it shouldn't be an issue.

and only when the request did not set the property rest_total_hits_as_int:

How would you solve this? The easiest solutions that come to my mind is to use a union, in the same way I did in BulkItems.

I think that would create extra work for the 80% case. If possible, I would suggest making the request object into a parameter on the type definition. I've made an example here for how I would write these types:

https://www.typescriptlang.org/play/index.html#code/JYOwLgpgTgZghgYwgAgEJwM4QMoTlBACwCUIMAHAexC2QG8AoZZ5OAczagjbjGGowB+AFzIAriADWISgHcQAbgYBfBgzABPcilz4ipCgIgAeUgEcxZMMgC84qTPkA+W8nOWM1iAA9IIACYY9MhcngD6YJRgcAA2YYTAYBhhmGGgYKJgUJbIysiCTCzoWLoEJGRUNCgAZPSFLCwJSaIgYgC2AEbQ9bnIosU4eGUGlbS1jA0NTRiidMgAbrGWLe1dUAohEDG8-CCiAORskPvIAD7I+xBm+6oNykoMCALWoaMQAHKr0KKl+hVGxjmoTAESisXiiWSqXSmWyKGULjsE0akNEAFYADQ9dicbg7ASzZRY+5qJ40F7-KoAeQ6ACsIAgMshfuVDFVAZtwpFonFpilkjDkPAYrQEa5kcxprMFksIOiMZttnxqAcricidiOFweMqaITiQ8yZ5OW8afTGQAmH5DP5s2hInpS4KLGLLZCYxX4vYXNW5LENHHar0zegakkMIA

Also, just for general awareness, I'd recommend looking at a discriminated union feature.

Anyway, what looks simpler to me is just using two separate overloads:

// Don't export this one, because it is likely to be removed when the legacy interface is removed interface BasicSearchResponse { /* all stable fields */ } export interface SearchResponse extends BasicSearchResponse { hits: { total: { value: number, relation: "gte" | "eq" // @delvedor I suggest using an explicit string literal union here instead of just a `string` } } } // legacy, thus the interface name is verbose: export interface SearchResponseWithTotalHintsAsInt extends BasicSearchResponse { hits: { total: number } } // don't export it for the same reason as BasicSearchResponse interface BasicSearchParams { /* all fields except for rest_total_hits_as_int */ } export interface SearchParams extends BasicSearchParams { rest_total_hits_as_int?: false; } export interface SearchParamsWithTotalHintsAsInt extends BasicSearchParams { rest_total_hits_as_int: true; } export class Client { search(params: SearchParams): Promise<SearchResponse>; // @deprecated: please use other overload search(params: SearchParamsWithTotalHintsAsInt): Promise<SearchResponseWithTotalHintsAsInt>; }

api/ResponseTypes.ts

lesyk-lesyk · 2020-02-25T14:57:56Z

@delvedor Any updates on this PR?

Should I define SearchResponse / Explanation / ShardsResponse etc in my code like describe in documentation here?
https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/6.x/typescript.html

Or should I wait for adding these types directly in the Elastic package?

delvedor

Hello everyone! After #1119 and #1132 I finally start working again on this proposal.
I've just pushed a few changes that can be summarized as follows.

We have two files that contain all we know about the Elasticsearch's API, which can be found in api/RequestTypes.d.ts and api/ResponseTypes.d.ts. The first one is generated from the rest-api-spec and provides every parameter aside from the body, which can be defined with a generic.
The response types at the moment are maintained by hand, so we can't expose all of them right now. We decided to start with the most commonly used APIs, which are already listed in the pr description. The code generation is aware of this list and will use the definition we have instead of a generic object definition.
All those types will be exposed through the client, for example:

import { Client, SearchRequest, SearchResponse } from '@elastic/elasticsearch'

By default, all APIs that have a dynamic element inside, such as Search, will accept a generic that defaults to unknown, if you want to give it a specific definition, you have two ways:

import { Client, SearchResponse } from '@elastic/elasticsearch'

interface Source {
  foo: string
}

// without a generic
const response = await client.search({
  index: 'test',
  body: {
    query: {
      match: { foo: 'bar' }
    }
  }
})

const source = response.body.hits.hits[0]._source as Source

// override the default generic
const response = await client.search<SearchResponse<Source>>({
  index: 'test',
  body: {
    query: {
      match: { foo: 'bar' }
    }
  }
})

const source = response.body.hits.hits[0]._source // this will be Source

Feedback is welcome!
cc @restrry @wylieconlon @joshdover @dgieselaar @nreese @spalger

Veetaha · 2020-04-07T10:27:15Z

api/ResponseTypes.d.ts

+      relation: string
+    }
+    max_score: number
+    hits: Array<{


@delvedor I'd suggest extracting this inline object literal type into an interface. I in my code usually use not only the Source type, but the whole hit type because there is an id in it I'd like to use:

interface Doc<TSource> { _id: string; _source: TSource; }

If we don't extract this into an interface we won't be able to write the following code:

interface MyDocSource { foo: number } // The type of hit should be Doc<MyDocSource> but we don't have the Doc type as per this PR right now function processHit(hit: ??) { console.log("doc with id", hit._id, "has foo:", hit._source.foo); } const res = await client.search<MyDocSource>({ ... }); // notice that we cannot annotate the docHit param type with an interface name here too res.body.hits.hits.forEach(docHit => processHit(docHit))

+1 here. I hit this problem a couple of days ago. An example from Kibana codebase:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/logstash/server/types.ts#L11-L12

Veetaha · 2020-04-07T10:27:24Z

@delvedor won't it be error-prone if the user can fully override the response type here?

client.msearch<MSearchResponse<Source>>(...);

The user can very easily mistakenly write an incorrect response type:

// notice the `M` prefix is mistakenly omitted
client.msearch<SearchResponse<Source>>(...);

My gut feeling is that we should pass not the response type as a whole, but only the generic parameters of that response type if there are any:

// Now this is way less verbose and less error-prone
client.msearch<Source>(...);

dgieselaar · 2020-04-07T10:33:42Z

@delvedor looks good to me, this should allow us to tack our types on top of those of the client.

Veetaha · 2020-04-07T10:42:33Z

api/ResponseTypes.d.ts

+  hits: {
+    total: {
+      value: number
+      relation: string


Suggested change

relation: string

relation: "eq" | "gte"

I'm surprised that we are able to generate request types from the API spec, but no responses. https://github.com/elastic/elasticsearch/blob/d7bbc9df1d3edf47f8d1c2f0983c88c64d78a4b4/rest-api-spec/src/main/resources/rest-api-spec/api/search.json#L231
@delvedor Do you know if the elasticesearch team is going to introduce a spec for API responses as well?

api/ResponseTypes.d.ts

Veetaha · 2020-04-07T10:51:12Z

api/RequestTypes.d.ts

+  master_timeout?: string;
+}
+
+export {


What is the rationale for using this export style?

As you can see in the index.d.ts, I'm exporting all the request/response type definitions with

export * from './api/RequestTypes' export * from './api/ResponseTypes'

For some reason that I cannot fully understand, if I use the export style you are suggesting, I was able to also use interfaces that were not exported.
For example:

/* ResponseTypes.d.ts */ export interface Foo {} interface Bar {} /* index.d.ts */ export * from './api/ResponseTypes.d.ts' export { Client } /* code.ts */ // here I should be able to import Bar, but I can import { Client, Foo, Bar } from './'

Then I discovered that using the "single export" statement solves this quirk.

Okay, this is indeed a quick of .d.ts. files, all their symbols are exported by default. However, I think there should not be any private interfaces in .d.ts files, users should be able to access any interface that the can potentially use in their code, e.g. why not exporting interface Shards?
The user might want to write the following code:

const res = await client.update(...); const shards: Shards = res._shards; processShards(shards); // Why not giving the possibility to use the Shards type? function processShards(shards: Shards) {}

As you know, the API surface of Elasticsearch is massive, and given that we are doing this work manually, it's very easy to miss something.
There might be a ShardsResponse that is different from Shards (I don't think this is the case, but it might happen), and I would love to avoid doing too many breaking changes because I wasn't cautious about what I'm exposing.
Adding these response types will be a very long work that will be done during time, and a big part of it will be fix and improve the types already written.
Thank you for your suggestions!

Veetaha · 2020-04-07T12:37:41Z

api/ResponseTypes.d.ts

+interface UpdateResponse {
+  _shards: Shards
+  _index: string
+  _type: string
+  _id: string
+  _version: number
+  result: string
+}


Elasticsearch 7 supports returning the document source in update requests, e.g. the following request

POST /<index>/_update/<id> { "_source": "*", "doc": { "field": "val" } }

gives the following response

{ "_index" : "<index>", "_type" : "_doc", "_id" : "<id>", "_version" : 5, "result" : "updated", "_shards" : { "total" : 2, "successful" : 1, "failed" : 0 }, "_seq_no" : 53, "_primary_term" : 1, "get" : { "_seq_no" : 53, "_primary_term" : 1, "found" : true, "_source" : { "field": "val" } } }

So the user should be able to specify the type of that _source (furthermore this get object should be an optional property in UpdateResponse<T>.

@delvedor also note that the shards object I got from Elasticsearch doesn't contain skipped field, this is either a bug or an intended behavior, so if the latter we should mark it as optional: interface Shards { skipped?: number }

Also note that there are two more fields:

{ "_seq_no" : 53, "_primary_term" : 1 }

I think they also should be included in interface UpdateResponse

delvedor · 2020-04-07T13:35:55Z

@delvedor won't it be error-prone if the user can fully override the response type here?

@Veetaha it's something that could happen, yes. The main issue right now is that we don't have all the response types, and while we are working on it, this is not something that will change soon.
I fear that if some APIs offer you a way to define the full body response, others allow you to specify a part of it, and others do not provide anything at all, what you will get at the end is a massive source of confusion. Using the same approach for every API until we have a stable enough type definition provides a predictable API, that is easy to think about and do not require you to go and read the documentation/type definitions every time you are writing a line of code.

mshustov · 2020-04-16T12:58:23Z

scripts/utils/generateRequestTypes.js

@@ -36,6 +37,11 @@ export interface Generic {
 `

  api.forEach(generateRequestType)
+  types += '\nexport {\n'
+  api.forEach(generateExport)


the line above and the line below belong to generateExport operation.

IMO it's not obvious that generateRequestType & generateExport mutate types variable. I'd refactor the function to follow Transform pattern

mshustov · 2020-04-16T13:02:48Z

scripts/utils/generateRequestTypes.js

+  function generateExport (spec) {
+    const api = Object.keys(spec)[0]
+    const name = api
+      .replace(/\.([a-z])/g, k => k[1].toUpperCase())


We repeat the same operation across several generate utils. I'm wondering if we can normalize data once in a centralized manner.

mshustov · 2020-04-16T13:37:50Z

api/ResponseTypes.d.ts

+  hits: {
+    total: {
+      value: number
+      relation: string


I'm surprised that we are able to generate request types from the API spec, but no responses. https://github.com/elastic/elasticsearch/blob/d7bbc9df1d3edf47f8d1c2f0983c88c64d78a4b4/rest-api-spec/src/main/resources/rest-api-spec/api/search.json#L231
@delvedor Do you know if the elasticesearch team is going to introduce a spec for API responses as well?

mshustov · 2020-04-16T13:41:11Z

scripts/utils/generateMain.js

@@ -181,36 +192,37 @@ function buildMethodDefinition (api, name, hasBody) {
  const Name = toPascalCase(name)
  const bodyType = ndjsonApiKey.includes(Name) ? 'RequestNDBody' : 'RequestBody'
  const defaultBodyType = ndjsonApiKey.includes(Name) ? 'Record<string, any>[]' : 'Record<string, any>'
+  const responseType = supportedResponses.includes(Name) ? `Res.${Name}Response` : 'Record<string, any>'


optional: why not export responses used Response namespace?

mshustov · 2020-04-16T13:45:39Z

api/ResponseTypes.d.ts

+  details: Explanation[]
+}
+
+interface MSearchResponse<TSource = unknown, TAggregations = unknown> {


don't see it's used in index.d.ts file

mshustov · 2020-04-16T13:53:22Z

api/RequestTypes.d.ts

@@ -1311,7 +1311,7 @@ export interface Termvectors<T = RequestBody> extends Generic {
  body?: T;
 }

-export interface Update<T = RequestBody> extends Generic {
+interface UpdateRequest<T = RequestBody> extends GenericRequest {


interestingly, we use RequestBody as a default value here, but Record<string, any> in index.d.ts.

villasv · 2020-04-30T21:36:06Z

Just wanted to chime in once more and thank @delvedor for the hard work on this, once released I'm going to slap this into our codebase like it's Flex Tape.

mshustov · 2020-07-21T13:03:02Z

api/ResponseTypes.d.ts

+// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information
+
+interface SearchResponse<TSource = unknown, TAggregations = unknown> {


It's not meant to be used in api/kibana.d.ts?

delvedor · 2021-03-16T08:43:48Z

Closed in favor of #1358.

delvedor added 2 commits September 18, 2019 17:45

Added Response types definitions

ed96a9d

Updated docs

fd70174

delvedor added documentation labels Sep 18, 2019

wylieconlon reviewed Sep 18, 2019

View reviewed changes

api/ResponseTypes.ts Outdated Show resolved Hide resolved

delvedor added the discussion label Sep 26, 2019

Merge branch 'master' into add-response-types

0892ecb

wylieconlon mentioned this pull request Oct 17, 2019

[APM] Refactor & scope aggregation types elastic/kibana#48394

Merged

joshdover mentioned this pull request Mar 16, 2020

Define and export type SearchResponse #933

Closed

Merge branch 'master' into add-response-types

9380adc

delvedor mentioned this pull request Mar 27, 2020

Switch Request and Response generics #1130

Closed

Veetaha mentioned this pull request Mar 27, 2020

[ENHANCEMENT] Request and Response types elastic/elasticsearch-rs#75

Open

delvedor added 7 commits April 6, 2020 14:28

Merge branch 'master' into add-response-types

511db34

API generation

14d6401

Merge branch 'master' into add-response-types

9f04d26

Updated code generation

36cb500

Removed old files

08b2827

Use new types namespaces and improve type exports+

e34aa55

Updated test

d53b68c

delvedor force-pushed the master branch from 14d6401 to 8522f30 Compare April 7, 2020 09:26

delvedor added 3 commits April 7, 2020 11:29

Merge branch 'master' into add-response-types

b083db6

API generation

e82cfe3

Added Request and Response types

728254b

delvedor commented Apr 7, 2020

View reviewed changes

Veetaha reviewed Apr 7, 2020

View reviewed changes

api/ResponseTypes.d.ts Show resolved Hide resolved

Veetaha reviewed Apr 7, 2020

View reviewed changes

mshustov reviewed Apr 16, 2020

View reviewed changes

delvedor added 5 commits July 20, 2020 17:33

Merge branch 'master' into add-response-types

911e883

Merge branch 'master' into add-response-types

5c6131d

API generation

1673020

Fixed kibana export

41529a4

API generation

acc9346

delvedor changed the title ~~Added Response types definitions~~ Added Request/Response types definitions Jul 20, 2020

delvedor added backport 7.x and removed v7.x labels Jul 20, 2020

mshustov mentioned this pull request Jul 21, 2020

add migration section for the new ES client elastic/kibana#71604

Merged

1 task

mshustov reviewed Jul 21, 2020

View reviewed changes

delvedor closed this Mar 16, 2021

delvedor deleted the add-response-types branch August 11, 2021 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Request/Response types definitions #970

Added Request/Response types definitions #970

delvedor commented Sep 18, 2019 •

edited

Loading

wylieconlon Sep 18, 2019

delvedor Sep 18, 2019

wylieconlon Sep 18, 2019

Veetaha Apr 7, 2020

lesyk-lesyk commented Feb 25, 2020

delvedor left a comment

Veetaha Apr 7, 2020 •

edited

Loading

mshustov Apr 16, 2020

Veetaha commented Apr 7, 2020

dgieselaar commented Apr 7, 2020

Veetaha Apr 7, 2020 •

edited

Loading

mshustov Apr 16, 2020

Veetaha Apr 7, 2020

delvedor Apr 7, 2020 •

edited

Loading

Veetaha Apr 7, 2020

delvedor Apr 7, 2020

Veetaha Apr 7, 2020 •

edited

Loading

delvedor commented Apr 7, 2020

mshustov Apr 16, 2020

mshustov Apr 16, 2020

mshustov Apr 16, 2020

mshustov Apr 16, 2020

mshustov Apr 16, 2020

mshustov Apr 16, 2020

villasv commented Apr 30, 2020 •

edited

Loading

mshustov Jul 21, 2020

delvedor commented Mar 16, 2021

Added Request/Response types definitions #970

Added Request/Response types definitions #970

Conversation

delvedor commented Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesyk-lesyk commented Feb 25, 2020

delvedor left a comment

Choose a reason for hiding this comment

Veetaha Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha commented Apr 7, 2020

dgieselaar commented Apr 7, 2020

Veetaha Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

delvedor commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villasv commented Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

delvedor commented Mar 16, 2021

delvedor commented Sep 18, 2019 •

edited

Loading

Veetaha Apr 7, 2020 •

edited

Loading

Veetaha Apr 7, 2020 •

edited

Loading

delvedor Apr 7, 2020 •

edited

Loading

Veetaha Apr 7, 2020 •

edited

Loading

villasv commented Apr 30, 2020 •

edited

Loading