azure-rest-api-specs
azure-rest-api-specs copied to clipboard
Azure
Search api data plane suggestions facets
Data Plane API - Pull Request
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your engagement record.
- Link to engagement record issue:
Is this review for (select one):
- [ ] a private preview
- [x] a public preview
- [ ] GA release
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous Open API document (swagger) if applicable, and the root paths that have been updated.
- Design Document:
- Previous Open API Doc:
- Updated paths:
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
- 💬 Teams Channel
Click here for links to tools, specs, guidelines & other good stuff
Tooling
- Open API validation tools were run on this PR. Go here to see how to fix errors
- Spectral Linting
- Open API Hub
Guidelines & Specifications
Helpful Links
Hi, @gregoks Thanks for your PR. I am workflow bot for review process. Here are some small tips.
![openapi-workflow-bot[bot] avatar](https://avatars.githubusercontent.com/in/81125?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger Validation Report
️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️⚠️Breaking Change(Cross-Version): 17 Warnings warning [Detail]
| compared swaggers (via Oad v0.10.1)] | new version | base version |
|---|---|---|
| search.json | 2022-09-25-preview(c0ea1d6) | 2022-08-17-preview(main) |
The following breaking changes are detected by comparison with the latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
| compared tags (via openapi-validator v1.13.0) | new version | base version |
|---|---|---|
| package-2022-09-25-preview | package-2022-09-25-preview(c0ea1d6) | default(main) |
| package-2022-09-25-preview | package-2022-09-25-preview(c0ea1d6) | default(main) |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ApiReadinessCheck succeeded [Detail] [Expand]
️️✔️~[Staging] ServiceAPIReadinessTest succeeded [Detail] [Expand]
Validation passes for ServiceAPIReadinessTest.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️CadlValidation succeeded [Detail] [Expand]
Validation passes for CadlValidation.
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
![openapi-pipeline-app[bot] avatar](https://avatars.githubusercontent.com/in/65541?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger pipeline restarted successfully, please wait for status update in this comment.
Swagger pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.
Hi @gregoks, Your PR has some issues. Please fix the CI sequentially by following the order of Avocado, semantic validation, model validation, breaking change, lintDiff. If you have any questions, please post your questions in this channel https://aka.ms/swaggersupport.
| Task | How to fix | Priority |
|---|---|---|
| Avocado | Fix-Avocado | High |
| Semantic validation | Fix-SemanticValidation-Error | High |
| Model validation | Fix-ModelValidation-Error | High |
| LintDiff | Fix-LintDiff | high |
NewApiVersionRequired reason: A service’s API is a contract with customers and is represented by using the api-version query parameter. Changes such as adding an optional property to a request/response or introducing a new operation is a change to the service’s contract and therefore requires a new api-version value. This is critically important for documentation, client libraries, and customer support. EXAMPLE: if a customer calls a service in the public cloud using api-version=2020-07-27, the new property or operation may exist but if they call the service in a government cloud, air-gapped cloud, or Azure Stack Hub cloud using the same api-version, the property or operation may not exist. Because there is no clear relationship between the service api-version and the new property/operation, customers can’t trust the documentation and Azure customer have difficulty helping customers diagnose issues. In addition, each client library version documents the service version it supports. When an optional property or new operation is added to a service and its Swagger, new client libraries must be produced to expose this functionality to customers. Without updating the api-version, it is unclear to customers which version of a client library supports these new features.
GET /search
- for azureBenefit, if you say that the default is null, then you must actually allow null as a value along with Eligible and NotElegible. I think you the description to say what happen if the parameter is not specified and not make mention of a default.
- For all the query params, you should say what happens if it is not specified.
- industryCloud might be better is it were just 'cloud' and the value were industry and whatever is the opposite of industry.
- For 'select' in the description, 'pricingTypes' should start with an uppercase P. Remember that the service should treat all these values as case-sensitive and a case-mismatch should result in a 400-Bad Request.
- ordery by needs to document all the possible values. You also need to document the possible value for many of the other parms as well: operatinsSystems, supportedProducts, applicableProducts, publisherIds, etc.
- For 'productTypes' I suggest alphabetizing the list.
- Do you really want the 'Types' suffix for some of your query params? Wouldn't 'product', 'publisher', 'pricing', 'vmArchitecture'. 'vmSecurity' etc be good enough?
- vmImageGenerations is very confusing to me. For example: What does 1 map to?
- vmArchitectureTypes: Can't they just specific x64 or ARM, do you need the 1 & 2?
- I think you need to make it clear for parameters that take multiple values if that mean OR or AND. For example: what happens if vmSecurityTypes is None,Trusted.
- top: top is NOT how many items to return on a page; it is how many items to return across all pages. There is usually no maximum for top and the default is infinity. If you want the customer to limit the number of items returned per page then use maxpagesize query parameter (please see our guidelines about this).
- x-ms-app: Azure's policy is that the app name can be in the user agent header; not in an x-ms-app header. Please see our guidelines about this.
- We do not put specific error codes in swagger (like 400 or 500); just include the default Azure error response
- In the response body, Azure uses "value" instead of "results" and this is what is pageable right? If so, then will only the first page include the 'facets' section or will this be repeated for each page?
GET /search
- ALmost all of the comments I made above apply to this operation as well.
GET /suggestions/products
-
Again, same comments as above.
-
In the response body: "suggestions" should be "value"
-
I see no nextLink property but I think this is correct here since there would never be any paging, right?
GET /search
- for azureBenefit, if you say that the default is null, then you must actually allow null as a value along with Eligible and NotElegible. I think you the description to say what happen if the parameter is not specified and not make mention of a default.
azureBenefit - we mention the information that you wrote in the description of the field, if no value is passed then we ignore this field. That's how it was defined by our PM and unfortunately we cannot change it.
- For all the query params, you should say what happens if it is not specified.
Added to description of all fields
- industryCloud might be better is it were just 'cloud' and the value were industry and whatever is the opposite of industry.
It was already approved in the previous review and also reviewed by our PM so we cannot change it unfortunately.
- For 'select' in the description, 'pricingTypes' should start with an uppercase P. Remember that the service should treat all these values as case-sensitive and a case-mismatch should result in a 400-Bad Request.
Done.
- ordery by needs to document all the possible values. You also need to document the possible value for many of the other parms as well: operatinsSystems, supportedProducts, applicableProducts, publisherIds, etc.
Done
- For 'productTypes' I suggest alphabetizing the list.
Done
- Do you really want the 'Types' suffix for some of your query params? Wouldn't 'product', 'publisher', 'pricing', 'vmArchitecture'. 'vmSecurity' etc be good enough?
Yes I need them since its based on CosmosDB properties and is defined with the same name in multiple internal services and DB's.
- vmImageGenerations is very confusing to me. For example: What does 1 map to?
- vmArchitectureTypes: Can't they just specific x64 or ARM, do you need the 1 & 2?
Again its based on internal data and we are working to update the values in future versions.
- I think you need to make it clear for parameters that take multiple values if that mean OR or AND. For example: what happens if vmSecurityTypes is None,Trusted.
Done
- top: top is NOT how many items to return on a page; it is how many items to return across all pages. There is usually no maximum for top and the default is infinity. If you want the customer to limit the number of items returned per page then use maxpagesize query parameter (please see our guidelines about this).
Checking regarding using maxpagesize instead of top
- x-ms-app: Azure's policy is that the app name can be in the user agent header; not in an x-ms-app header. Please see our guidelines about this.
The guidelines talk mostly about SDK and since we're not SDK, this value is for our internal usage and since clients already use this header, we dont want to change it at the moment. We will consider moving to user-agent header in our next versions.
- We do not put specific error codes in swagger (like 400 or 500); just include the default Azure error response
I am using the ErrorRespone . in the default error description , I just put text with a bit of explanation with possible http error codes.
- In the response body, Azure uses "value" instead of "results" and this is what is pageable right? If so, then will only the first page include the 'facets' section or will this be repeated for each page?
Facets and Suggestions wont be pageable, they wont have many rows. Search currently isnt pageable but will be in the future. We will discuss internally about returning facets as part of search in each page or not. But its not relevant for this version.
GET /search
- ALmost all of the comments I made above apply to this operation as well.
Done
GET /suggestions/products
- Again, same comments as above.
- In the response body: "suggestions" should be "value"
Done
- I see no nextLink property but I think this is correct here since there would never be any paging, right?
Suggestion isnt pageable.
GET /search
- for azureBenefit, if you say that the default is null, then you must actually allow null as a value along with Eligible and NotElegible. I think you the description to say what happen if the parameter is not specified and not make mention of a default.
azureBenefit - we mention the information that you wrote in the description of the field, if no value is passed then we ignore this field. That's how it was defined by our PM and unfortunately we cannot change it.
- For all the query params, you should say what happens if it is not specified.
Added to description of all fields
- industryCloud might be better is it were just 'cloud' and the value were industry and whatever is the opposite of industry.
It was already approved in the previous review and also reviewed by our PM so we cannot change it unfortunately.
- For 'select' in the description, 'pricingTypes' should start with an uppercase P. Remember that the service should treat all these values as case-sensitive and a case-mismatch should result in a 400-Bad Request.
Done.
- ordery by needs to document all the possible values. You also need to document the possible value for many of the other parms as well: operatinsSystems, supportedProducts, applicableProducts, publisherIds, etc.
Done
- For 'productTypes' I suggest alphabetizing the list.
Done
- Do you really want the 'Types' suffix for some of your query params? Wouldn't 'product', 'publisher', 'pricing', 'vmArchitecture'. 'vmSecurity' etc be good enough?
Yes I need them since its based on CosmosDB properties and is defined with the same name in multiple internal services and DB's.
- vmImageGenerations is very confusing to me. For example: What does 1 map to?
- vmArchitectureTypes: Can't they just specific x64 or ARM, do you need the 1 & 2?
Again its based on internal data and we are working to update the values in future versions.
- I think you need to make it clear for parameters that take multiple values if that mean OR or AND. For example: what happens if vmSecurityTypes is None,Trusted.
Done
- top: top is NOT how many items to return on a page; it is how many items to return across all pages. There is usually no maximum for top and the default is infinity. If you want the customer to limit the number of items returned per page then use maxpagesize query parameter (please see our guidelines about this).
Checking regarding using maxpagesize instead of top
- x-ms-app: Azure's policy is that the app name can be in the user agent header; not in an x-ms-app header. Please see our guidelines about this.
The guidelines talk mostly about SDK and since we're not SDK, this value is for our internal usage and since clients already use this header, we dont want to change it at the moment. We will consider moving to user-agent header in our next versions.
- We do not put specific error codes in swagger (like 400 or 500); just include the default Azure error response
I am using the ErrorRespone . in the default error description , I just put text with a bit of explanation with possible http error codes.
- In the response body, Azure uses "value" instead of "results" and this is what is pageable right? If so, then will only the first page include the 'facets' section or will this be repeated for each page?
Facets and Suggestions wont be pageable, they wont have many rows. Search currently isnt pageable but will be in the future. We will discuss internally about returning facets as part of search in each page or not. But its not relevant for this version.
GET /search
- ALmost all of the comments I made above apply to this operation as well.
Done
GET /suggestions/products
- Again, same comments as above.
- In the response body: "suggestions" should be "value"
Done
- I see no nextLink property but I think this is correct here since there would never be any paging, right?
Suggestion isnt pageable.
@JeffreyRichter Regarding the top vs maxpagesize comment, we will want to keep using "top" since inside our service calls the azure cognitive search and they also use "$top" for their server side paging:
https://learn.microsoft.com/en-us/rest/api/searchservice/search-documents
Hi @gregoks, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. Action: To initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki. If you want to know the production traffic statistic, please see ARM Traffic statistic. If you think it is false positive breaking change, please provide the reasons in the PR comment, report to Swagger Tooling Team via https://aka.ms/swaggerfeedback. Note: To avoid breaking change, you can refer to Shift Left Solution for detecting breaking change in early phase at your service code repository.
Regarding the top vs maxpagesize comment, we will want to keep using "top" since inside our service calls the azure cognitive search and they also use "$top" for their server side paging:
https://learn.microsoft.com/en-us/rest/api/searchservice/search-documents
I think I'm a bit confused by this statement - yes, top is used for server driven paging, but what @JeffreyRichter calls out is both correct and how search has implemented it (per the link provided).
azureBenefit - we mention the information that you wrote in the description of the field, if no value is passed then we ignore this field. That's how it was defined by our PM and unfortunately we cannot change it.
You are confusing passing no value and passing a null value here. They are not the same. At the very least, the description should be updated.
Regarding the top vs maxpagesize comment, we will want to keep using "top" since inside our service calls the azure cognitive search and they also use "$top" for their server side paging:
https://learn.microsoft.com/en-us/rest/api/searchservice/search-documents
I think I'm a bit confused by this statement - yes, top is used for server driven paging, but what @JeffreyRichter calls out is both correct and how search has implemented it (per the link provided).
The internal Azure cognitive search service uses the $top parameter for pagination. the parameter in our API was already discussed in the stewardship committee when we presented the API and it was recommended by the committee to use "top" parameter. I wouldn't want to change it especially since we told our internal clients to use "top" as well by the committee recommendation.