Add CustomEntityLookupSkill by shuyangmsft · Pull Request #10832 · Azure/azure-rest-api-specs

shuyangmsft · 2020-09-17T20:09:06Z

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

I commit to follow the Breaking Change Policy of “no breaking changes
I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

If any further question about AME onboarding or validation tools, please view the FAQ.

ARM API Review Checklist

Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
- Adding new API(s)
- Adding a new API version
- Adding a new service
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist

If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

Removing API(s) in stable version
Removing properties in stable version
Removing API version(s) in stable version
Updating API in stable version with Breaking Change Validation errors
Updating API(s) in preview over 1 year

Please follow the link to find more details on PR review process.

openapi-pipeline-app · 2020-09-17T20:09:11Z

Swagger pipeline restarted successfully, please wait for status update in this comment.

azure-pipelines · 2020-09-17T20:09:22Z

Azure Pipelines successfully started running 1 pipeline(s).

openapi-sdkautomation · 2020-09-17T20:10:43Z

azure-sdk-for-java - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:44Z

Azure CLI Extension Generation - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:45Z

azure-sdk-for-go - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:46Z

azure-sdk-for-net - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:48Z

azure-sdk-for-python - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:48Z

azure-sdk-for-js - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:51Z

azure-resource-manager-schemas - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

openapi-sdkautomation · 2020-09-17T20:10:57Z

Trenton Generation - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

lmazuel

From a Swagger perspective LGTM

heaths

I know a precedent is set with so many nullable properties, but wouldn't it save bandwidth to just not serialize them across the wire?

heaths · 2020-09-21T22:08:24Z

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

+        },
+        "caseSensitive": {
+          "type": "boolean",
+          "x-nullable": true,


There are a lot of x-nullable fields. Can you just not send them when null? Is there an advantage to sending, for example, "caseSensitive": null and similar for everything that isn't explicitly set?

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

heaths · 2020-09-21T22:10:42Z

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

+        "caseSensitive": {
+          "type": "boolean",
+          "x-nullable": true,
+          "description": "Determine if the alias is case sentitive."


You should document the default either in the description or in swagger. Note that the generators won't (or at least shouldn't) generate code for the default since that creates a tight coupling across versions. Defaults must be applied at the service.

heaths · 2020-09-21T22:12:17Z

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

+        }
+      ],
+      "properties": {
+        "defaultLanguageCode": {


Based on other skills, this should be "x-nullable": true since you send the others will a null value, or will that not be the case here? Better not to waste bandwidth with null values if there's no benefit.

heaths · 2020-09-21T22:13:56Z

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

+        },
+        "entitiesDefinitionUri": {
+          "type": "string",
+          "description": "Path to a JSON or CSV file containing all the target text to match against."


Any auth requirements that should be documented? E.g. "Must be publicly accessible via a GET query."

heaths · 2020-09-21T22:15:28Z

specification/search/data-plane/Azure.Search/preview/2019-05-06-preview/searchservice.json

+        "url": "https://docs.microsoft.com/en-us/azure/search/cognitive-search-skill-custom-entity-lookup"
+      },
+      "description": "A skill looks for text from a custom, user-defined list of words and phrases."
+    },


No required values? I would assume that at least the inline or URI parameters need to be specified. You might add that to the description. Swagger can actually require one of the two, but I'm not sure our generators would handle that.

brjohnstmsft

This appears to be targeting the wrong API version. We shouldn't be making changes to the 2019 specs anymore.

azure-pipelines · 2020-09-28T18:33:05Z

Azure Pipelines successfully started running 1 pipeline(s).

openapi-sdkautomation · 2020-09-28T18:37:05Z

azure-sdk-for-python-track2 - Release

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

azure-pipelines · 2020-09-28T18:38:58Z

Azure Pipelines successfully started running 1 pipeline(s).

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json

johanste · 2020-09-29T20:15:38Z

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json

