Skip to content

[Search] Add client names for ResetSkills and ResetDocs #16222

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.

Sign up for GitHub

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

Merged
lmazuel merged 6 commits into from
Sep 30, 2021
Merged

[Search] Add client names for ResetSkills and ResetDocs #16222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56692da
Add client names for ResetSkills and ResetDocs
Mohit-Chakraborty Sep 29, 2021
439d1ae
Make 'skillNames' a list of items and not a class with a list property
Mohit-Chakraborty Sep 29, 2021
6731185
Revert "Make 'skillNames' a list of items and not a class with a list…
Mohit-Chakraborty Sep 29, 2021
f2896cd
Remove outer 'description' property
Mohit-Chakraborty Sep 29, 2021
f996c16
Revert "Remove outer 'description' property"
Mohit-Chakraborty Sep 29, 2021
8e1a301
Try to fix the example as per the spec
Mohit-Chakraborty Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ...data-plane/Azure.Search/preview/2021-04-30-Preview/examples/SearchServiceResetSkills.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
"endpoint": "https://myservice.search.windows.net",
"skillsetName": "mySkillset",
"api-version": "2021-04-30-preview",
"skillNames": [
"skillName1",
"skillName2"
]
"skillNames": {
"skillNames": [
"skillName1",
"skillName2"
]
}
},
"responses": {
"204": {}
Expand Down
3 changes: 3 additions & 0 deletions specification/search/data-plane/Azure.Search/preview/2021-04-30-Preview/searchservice.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@
"required": false,
"schema": {
"type": "object",
"x-ms-client-name": "DocumentKeysOrIds",
"properties": {
"documentKeys": {
"type": "array",
Expand Down Expand Up @@ -1061,6 +1062,8 @@
"in": "body",
"required": true,
"schema": {
Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Still not happy with this part.
This generates a class with an IList as its only property -

    public partial class SkillNames
    {
        /// <summary> Initializes a new instance of SkillNames. </summary>
        public SkillNames()
        {
            SkillNamesValue = new ChangeTrackingList<string>();
        }

        /// <summary> the names of skills to be reset. </summary>
        public IList<string> SkillNamesValue { get; }
    }

Looking for ways to have this parameter as a list itself. Can we do that without breaking the service protocol?

Copy link
Contributor

@bleroy bleroy Sep 29, 2021

Choose a reason for hiding this comment

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

I agree it doesn't seem to warrant its own type. @shuyangmsft ?

Copy link
Member

@ShivangiReja ShivangiReja Sep 29, 2021

Choose a reason for hiding this comment

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

@Mohit-Chakraborty If you want to add skillNames as a parameter directly. You can add skillNames parameter like-

{
      "name": "skillNames",
       "in": "body",
       "required": true,
       "schema": {
          "type": "array",
           "items": {
           "type": "string"
             }
        },
       "description": "The names of skills to reset."
}

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021

Choose a reason for hiding this comment

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

Thanks @ShivangiReja. Do you know if this will be a breaking change for the service?

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021

Choose a reason for hiding this comment

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

❌ 1033 - RemovedProperty The new version is missing a property found in the old version. Was 'skillNames' renamed or removed?Old: Azure.Search/preview/2021-04-30-Preview/searchservice.json#L1064:15

Validation tells me this is a breaking change.

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, we should probably make this change in the client to make things look better.

Copy link
Member

@ShivangiReja ShivangiReja Sep 29, 2021

Choose a reason for hiding this comment

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

It looks like a breaking change, since previously user was passing the object type in the method and now with this change user will have to pass the list.

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021

Choose a reason for hiding this comment

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

I reverted the change.

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 29, 2021

Choose a reason for hiding this comment

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

I still have an error -

❌ INVALID_TYPE Expected type object but found type arrayUrl: Azure.Search/preview/2021-04-30-Preview/searchservice.json#L1064

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Sep 30, 2021

Choose a reason for hiding this comment

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

Finally managed to fix all validation errors. The above error was due to a discrepancy between the spec and the example for the ResetSkills operation. I updated the example to follow the design of the spec.

"type": "object",
"x-ms-client-name": "SkillNames",
"properties": {
"skillNames": {
"type": "array",
Expand Down