azure-rest-api-specs
azure-rest-api-specs copied to clipboard
Azure
[Maps] Async Batch APIs for Forward/Reverse Geocode and Routing
Data Plane API - Pull Request
Add async batch APIs for forward geocode, reverse geocode, and routing.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
- Link to API Spec engagement record issue: https://github.com/Azure/azure-rest-api-specs/issues/27158
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 API Spec document (if applicable), and the root paths that have been updated.
- Design Document:
- Service Design: https://microsoft.sharepoint.com/:w:/t/Azure-Maps/Eb8_fl1CuqpMkFYqGIW5cqUB5W-SY1MPbEwqOmS14mToVw?e=b3Bg37
- API spec: https://microsoft.sharepoint.com/:w:/t/Azure-Maps/ERzkm1S1_ehPmrpXSKwtcogBruwDXaBQnKq99N9UeVa0-g?e=bwoPYY
- Previous API Spec Doc: NA
- Updated paths:
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.
Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the Swagger-Suppression-Process to get approval.
❔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
Checks stuck in `queued` state?
If the PR CI checks appear to be stuck in `queued` state, please add a comment with contents `/azp run`. This should result in a new comment denoting a `PR validation pipeline` has started and the checks should be updated after few minutes.
Next Steps to Merge
Important checks have failed. As of today they are not blocking this PR, but in near future they will.Addressing the following failures is highly recommended:
- ⚠️ The check named
TypeSpec Requirement (data-plane)has failed. TypeSpec usage is required for all new (greenfield) services. This is currently enforced as a warning for data-plane specs, but will be made a blocking error in the near future. For information on converting from OpenAPI specs to TypeSpec specs or on data-plane (DP) policies, refer to aka.ms/azsdk/typespec. If you have general questions on resource provider (RP) policies, refer to aka.ms/rphelp.
![openapi-pipeline-app[bot] avatar](https://avatars.githubusercontent.com/in/65541?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): 9 Warnings warning [Detail]
| Compared specs (v0.10.8) | new version | base version |
|---|---|---|
| route.json | 2024-04-01-preview(18e0f51) | 2023-10-01-preview(main) |
| search.json | 2024-04-01-preview(18e0f51) | 2023-06-01(main) |
| search.json | 2024-04-01-preview(18e0f51) | 2022-12-01-preview(main) |
The following breaking changes are detected by comparison with the latest stable version:
| Rule | Message |
|---|---|
|
The new version is missing a definition that was found in the old version. Was 'default' removed or renamed? New: Search/preview/2024-04-01-preview/search.json#L52:3 Old: Search/stable/2023-06-01/search.json#L52:3 |
The following breaking changes are detected by comparison with the latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 34 Warnings warning [Detail]
| Compared specs (v2.2.1) | new version | base version |
|---|---|---|
| package-preview-2024-04 | package-preview-2024-04(18e0f51) | default(main) |
| package-preview-2024-04 | package-preview-2024-04(18e0f51) | default(main) |
| package-2024-04-01-preview | package-2024-04-01-preview(18e0f51) | default(main) |
| package-stable-2023-06-01 | package-stable-2023-06-01(18e0f51) | package-stable-2023-06-01(main) |
| package-preview-2024-04 | package-preview-2024-04(18e0f51) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
Only 30 items are listed, please refer to log for more details.
| Rule | Message | Related RPC [For API reviewers] |
|---|---|---|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L120 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L120 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L160 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L160 |
|
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L302 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L244 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L764 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L764 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L120 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L120 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L160 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L160 |
|
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: AsyncBatchManagement/preview/2024-04-01-preview/asyncBatchManagement.json#L302 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L121 |
|
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Route/preview/2024-04-01-preview/route.json#L136 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L184 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L244 |
|
| :warning: EnumInsteadOfBoolean | Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Route/preview/2024-04-01-preview/route.json#L354 |
|
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L848 |
|
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L853 |
|
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L996 |
|
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L1001 |
|
| :warning: EnumInsteadOfBoolean | Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Route/preview/2024-04-01-preview/route.json#L1791 |
|
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: Route/preview/2024-04-01-preview/route.json#L2048 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
|
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L515 |
The following errors/warnings exist before current PR submission:
Only 30 items are listed, please refer to log for more details.
| Rule | Message |
|---|---|
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L121 |
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Route/preview/2024-04-01-preview/route.json#L136 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Route/preview/2024-04-01-preview/route.json#L184 |
| :warning: EnumInsteadOfBoolean | Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Route/preview/2024-04-01-preview/route.json#L354 |
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L848 |
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L853 |
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L996 |
| :warning: PropertyType | Property should have a defined type. Location: Route/preview/2024-04-01-preview/route.json#L1001 |
| :warning: EnumInsteadOfBoolean | Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Route/preview/2024-04-01-preview/route.json#L1791 |
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: Route/preview/2024-04-01-preview/route.json#L2048 |
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Search/preview/2024-04-01-preview/search.json#L289 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L403 |
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L403 |
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Search/preview/2024-04-01-preview/search.json#L418 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L455 |
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L455 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L585 |
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L585 |
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Search/preview/2024-04-01-preview/search.json#L600 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L652 |
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L652 |
| :warning: PaginationResponse | Operation might be pageable. Consider adding the x-ms-pageable extension. Location: Search/preview/2024-04-01-preview/search.json#L667 |
| :warning: ErrorResponse | Error schema should define code and message properties as required.Location: Search/preview/2024-04-01-preview/search.json#L704 |
| :warning: ErrorResponse | The error property in the error response schema should be required.Location: Search/preview/2024-04-01-preview/search.json#L704 |
| :warning: SchemaDescriptionOrTitle | Schema should have a description or title. Location: Search/preview/2024-04-01-preview/search.json#L780 |
| :warning: SchemaDescriptionOrTitle | Schema should have a description or title. Location: Search/preview/2024-04-01-preview/search.json#L1049 |
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: Search/preview/2024-04-01-preview/search.json#L1074 |
| :warning: SchemaDescriptionOrTitle | Schema should have a description or title. Location: Search/preview/2024-04-01-preview/search.json#L1102 |
| :warning: SchemaDescriptionOrTitle | Schema should have a description or title. Location: Search/preview/2024-04-01-preview/search.json#L1108 |
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: Search/preview/2024-04-01-preview/search.json#L1131 |
️⚠️Avocado: 1 Warnings warning [Detail]
| Rule | Message |
|---|---|
| :warning: MULTIPLE_API_VERSION | The default tag contains multiple API versions swaggers. readme: specification/maps/data-plane/readme.md tag: specification/maps/data-plane/readme.md#tag-package-stable-2023-06-01 |
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
️🔄ApiDocPreview inProgress [Detail]
Generated ApiView
| Language | Package Name | ApiView Link |
|---|---|---|
| Swagger | AsyncBatchManagement | https://apiview.dev/Assemblies/Review/f86e0470c79f4026abe33303866b1dfd?revisionId=e34ef13b14ce4bf3970d6503a8da5fc1 |
| Swagger | Route | https://apiview.dev/Assemblies/Review/46fc96bf83a8468da2623389b47566b4?revisionId=e177a5cf729541c4bc9c1e98a77c185b |
| Swagger | Search | https://apiview.dev/Assemblies/Review/30ba16cfc8324624ae002a76347faac0?revisionId=1682045926794d948c48b877454aa7f1 |
I don't see a baseline commit, so it's hard to know exactly what's new in this api-version. I also see several RPs in this review. Do you have any documentation that highlights what is new for us to review? We likely will not have time to review the entirety of this PR.
Hi @danielthank! For review efficiency consideration, when creating a new API version, it is required to place API specs of the base version in the first commit, and push new version updates into successive commits. You can use OpenAPIHub to initialize the PR for adding a new version.
For more details refer to the wiki.
![openapi-workflow-bot[bot] avatar](https://avatars.githubusercontent.com/in/81125?v=4 "Published by a pub.dev verified publisher"){kind=small}
Also, from the meeting, I see the confusion: you're treating each group of endpoints as a separate resource provider, which is atypical and confusing. You can put things in separate swaggers, but you should combine all swaggers as a single service in the top-level readme.md and ship together e.g., https://github.com/Azure/azure-rest-api-specs/blob/main/specification/keyvault/data-plane/readme.md#tag-package-75
@heaths Thanks for the review! I have addressed all of the comments you left and just top-level readme.md is left now. I will consult with SDK members in our team and come up with a better solution. The following is the justification for the breaking changes. Please let me know what I need to do to get an approval.
Justification for breaking changes
- common.json I am trying to follow Azure API guideline and fix the existing error response model. It won't cause breaking change because we have already been returning the error response that is aligned with the Azure API guideline.
- cross version They are also caused by the error response model change. The error response for GA version APIs in production aligns with the error response model of Azure API guideline.
Justification for SpellCheck
- The
GeocodingResponsein the description is for referencing the model and the writer wrote it to make the docs more readable. I think it's better to leave it as it was.
Let me try explain the problem of api-version.
case 1
User initiates with REST API /route/directions:asyncBatch?api-version=A.
During polling, SDK would invoke /asyncBatch/operations/{operationId}?api-version=A (even if your operation-location header from /route returns api-version=B)
This means "AsyncBatchManagement" Swagger would support all api-versions from any of the service doing the async.
case 2
For now, your service does not have a resource model. So this does not apply. But when you have one, the reverse of case 1 would also happen.
User can start with REST API /asyncBatch/operations/{operationId}?api-version=A, which in the end (after LRO completed), could result in a resource in the other service e.g. /route/someresource?api-version=A
Let me try explain the problem of api-version.
case 1
User initiates with REST API
/route/directions:asyncBatch?api-version=A. During polling, SDK would invoke/asyncBatch/operations/{operationId}?api-version=A(even if youroperation-locationheader from/routereturnsapi-version=B)This means "AsyncBatchManagement" Swagger would support all api-versions from any of the service doing the async.
case 2
For now, your service does not have a resource model. So this does not apply. But when you have one, the reverse of case 1 would also happen.
User can start with REST API
/asyncBatch/operations/{operationId}?api-version=A, which in the end (after LRO completed), could result in a resource in the other service e.g./route/someresource?api-version=A
Loop in @JeffreyRichter for api-version discussion.
@weidongxu-microsoft Thank you for the review. Following the POST or DELETE LRO pattern guideline, we will
- Return the
Operation-Locationheader withapi-versionthe same as what users pass for job submission. - The status monitor endpoint will be backward compatible, which means the client can replace the
api-versioninOperation-Locationheader with newer api-version and get the status and result successfully.
For case 2, considering we have already decided to use POST pattern, we won't get a resource model in the status monitor. It means SDK users will not pass any part of the result of status monitor to another function in the SDK.
For case 2, the model which contains "outputBlobUrl" is kind of a versioned response already (just a very simple one, for now).
You can have v1 with only "outputBlobUrl" in e.g. route; and v2 with e.g. "outputBlobUrl" and "additionalProperty". Whether to return "additionalProperty" in the response would depends on the api-version that initiate the /asyncBatch/operations/{operationId}.
@weidongxu-microsoft that's right. We have a model called AsyncBatchOperationResult in the swagger. There's nothing stopping us from extending the model in the future.
@heaths @weidongxu-microsoft @JeffreyRichter We are ready to merge this PR. Please let us know if there's anything we need to change. Thank you.
Please fix SpellCheck CI.
Also review BreakingChange CI, verify they are expected or false alert. If expected, you need approval from breaking change board.
@weidongxu-microsoft I have written justification of SpellCheck and BreakingChange in previous discussion.
You must justify the breaking changes: https://github.com/Azure/azure-rest-api-specs/pull/27188/checks?check_run_id=21209757976
The entire service must version uniformly. Please update all swaggers so that they all share the same api-version value.
For spellcheck, click the "how to fix" link. which points to https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/ci-fix.md#spell-check
Basically, update cspell.json
@pbrasil Please help to clarify the uniform api version concern raised by Jeffrey.
cc @jsedlak-microsoft
@heaths @weidongxu-microsoft The breaking change pipeline fail is fixed. Can you help to review again and sign off if there are no further issue.
New change after the last review
- field linkedResource is replaced by outputStorageUrl. outputStorageUrl is the storage account blob storage url for storing the output result.
The entire service must version uniformly <= action raised by @JeffreyRichter The Azure Maps service comprises various category APIs, each maintaining its own API version. This situation has persisted for some time. While there is a plan to unify these versions, it will not be addressed in this PR.
@jhendrixMSFT Could you help merge this PR? We have got review board's approval.
@JeffreyRichter has this been reviewed and approved? I don't see the necessary tags.
@heaths was commenting in the APIView. Heath, are you ready to sign off on this PR?
@JeffreyRichter I don't have time to re-review so if you guys are happy with it, go ahead and merge. I don't want to sign off since I didn't review, but I can't seem to dismiss my review. The top doesn't show I'm blocking:
And there's no dismiss option in the summary at the bottom like I've seen on other PRs:
The author can't dismiss it either, which I know is possible because people have dismissed mine before (for good or bad reasons).
Can you dismiss it? Otherwise, I can just click the "Approve" option from the last screenshot above with a note I didn't review it.
To be clear, I'm only approving to unblock. Seems the option to dismiss my review is missing from this PR; though, I've seen it before. I just don't have time to re-review in full right now.