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

Search documents migration #12829

Merged
merged 6 commits into from
Jan 23, 2021
Merged

Conversation

sarangan12
Copy link
Member

@sarangan12 sarangan12 commented Dec 9, 2020

This PR is to move the search-documents SDK's generated code to the new v6 code generator and perform the corresponding custom code changes.

@xirzec @joheredi Please review and approve the PR.

@ramya-rao-a FYI....

@ghost ghost added the Search label Dec 9, 2020
@sarangan12 sarangan12 changed the title Search documents migration[NOT READY FOR REVIEW/MERGE] Search documents migration Dec 14, 2020
@sarangan12 sarangan12 marked this pull request as ready for review December 14, 2020 08:17
@sarangan12 sarangan12 requested a review from joheredi December 14, 2020 08:19
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.

This is a bit tricky to review because of a lot of churn in result shapes, especially the way we've pulled out these seemingly not useful 'base' types.

My only real concern, however, is there are a few places were we were careful to deserialize special @odata properties separately from the user's document schema that appear to be broken. When we fix this, we should add tests that create a schema with properties like score, highlights, and text to check for these collisions.

@@ -82,68 +81,76 @@ export interface AzureActiveDirectoryApplicationCredentials {
export { AzureKeyCredential }

// @public
export type BlobIndexerDataToExtract = 'storageMetadata' | 'allMetadata' | 'contentAndMetadata';
export type BlobIndexerDataToExtract = string;
Copy link
Member

Choose a reason for hiding this comment

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

did these change to be extensible enums?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I have also exposed Known values.

sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
sdk/search/search-documents/src/indexModels.ts Outdated Show resolved Hide resolved
sdk/search/search-documents/src/index.ts Outdated Show resolved Hide resolved
sdk/search/search-documents/swagger/Data.md Show resolved Hide resolved
sdk/search/search-documents/swagger/Data.md Show resolved Hide resolved
@sarangan12 sarangan12 force-pushed the SearchDocumentsMigration branch from 086e48b to 487b50f Compare January 22, 2021 05:46
@sarangan12 sarangan12 requested a review from chradek as a code owner January 22, 2021 05:46
/** The data container for the datasource. */
container: SearchIndexerDataContainer;
/** The data change detection policy for the datasource. */
dataChangeDetectionPolicy?: DataChangeDetectionPolicyUnion | null;
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 One of the questions you had was - What is the reason for this | null value at the end of dataChangeDetectionPolicy. To explain this, we need to go the swagger:

....................
....................
"SearchIndexerDataSource": {
  "properties": {
    "name": {
      "externalDocs": {
        "url": "https://docs.microsoft.com/rest/api/searchservice/Naming-rules"
      },
      "type": "string",
      "description": "The name of the datasource."
    },
    "description": {
      "type": "string",
      "description": "The description of the datasource."
    },
    "type": {
      "$ref": "#/definitions/SearchIndexerDataSourceType",
      "description": "The type of the datasource."
    },
    "credentials": {
      "$ref": "#/definitions/DataSourceCredentials",
      "description": "Credentials for the datasource."
    },
    "container": {
      "$ref": "#/definitions/SearchIndexerDataContainer",
      "description": "The data container for the datasource."
    },
    "dataChangeDetectionPolicy": {
      "$ref": "#/definitions/DataChangeDetectionPolicy",
      "x-nullable": true,
      "description": "The data change detection policy for the datasource."
    },
    "dataDeletionDetectionPolicy": {
      "$ref": "#/definitions/DataDeletionDetectionPolicy",
      "x-nullable": true,
      "description": "The data deletion detection policy for the datasource."
    },
    "@odata.etag": {
      "x-ms-client-name": "ETag",
      "type": "string",
      "description": "The ETag of the data source."
    },
    "encryptionKey": {
      "$ref": "#/definitions/SearchResourceEncryptionKey",
      "description": "................",
      "externalDocs": {
        "url": "https://aka.ms/azure-search-encryption-with-cmk"
      },
      "x-nullable": true
    }
  },
  "required": [
    "name",
    "type",
    "credentials",
    "container"
  ],
  "description": "Represents a datasource definition, which can be used to configure an indexer."
}

As you can see from the above swagger, the dataChangeDetectionPolicy has the property x-nullable: true. So, we need to add the | null value at the end.

What would happen if we do not add this?
If the SDK user wants to actually send in the null value (as the service allows it - assuming the value null has some special property on the server side.), it might not be possible to send it. Also, to reflect the swagger code closely, we need to add this | null at the end.

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 fine supporting something the server does -- I'm just curious how the server handles it. If you set null on an update, is that the proper way to remove the thing? And how is that functionally different from us removing that part of the API shape?

What's confusing me is null is how you'd blank out part of the payload in C#, but in JS we have undefined to do this same job. It's rare that undefined and null have different meanings. If they mean the same thing, do we need both?

Copy link
Member

Choose a reason for hiding this comment

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

To me, nullable should mean optional. We really don't want users to have to think about the differences between {x: undefined}, {}, and {x: null} do we? I hope that no service treats these differently.

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.

I'm ok with the changes, though I'm a little confused why we need to have both null and undefined still. Like how would the actual REST call be different if you picked one or the other? Wouldn't they both be serialized as the empty string? Or is null going to actually send the string null or something?

@sarangan12 sarangan12 merged commit cd4ad88 into Azure:master Jan 23, 2021
@sarangan12
Copy link
Member Author

I am going to discuss next week with @joheredi about this issue. At least, we could remove the null if the property is optional.

Now, I am merging this PR.

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Feb 9, 2021
[Hub Generated] Review request for Microsoft.Cache to add version stable/2021-03-01 (Azure#12829)

* Adds base for updating Microsoft.Cache from version preview/2020-10-01-preview to version 2021-03-01

* Updates readme

* Updates API version in new specs and examples

* Add support for persistence and fix other typos

* Fix SDK Warning: R2063 OperationIdNounConflictingModelNames

* Fix spellcheck error

* Fix acronym capitalization in descriptions

* Replace local parameters with common-types parameters

* Use naming convention consistent with Azure documentation

* Update word choice, remove deprecated x-ms-code-generation-setting, and fix operationId

* Add x-ms-secret extension and make TLS version a string enum

* Suppress errors about secrets being sent in responses
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Feb 22, 2021
[Hub Generated] Review request for Microsoft.Cache to add version stable/2021-03-01 (Azure#12829)

* Adds base for updating Microsoft.Cache from version preview/2020-10-01-preview to version 2021-03-01

* Updates readme

* Updates API version in new specs and examples

* Add support for persistence and fix other typos

* Fix SDK Warning: R2063 OperationIdNounConflictingModelNames

* Fix spellcheck error

* Fix acronym capitalization in descriptions

* Replace local parameters with common-types parameters

* Use naming convention consistent with Azure documentation

* Update word choice, remove deprecated x-ms-code-generation-setting, and fix operationId

* Add x-ms-secret extension and make TLS version a string enum

* Suppress errors about secrets being sent in responses
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.

3 participants