Search SDK: Adding arm-search spec for 2015-08-19 API version by bitsforthought · Pull Request #637 · Azure/azure-rest-api-specs

bitsforthought · 2016-10-25T21:29:50Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

FYI @chaosrealm @seansaleh @HeidiSteen @EvanBoyle @jack4it

This change is

This spec covers all functionality of the Azure Search Management REST API version 2015-08-19, including key management and search service management, provisioning, and scaling.

This is part 1 of a final pass over the Swagger spec for arm-search. Types of issues that are addressed include: - Consistency of parameter names (serviceName -> searchServiceName). - Consistency of method names with established Azure conventions (List -> ListByResourceGroup, etc.). - Added the ability to pass in x-ms-client-request-id (test coverage still pending). - Correctly modeled error cases for all key management operations. - Enabled paging for List operations (although the Search RP doesn't support paging, we at least benefit from the improved client programming model). Part 2 will address the same sorts of issues in Search service management operations.

hovsepm · 2016-10-25T22:04:23Z

+        ],
+        "x-ms-pageable": {
+          "nextLinkName": null
+        },


Listing services inside resource group will never return more than one page of results?

As implemented in our RP currently, no it won't.

bitsforthought · 2016-10-28T16:23:10Z

FYI @tbombach

bitsforthought · 2016-10-28T17:12:56Z

We'll be replacing the externalDocs links to MSDN as soon as we have new content (or at least aka.ms links) to replace it.

The aka.ms links still point to MSDN, but they'll be updated to docs.microsoft.com after the migration.

tbombach

The JSON constraints aren't a "must-have" for merging (especially not a regex pattern that covers the rules for dashes).

The only question I have before merging is around use of enums - is it fine that the client will be unable to handle new values for that property? It would take a major version bump in the SDKs to support a new value (since enum additions are considered breaking changes in strongly typed languages). If you want to minimize breaking changes, then modeling as string will do that.

tbombach · 2016-10-28T22:08:50Z

