Add Synonyms API spec #2210
Add Synonyms API spec #2210
Conversation
carlosdelest
requested review from
afoucret,
demjened,
kderusso and
saikatsarkar056
| @@ -5,8 +5,7 @@ | |||
| "description": "Creates or updates a synonyms set" | |||
| }, | |||
| "stability": "experimental", | |||
| "visibility": "feature_flag", | |||
| "feature_flag": "es.synonyms_feature_flag_enabled", | |||
| "visibility": "public", | |||
There was a problem hiding this comment.
Feature flags were removed per elastic/elasticsearch#97962
|
|
||
| export type SynonymString = string | ||
|
|
||
| export class SynonymRuleOptionalId { |
There was a problem hiding this comment.
There are operations (PUT) that don't need an ID to be included, so it will be generated by the API. I wanted to use a strict type for that, but it might be overkill...
There was a problem hiding this comment.
My two cents: I don't think it hurts, but I'm not sure what it adds. Happy to leave it in but I don't have strong feelings either way.
There was a problem hiding this comment.
I think its fine, I understand the intent clearly enough and it will allow some clients to have validation for mandatory fields which is great.
As a nit, maybe I'd add a comment only for the spec with // to highlight why this is like this. Comments in form /**/ are included in the generated schema and the clients, // are not.
There was a problem hiding this comment.
Thanks @Anaethelion - I added comments to clarify why they're used 👍
|
|
||
| import { SynonymRule, SynonymRuleOptionalId } from './SynonymRule' | ||
|
|
||
| export class SynonymsSetUpdate { |
There was a problem hiding this comment.
Same here, happy to hear thoughts on using specific types for including ids or not
| }, | ||
| "stability": "experimental", | ||
| "visibility": "public", | ||
| "feature_flag": "es.synonyms_api_feature_flag_enabled", |
There was a problem hiding this comment.
I think we want to remove the FF from this file
There was a problem hiding this comment.
Yes, thanks for the catch! 👍
|
|
||
| import { SynonymsUpdateResult } from '../../synonyms/_types/SynonymsUpdateResult' | ||
|
|
||
| export class Response { |
There was a problem hiding this comment.
There's some duplication between this response signature and others such as SynonymRuleDeleteResponse. Is this intentional or is there any way to consolidate?
There was a problem hiding this comment.
I tried to consolidate as much as possible by creating the SynonymsUpdateResult type, which is essentially the body for these responses.
I'd say we need the separate Request / Response classes for the codegen to work - hopefully the clients team can recommend a better pattern here.
There was a problem hiding this comment.
I'd say we need the separate Request / Response classes for the codegen to work - hopefully the clients team can recommend a better pattern here.
Yes we need those for codegen, each request/response couple will be generated into specific expanded types with their namespace.
In this case it does seem like there's duplication but I don't have a better pattern, you went as far as we can at the moment.
Kudos for a consistent API! 👍
| */ | ||
| from?: integer | ||
| /** | ||
| * specifies a max number of results to get |
There was a problem hiding this comment.
Should we include the default in the comment here too for consistency?
There was a problem hiding this comment.
Sure! Thanks for the suggestion
|
|
||
| export type SynonymString = string | ||
|
|
||
| export class SynonymRuleOptionalId { |
There was a problem hiding this comment.
My two cents: I don't think it hurts, but I'm not sure what it adds. Happy to leave it in but I don't have strong feelings either way.
|
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
Adds Synonyms API spec and types