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

Code changes to add muli api support #14706

Merged

Conversation

sarangan12
Copy link
Member

Two new requests have come for search-documents SDK.

  1. There is a bug in the current swagger. This should be fixed before next release. In the meantime, we need to add a swagger transform and handle the bug. The issue is detailed here

  2. This issue is regarding passing a different version to the search clients constructors. It has already been communicated to the customers that there might be additional fields coming up due to version changes and to the service team that there should not be any compatibility changes. This design will be dicussed in detail before next release. But for this release, we wanted to try this design of multi api support.

@xirzec Please review this PR

@sarangan12 sarangan12 requested a review from xirzec April 5, 2021 02:45
@ghost ghost added the Search label Apr 5, 2021
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm not understanding the context behind these changes, but I have some questions.

@@ -1596,7 +1600,7 @@ export interface SearchIndex {

// @public
export class SearchIndexClient {
constructor(endpoint: string, credential: KeyCredential, options?: SearchIndexClientOptions);
constructor(endpoint: string, credential: KeyCredential, apiVersion?: string, options?: SearchIndexClientOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having multiple optional parameters. This forces anyone who wants to pass options to pass undefined to apiVersion if they don't want to set it, which feels bad.

Why can't apiVersion live on SearchIndexClientOptions ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I will change it for all the 3 clients in the next commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xirzec Updated in the latest commit

/** Identifies the concrete type of the normalizer. */
odatatype: string;
/** Polymorphic discriminator, which specifies the different types this object can be */
odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this. Why would a generic LexicalNormalizer have the same odatatype as a CustomNormalizer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xirzec Yeah. It was little confusing to me too. Let me explain with an example. For the following swagger:

"Similarity": {
  "discriminator": "@odata.type",
  .........
}

"ClassicSimilarity": {
  "x-ms-discriminator-value": "#Microsoft.Azure.Search.ClassicSimilarity",
  .........
}

"BM25Similarity": {
  "x-ms-discriminator-value": "#Microsoft.Azure.Search.BM25Similarity",
  .........
}

For the above swagger, the SDK will be generated as:

export interface Similarity {
  odatatype: "#Microsoft.Azure.Search.ClassicSimilarity" | "#Microsoft.Azure.Search.BM25Similarity";
  .........
}

export type ClassicSimilarity = Similarity & {
  odatatype: "#Microsoft.Azure.Search.ClassicSimilarity";
  .........
};

export type BM25Similarity = Similarity & {
  odatatype: "#Microsoft.Azure.Search.BM25Similarity";
  .........
}

In the above code, look at the Similarity interface. It is the base class. But, the odatatype of Similarity is actually the union of odatatype of the two sub classes (ClassicSimilarity & BM25Similarity).

Now, let us look into the LexicalNormalizer. The swagger is:

"LexicalNormalizer": {
  "discriminator": "@odata.type",
   ......
}

"CustomNormalizer": {
  "x-ms-discriminator-value": "#Microsoft.Azure.Search.CustomNormalizer",
   ..........
}

For the above swagger, the SDK looks like:

export interface LexicalNormalizer {
  odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
  .........
}

export type CustomNormalizer = LexicalNormalizer & {
  odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
  .........
}

In the above code the base class LexicalNormalizer, the odatatype is set to that of the CustomNormalizer which may look a little odd.

But, consider there is another normalizer in the swagger such as this:

"SampleNormalizer": {
  "x-ms-discriminator-value": "#Microsoft.Azure.Search.SampleNormalizer",
   ..........
}

then the resulting SDK will look like:

export interface LexicalNormalizer {
  odatatype: "#Microsoft.Azure.Search.CustomNormalizer" | "#Microsoft.Azure.Search.SampleNormalizer";
  .........
}

export type CustomNormalizer = LexicalNormalizer & {
  odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
  .........
}

export type SampleNormalizer = LexicalNormalizer & {
  odatatype: "#Microsoft.Azure.Search.SampleNormalizer";
  .........
}

The confusion is that we do not have more than one sub type of normalizer, it is looking a little odd. Am I making sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh! This does make sense and helps a lot. Thanks! 👍

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update looks good!

@sarangan12 sarangan12 merged commit d31aa59 into Azure:master Apr 6, 2021
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
* Code changes to add muli api support

* Format

* Add Version Check

* Format

* Move ApiVersion to options bag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants