Document the Api Schema Contract by solankisamir · Pull Request #1801 · Azure/azure-rest-api-specs

solankisamir · 2017-10-05T05:05:11Z

Based on issue #1776

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.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

solankisamir · 2017-10-05T16:31:09Z

@ravbhatnagar FYI

jhendrixMSFT · 2017-10-05T16:29:49Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

+        "tags": [
+          "ApiSchema"
+        ],
+        "operationId": "ApiSchema_CreateOrUpdate",


Example missing for this operation.

jhendrixMSFT · 2017-10-05T16:32:26Z

.../resource-manager/Microsoft.ApiManagement/2017-03-01/examples/ApiManagementGetApiSchema.json

+                            "ProductArray": {
+                                "type": "array",
+                                "items": {
+                                    "$ref": "#/definitions/Product"


This reference cannot be resolved, see log.

That is just a Schema. It will differ from Api to API. I will update the example with a WSDL Api Schema, so that the tooling doesn't get confused.

jhendrixMSFT · 2017-10-05T16:33:18Z

.../resource-manager/Microsoft.ApiManagement/2017-03-01/examples/ApiManagementGetApiSchema.json

+                            "PriceEstimateArray": {
+                                "type": "array",
+                                "items": {
+                                    "$ref": "#/definitions/PriceEstimate"


This reference cannot be resolved, see log.

That is just a Schema. It will differ from Api to API. I will update the example with a WSDL Api Schema, so that the tooling doesn't get confused.

jhendrixMSFT · 2017-10-05T16:40:27Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

+        "tags": [
+          "ApiSchema"
+        ],
+        "operationId": "ApiSchema_ListByApi",


Since this operation returns pageable results I believe you need to annotate it with the x-ms-pageable extension. See here for more information.

nice catch. #fixed

azuresdkciprbot · 2017-10-06T22:16:58Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/apimanagement/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 22
After the PR: Warning(s): 0 Error(s): 22

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

jhendrixMSFT · 2017-10-06T22:21:36Z

There are still some model validation failures for the examples, see the log. Note that you can run the model validation locally, you can find instructions for installing the validation tool here, use the validate-example command.

azuresdkciprbot · 2017-10-06T22:45:03Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/apimanagement/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 22
After the PR: Warning(s): 0 Error(s): 22

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

ravbhatnagar

Looks fine. A few minor notes/questions

ravbhatnagar · 2017-10-06T23:02:11Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

+      "description": "Schema Contract details."
+    },
+    "SchemaContractProperties": {
+      "properties": {


provisioningState property missing? DO you need it? Just wanted to check since you are returning 201. But its not long running so its not required. But still checking.

This is again a proxy resource in ARM. I guess we dont need the provisioningState property.

In reply to: 143310129 [](ancestors = 143310129)

Again, I think we should have a call and clarify a few things around tracked and non-tracked :). If it is long running operation, and you are returning 201 for the LRO, you will need to add a provisioning state property. Is it LRO?

This is not a LRO.

ravbhatnagar · 2017-10-06T23:03:40Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

+          "type": "string",
+          "description": "Must be a valid a media type used in a Content-Type header as defined in the RFC 2616. Media type of the schema document (e.g. application/json, application/xml)."
+        },
+        "document": {


Is there a upper limit on the size of this?

no upper size limit.

In reply to: 143310265 [](ancestors = 143310265)

sounds good.

ravbhatnagar · 2017-10-06T23:04:30Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

+            "$ref": "./apimanagement.json#/parameters/SubscriptionIdParameter"
+          }
+        ],
+        "responses": {


Also add a 200 OK response.

We only return a 204 on Delete. This is an untracked resource in ARM.

In reply to: 143310351 [](ancestors = 143310351)

@solankisamir - being untracked or tracked has no bearing on this. 200 on a DELETE should be the response code returned to the customers if the resource was successfully deleted. 204OK is fine to return when the requested resource was not found but the URI was well formed. This is as per the RPC and applies to both tracked and untracked resource.

As per https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.7 if the response does not return entity then 204 is expected. We don't return entity on DELETE.

@solankisamir - Yes, I am aware of that and it is one place where ARM recommendation slightly different. But as per RPC, the recommendation is as follows -
"The resource provider can return 200 (OK) or 204 (NoContent) to indicate that the operation completed successfully. A 200 (OK) should be returned if the object exists and was deleted successfully; and a 204 (NoContent) should be used if the resource does not exist and the request is well formed."
https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/resource-api-reference.md#delete-resource
It will work and everything as you have it now, but for consistency across the azure platform, I think this should be fixed.

@ravbhatnagar we don't return 200 for any of our DELETE currently on this and it's parent entity and we have this behavior documented. We believe we can do this consistently across all entities with a new api-version, as it could be breaking change for some customers who have already taken dependency. What do you suggest?

@solankisamir Presumably this has been the behavior for some time, so with that in mind IMO fixing this in a new API version is the correct way to go. @ravbhatnagar are you ok with that?

Yes, I have opened a github issue tracking this #1847

ravbhatnagar · 2017-10-06T23:05:49Z

specification/apimanagement/resource-manager/Microsoft.ApiManagement/2017-03-01/apimapis.json

        }
      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ApiManagement/service/{serviceName}/apis/{apiId}/schemas": {


Is schemas a tracked or a untracked resource. If tracked PATCH would be needed. But I am guessing this is untracked resource. Please confirm

untracked resource

In reply to: 143310482 [](ancestors = 143310482)

jhendrixMSFT · 2017-10-09T21:21:37Z

Looks good from SDK perspective. Once we have ARM sign-off we can merge.

solankisamir · 2017-10-10T01:10:17Z

@ravbhatnagar let me know if you have any questions. We are waiting on your sign-off.

ravbhatnagar · 2017-10-10T01:43:24Z

@solankisamir - left some comments
@jhendrixMSFT - based on the answers to the 2 open questions, feel free to merge. for #1, the RP should return a 200 OK for delete. For #2, if its long running and RP is returning a 201 response, a provisioningState needs to be added. If not, then its ok.

AutorestCI · 2017-10-11T22:39:16Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-10-11T22:39:22Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-10-11T22:40:03Z

No modification for AutorestCI/azure-sdk-for-ruby

Document the Api Schema Contract

ebfa7fb

azuresdkci assigned jhendrixMSFT Oct 5, 2017

jhendrixMSFT added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Oct 5, 2017

jhendrixMSFT requested changes Oct 5, 2017

View reviewed changes

Example for Create call and missing the pageable link

25fd037

Fix the ApiSchemaDocument object.

6a68d06

jhendrixMSFT requested a review from ravbhatnagar October 6, 2017 22:49

ravbhatnagar reviewed Oct 6, 2017

View reviewed changes

ravbhatnagar added ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 7, 2017

jhendrixMSFT approved these changes Oct 11, 2017

View reviewed changes

jhendrixMSFT merged commit 25b9acd into Azure:current Oct 11, 2017

Conversation

solankisamir commented Oct 5, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

solankisamir commented Oct 5, 2017

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!

azuresdkciprbot commented Oct 6, 2017

Uh oh!

jhendrixMSFT commented Oct 6, 2017

Uh oh!

azuresdkciprbot commented Oct 6, 2017

Uh oh!

ravbhatnagar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solankisamir Oct 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

solankisamir Oct 10, 2017 • 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!

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!

solankisamir Oct 10, 2017 • 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!

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!

jhendrixMSFT commented Oct 9, 2017

Uh oh!

solankisamir commented Oct 10, 2017

Uh oh!

ravbhatnagar commented Oct 10, 2017

Uh oh!

AutorestCI commented Oct 11, 2017

Uh oh!

AutorestCI commented Oct 11, 2017

solankisamir commented Oct 5, 2017 •

edited by azuresdkci

Loading

solankisamir Oct 7, 2017 •

edited

Loading

solankisamir Oct 10, 2017 •

edited

Loading

solankisamir Oct 10, 2017 •

edited

Loading