+      ],
+      "properties": {
+        "name": {
+          "description": "The Search service name to validate. Search service names must only contain lowercase letters, digits or dashes, cannot use dash as the first two or last one characters, cannot contain consecutive dashes, and must be between 2 and 60 characters in length.",


It's nice that these constraints are described, but should they also be JSON constraints? The regex pattern might be complicated to do, but the min/max string constraints wouldn't be difficult.

What's the purpose of the regex? If it's for codegen, I'm not sure it's worth duplicating the validation logic as our service already does that. If it's for documentation, I think English prose works better than regexes anyway.

The benefit is that validation would fail before a call is made the service.

The constraints (including regex pattern) get turned into checks in the Validate() method: https://github.com/Azure/azure-sdk-for-net/blob/04d78e2101f7d040aaa97481757a2fb69fffe9c9/src/Search/Microsoft.Azure.Search/GeneratedSearchService/Models/StandardAnalyzer.cs#L65

That get called when making requests: https://github.com/Azure/azure-sdk-for-net/blob/04d78e2101f7d040aaa97481757a2fb69fffe9c9/src/Search/Microsoft.Azure.Search/GeneratedSearchService/DataSourcesOperations.cs#L102

That way a call would fail fast with a ValidationException, rather than making an actual call to the service and waiting for a response. The goal is to do client-side validation to the best of our ability, even though there's already server-side validation. It's also easier for a user writing code that uses the client - the response from the service is probably some variation of a 4xx Bad Request, while a ValidationException contains the regex pattern that failed and the target property that failed the pattern. If you rely on the response, then the customer code has to parse the message on the CloudError to find the problem.

This seems like solving the problem at the wrong layer. It introduces unnecessary coupling between the service and the client. Consider versioning -- What if we discover a bug in the regex and fix it in our RP, but forget to fix it in the client? Or maybe we fix it in both places, but some customers are still using an older client with the wrong regex. This kind of coupling seems like too high a price to pay just to shave off some latency in an error case.

Regarding usability, here is the experience of providing an invalid service name today:

[Fact] public void CreateServiceWithInvalidNameGivesUsefulMessage() { Run(() => { SearchManagementClient searchMgmt = GetSearchManagementClient(); SearchService service = DefineServiceWithSku(SkuName.Free); CloudException e = Assert.Throws<CloudException>(() => searchMgmt.Services.CreateOrUpdate(Data.ResourceGroupName, InvalidServiceName, service)); Assert.Equal("Service name '----badname' is invalid: Service name must only contain lowercase letters, digits or dashes, cannot start or end with or contain consecutive dashes and is limited to 60 characters.", e.Message); }); }

If need be, we can include the regex in the error message, but for most users this won't make much sense.

tbombach · 2016-10-28T22:14:09Z

+          ],
+          "x-ms-enum": {
+            "name": "SkuName",
+            "modelAsString": false


One note with modelAsString: false is if a new value is ever added, the client will be unable to handle it. When trying to deserialize the name property, it will be unable to map any values not listed here to a correct enum value (for languages that use real enums).

Good point. One thing I don't like about modelAsString though is that it breaks Intellisense. In our data plane SDK, I created an "extensible enum" pattern to handle this, but I haven't had time to try to fold it back into AutoRest.

How about this... Since we don't plan to add any SKUs in the near future, let's leave it like this for now, and I'll use the addition of a new SKU as a forcing function add extensible enums to AutoRest. Sound good?

Yes, I think that's our currently only supported way of having the desired Intellisense experience for a set of known values. Let's leave it that way for now with the knowledge that a major version bump for SDKs would be required once SKU is expanded (or the extensible enums that you mentioned).

bitsforthought · 2016-10-28T22:31:51Z

I decided to add some JSON examples to the spec... Please hold off merging for now. Thanks!

bitsforthought · 2016-10-28T22:47:42Z

@HeidiSteen I ended up adding the examples after all, since it's possible to have different examples for the request & response.

@tbombach I'm ready for this to be merged now, assuming you agree with my comments above.

tbombach · 2016-10-28T23:31:06Z

Thanks for the update @brjohnstmsft. The enum plan makes sense. I left another comment re: the regex constraint.

bitsforthought · 2016-10-28T23:54:10Z

@tbombach Replied to your comment above.

AutorestCI · 2016-10-31T16:51:36Z

No modification for Ruby

AutorestCI · 2016-10-31T16:51:46Z

No modification for NodeJS

AutorestCI · 2016-10-31T16:51:48Z

No modification for Python

seansaleh · 2016-10-31T18:18:05Z

-      "description": "Input of check name availability API."
+      "description": "Input of check name availability API.",
+      "example": {
+        "name": "azs-test",  


Do we want to use the azs- convention here?

Good point... probably not

Review opened to change it #646

bitsforthought added 4 commits October 25, 2016 14:26

Search SDK: New Swagger spec for Search Management API 2015-08-19

e780e64

This spec covers all functionality of the Azure Search Management REST API version 2015-08-19, including key management and search service management, provisioning, and scaling.

Search SDK: Part 2 of fleshing out Managment plane documentation

cc90bdd

Search SDK: Updating max partition count for S3 HD

e72da3a

azurecla added the cla-not-required label Oct 25, 2016

hovsepm reviewed Oct 25, 2016

View reviewed changes

seansaleh approved these changes Oct 26, 2016

View reviewed changes

tbombach self-assigned this Oct 28, 2016

Search: Replacing MSDN links with aka.ms links

c6eff9c

The aka.ms links still point to MSDN, but they'll be updated to docs.microsoft.com after the migration.

tbombach suggested changes Oct 28, 2016

View reviewed changes

bitsforthought changed the title ~~Search SDK: Adding arm-search spec for 2015-08-19 API version~~ [DO NOT MERGE] Search SDK: Adding arm-search spec for 2015-08-19 API version Oct 28, 2016

Search: Adding examples to arm-search spec

9b23f29

bitsforthought changed the title ~~[DO NOT MERGE] Search SDK: Adding arm-search spec for 2015-08-19 API version~~ Search SDK: Adding arm-search spec for 2015-08-19 API version Oct 28, 2016

tbombach approved these changes Oct 31, 2016

View reviewed changes

tbombach merged commit 51bbe9d into Azure:master Oct 31, 2016

bitsforthought deleted the new-search-spec branch October 31, 2016 18:07

seansaleh reviewed Oct 31, 2016

View reviewed changes

bitsforthought mentioned this pull request Nov 24, 2016

Search: Adding new data plane Swagger specs for API version 2016-09-01 #756

Merged

10 tasks

This was referenced Mar 31, 2017

Search: Adding some missing descriptions to the 2016-09-01 spec #1086

Merged

AutoRest should provide a way to disable client-side validation Azure/autorest#2100

Closed

Conversation

bitsforthought commented Oct 25, 2016 • edited by amarzavery Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitsforthought commented Oct 28, 2016

Uh oh!

bitsforthought commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbombach left a comment

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!

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!

bitsforthought commented Oct 28, 2016

Uh oh!

bitsforthought commented Oct 28, 2016

Uh oh!

tbombach commented Oct 28, 2016

Uh oh!

bitsforthought commented Oct 28, 2016

Uh oh!

AutorestCI commented Oct 31, 2016

Uh oh!

AutorestCI commented Oct 31, 2016

Uh oh!

AutorestCI commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seansaleh Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bitsforthought commented Oct 25, 2016 •

edited by amarzavery

Loading

bitsforthought commented Oct 28, 2016 •

edited

Loading

seansaleh Oct 31, 2016 •

edited

Loading