+        },
+        "globalDefaultCaseSensitive": {
+          "type": "boolean",
+          "x-nullable": true,


Why is this property both optional and nullable? What is the difference between the key/value not being present and the key/value being present with a null value?

For some reason all my replies seem gone...

The key/value not being present and key/value being present with a null value don't make a difference from the backend service perspective. Do you recommend me to remove "x-nullable" here and for all other similar fields?

I would recommend for the service to not send the null value on the wire if that is the case. If the service isn't actually sending null on the wire, then the x-nullable annotation should be removed.

If the service is sending a null value on the wire, it is unfortunate, but it has to be marked as x-nullable in the swagger. At the end of the day, the swagger has to describe what how the service is behaving regardless of if it is following best practices or not...

I recently had this question with @bleroy and @brjohnstmsft. Not only is it a waste of bandwidth, but it has been causing problems since not nearly everything that should be marked x-nullable is, and our generators do care that it's authored correctly. Alas, it sounds like the OData library they use does this. Still, is there some way to get that fixed, or opt into a more wire-friendly serializer? I know Newtonsoft.Json won't send explicit nulls by default, so it seems OData is overriding that. Why, I can't imagine.

I know Newtonsoft.Json won't send explicit nulls by default, so it seems OData is overriding that.

OData .NET has its own serializer; It's not based on JSON.NET.

Trying to fix this would be a potentially costly change for us (either swapping out OData's serializer, or forking the library and fixing it), and would be hard to justify given our other priorities. You should proceed on the assumption that the service behaviour isn't going to change.

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json

azure-pipelines · 2020-09-29T22:13:08Z

Azure Pipelines successfully started running 1 pipeline(s).

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json

azure-pipelines · 2020-09-29T23:08:35Z

Azure Pipelines successfully started running 1 pipeline(s).

Remove unnecessary x-nullable

azure-pipelines · 2020-10-02T22:36:25Z

Azure Pipelines successfully started running 1 pipeline(s).

…0-Preview/searchservice.json Co-authored-by: Heath Stewart <heaths@outlook.com>

azure-pipelines · 2020-10-08T23:36:37Z

No pipelines are associated with this pull request.

lmazuel · 2020-10-09T17:25:58Z

ADO seems stuck, second to last commit was green, and last commit was just string replace, merging

This reverts commit c53d3cb.

shuyangmsft added 3 commits September 17, 2020 10:58

Add CustomEntityLookupSkill to searchservice.json

981fb28

Fix typos

b3cc204

remove non-required fields from "required"

01a4fb8

shuyangmsft requested review from arv100kri, bleroy and brjohnstmsft as code owners September 17, 2020 20:09

openapi-assignment-bot bot assigned lmazuel Sep 17, 2020

lmazuel approved these changes Sep 17, 2020

View reviewed changes

MarkHeff requested review from AlexGhiondea and heaths September 21, 2020 21:39

heaths approved these changes Sep 21, 2020

View reviewed changes

brjohnstmsft suggested changes Sep 28, 2020

View reviewed changes

Move API change to 2020-06-30

9fe6d35

remove whiteline

59133f7

shuyangmsft requested review from brjohnstmsft and lmazuel September 28, 2020 18:39

johanste reviewed Sep 29, 2020

View reviewed changes

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json Outdated Show resolved Hide resolved

johanste reviewed Sep 29, 2020

View reviewed changes

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json Outdated Show resolved Hide resolved

johanste reviewed Sep 29, 2020

View reviewed changes

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json Show resolved Hide resolved

Update searchservice.json

f72c587

heaths approved these changes Sep 29, 2020

View reviewed changes

specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json Outdated Show resolved Hide resolved

fix typo copied from public API docs.

09d1f4c

Update searchservice.json

078593a

Remove unnecessary x-nullable

brjohnstmsft approved these changes Oct 8, 2020

View reviewed changes

Update specification/search/data-plane/Azure.Search/preview/2020-06-3…

e27dfb6

…0-Preview/searchservice.json Co-authored-by: Heath Stewart <heaths@outlook.com>

heaths approved these changes Oct 8, 2020

View reviewed changes

lmazuel merged commit c53d3cb into Azure:master Oct 9, 2020

shuyangmsft mentioned this pull request Oct 9, 2020

Revert "Add CustomEntityLookupSkill" #11109

Merged

jhendrixMSFT pushed a commit that referenced this pull request Oct 9, 2020

Revert "Add CustomEntityLookupSkill (#10832)" (#11109)

e94d561

This reverts commit c53d3cb.

Conversation

shuyangmsft commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution checklist:

ARM API Review Checklist

Breaking Change Review Checklist

Uh oh!

openapi-pipeline-app bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines bot commented Sep 17, 2020

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-java - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Azure CLI Extension Generation - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-go - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-net - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-python - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-js - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-resource-manager-schemas - Release

Uh oh!

openapi-sdkautomation bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trenton Generation - Release

Uh oh!

lmazuel left a comment

Choose a reason for hiding this comment

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brjohnstmsft left a comment

Choose a reason for hiding this comment

Uh oh!

azure-pipelines bot commented Sep 28, 2020

Uh oh!

openapi-sdkautomation bot commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

azure-sdk-for-python-track2 - Release

Uh oh!

azure-pipelines bot commented Sep 28, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shuyangmsft commented Sep 17, 2020 •

edited

Loading

openapi-pipeline-app bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 17, 2020 •

edited

Loading

openapi-sdkautomation bot commented Sep 28, 2020 •

edited

Loading