Search: Adding new data plane Swagger specs for API version 2016-09-01 #756
Conversation
For now, these are identical to the 2015-02-28-Preview specs except for the version number, and all external URLs that pointed to azure.com or MSDN now point to docs.microsoft.com.
| } | ||
| }, | ||
| "definitions": { | ||
| "IndexingResult": { |
There was a problem hiding this comment.
The models described here are not being referenced anywhere in the spec, wondering what the intent is here
There was a problem hiding this comment.
@dsgouda It's a long story, but if you're going to be reviewing Azure Search specs frequently, you need to know it. :-)
These Swagger specs are for the Azure Search data plane API. AutoRest currently does not support generating the kind of client-side code we need to handle schematized customer data (since it assumes that model types are known at design time). Therefore, we have developed the Azure Search SDK using a mixture of code generation and hand-written code. This spec in particular omits a bunch of operations because the generated code for them would be so far off what we need that there is no point in generating them. Now that REST API docs are generated from Swagger, we realize that we need to add the operations even if we don't want code generated for them. However, we do not have the resources to undertake that effort right now. For now, our REST API docs on docs.microsoft.com are also hand-written.
The unreferenced types you see are in this Swagger spec because they are part of the API and we want code generated for them. It's just that the operations that use them are not modeled in the Swagger yet.
| "description": "Specifies the syntax of the search query. The default is 'simple'. Use 'full' if your query uses the Lucene query syntax." | ||
| }, | ||
| "SearchParametersPayload": { | ||
| "properties": { |
There was a problem hiding this comment.
seems like none of the parameters are required, is an empty SearchParametersPayload object a valid request?
| "description": "The OData $filter expression to apply to the search query." | ||
| }, | ||
| "highlight": { | ||
| "type": "string", |
There was a problem hiding this comment.
Generic comments: These string search parameters should ideally have a min/max length and regex constraints
There was a problem hiding this comment.
@dsgouda I have some (IMO) well-grounded objections to regex constraints. Rather than repeat them, I'll direct you to this discussion on an earlier PR.
Regarding min/max constraints, I believe they are valuable for documentation purposes where applicable. It may be the case that we have no limits on some of these properties right now, but if we do, they are documented in the (hand written) REST API reference. However, I would still refrain from adding them to the Swagger until AutoRest supports disabling client-side validation.
| "type": "string", | ||
| "description": "A full-text search query expression; Use null or \"*\" to match all documents." | ||
| }, | ||
| "searchFields": { |
There was a problem hiding this comment.
Any reason why this should be comma-separated, its better to have an array of strings here
There was a problem hiding this comment.
@dsgouda Hindsight is 20/20. This API has been in production for several years; We are not going to change it now. The churn for customers doesn't seem worth it.
| "type": "string", | ||
| "description": "The OData $filter expression to apply to the suggestions query." | ||
| }, | ||
| "fuzzy": { |
There was a problem hiding this comment.
Generic comment: if the service assumes a default value for these parameters its better to specify it here
There was a problem hiding this comment.
@dsgouda Noted. Many of these do not have defaults. Those that do are documented in our (hand written) REST API reference. We will flesh out this spec when we're ready to start generating docs from these specs. This is a longer-term goal for us.
| "type": "string", | ||
| "description": "A string tag that is prepended to hit highlights. Must be set with HighlightPostTag. If omitted, hit highlighting of suggestions is disabled." | ||
| }, | ||
| "minimumCoverage": { |
There was a problem hiding this comment.
This shares a bunch of properties with SearchParametersPayload maybe you can extract those out and use an "allof" instead
There was a problem hiding this comment.
@dsgouda The similarities between these two is incidental. We do not want them to be related by inheritance in the client programming model. Even if we did, changing it would cause a bunch of customer churn for no good reason.
Keep in mind, this is simply another iteration of an API that's existed for a few years now. We are well past the point of being able to make trivial changes without breaking backwards compatibility.
| "200": { | ||
| "description": "", | ||
| "schema": { | ||
| "$ref": "#/definitions/IndexListResult" |
There was a problem hiding this comment.
One thing I noticed:
IndexListResult
IndexerListResult
DocumentIndexResult
DataSourceListResult
merely encapsulate an array value (the actual result).
Consider marking operations that return one of the above as
"x-ms-pageable": {
"nextLinkName": null
}
This will make the encapsulated list the actual return type (e.g. IEnumerable<Indexer> instead of IndexerListResult) - in case that is something you desire.
There was a problem hiding this comment.
@olydis Thanks for the tip. I had noticed that feature when updating our Management SDK. However, our data plane SDK is still on a much older version of AutoRest and we didn't have the time to upgrade (this involves changing a lot of hand-written code to fix bugs fixed in the generated code, etc.). The next time we bump the major version number, we'll make this change.
|
No modification for Python |
|
No modification for NodeJS |
|
No modification for Ruby |
For now, these are identical to the 2015-02-28-Preview specs except for the version number, and all external URLs that pointed to azure.com or MSDN now point to docs.microsoft.com.
Reminder to reviewers: Azure Search data plane Swagger specs are not currently being used to generate REST API reference documentation; This is why responses do not all have descriptions.
FYI @chaosrealm @seansaleh @mhko @Yahnoosh
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
api-versionin the path should match theapi-versionin the spec).Quality of Swagger
For now, these are identical to the 2015-02-28-Preview specs except for the
version number, and all external URLs that pointed to azure.com or MSDN now
point to docs.microsoft.com.