Add examples for SQL Swagger#954
Conversation
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
@nathannfan thanks for adding these examples! I left some comments inline.
There looks to be some APIs that don't have examples associated with them, is that intentional on your end?
Additionally, Our linter/rule validation is throwing some warnings you may want to take a look at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204330238#L2368 . For the OperationsAPIImplementationValidation, here's an example of what's expected: https://github.com/Azure/azure-rest-api-specs/blob/master/arm-cdn/2016-10-02/swagger/cdn.json#L1578
| } | ||
| }, | ||
| "definitions": { | ||
| "AsyncOperationResponse": { |
There was a problem hiding this comment.
This definition is not being used, if not needed, it should be removed from the file.
| } | ||
| }, | ||
| "definitions": { | ||
| "AsyncOperationResponse": { |
There was a problem hiding this comment.
This definition is not referred from this file.
| "currentServiceObjectiveId": "f1173c43-91bd-4aaa-973c-54e79e15235b", | ||
| "requestedServiceObjectiveId": "f1173c43-91bd-4aaa-973c-54e79e15235b", | ||
| "requestedServiceObjectiveName": "S0", | ||
| "sampleName": null, |
There was a problem hiding this comment.
Is there a reason why you included this property with null value in the request body? The property doesn't seem to be required, and if that's correct and you remove the null properties from the request body, the errors REQUEST_VALIDATION_ERROR reported by the tool should go away: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204330240#L2507
There was a problem hiding this comment.
I may or may not have taken "every parameter specified" too literally and used response bodies to generate request bodies.
Removed null request parameters.
There was a problem hiding this comment.
Can you change the Max sample to specify sampleName?
| } | ||
| } | ||
| }, | ||
| "202": { |
There was a problem hiding this comment.
If this information is not useful to the user and the decision is to not include it in the swagger file, then it should also be removed from here. Do you expect the body on this response to be removed from the service API?
There was a problem hiding this comment.
I don't expect it to be removed from the Service API before we go GA with what we have here. I can remove it from examples.
There was a problem hiding this comment.
if the swagger is not doing to define a body for this error code, then the body should be removed from the example https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204765825#L2611
There was a problem hiding this comment.
Good point. Removed from examples.
…itr example. Also updated descriptions of existing restore related db properties.
There was a problem hiding this comment.
I see that there are usually 2 create examples one min and one max with more properties like ID, location, kind, type, name etc. Only location and kind are required for SQL Server, for all other resources none of these are settable and should be removed from examples. Getting 2 names from body and path is source of bugs for users, same is true for other parameters as well.
| "firewallRuleName": "firewallrulecrudtest-5370", | ||
| "api-version": "2014-04-01", | ||
| "parameters": { | ||
| "id": "/subscriptions/00000000-1111-2222-3333-444444444444/resourceGroups/firewallrulecrudtest-12/providers/Microsoft.Sql/servers/firewallrulecrudtest-6285/firewallRules/firewallrulecrudtest-3927", |
There was a problem hiding this comment.
properties id, location and kind are being ignored, and marked as read only for Firewall, you can remove this as this has no effect on the creation of firewall, also then I think there won't be 2 variants of this for min and max.
| "elasticPoolName": "sqlcrudtest-8102", | ||
| "api-version": "2014-04-01", | ||
| "parameters": { | ||
| "id": "/subscriptions/00000000-1111-2222-3333-444444444444/resourceGroups/sqlcrudtest-2369/providers/Microsoft.Sql/servers/sqlcrudtest-8069/elasticPools/sqlcrudtest-8102", |
There was a problem hiding this comment.
from creates remove ID, name, location (remove for child resource only), kind, type for resources. We should not encourage users to send these values in. I have not cross checked if in swagger these are marked as readonly or not. These should be marked readonly, so that we don't generate setter for the same and don't send these across network
There was a problem hiding this comment.
Done, and confirmed they are marked read-only.
| "firewallRuleName": "firewallrulecrudtest-3927", | ||
| "api-version": "2014-04-01", | ||
| "parameters": { | ||
| "id": "/subscriptions/00000000-1111-2222-3333-444444444444/resourceGroups/firewallrulecrudtest-12/providers/Microsoft.Sql/servers/firewallrulecrudtest-6285/firewallRules/firewallrulecrudtest-3927", |
There was a problem hiding this comment.
take care of removing id, location, kind, type etc.
There was a problem hiding this comment.
Done in all examples from the requests.
These properties have dependence on CreateMode that was not clearly explained.
|
Discussed with @veronicagg offline - the source of the warnings from the linter were here before this checkin, and mostly apply to API changes. Since this PR is just for examples, I plan to ignore these for now. |
| "Name": { | ||
| "readOnly": true, | ||
| "type": "string", | ||
| "description": "The name of the Azure SQL database being upgraded." |
There was a problem hiding this comment.
this description seems wrong
There was a problem hiding this comment.
Removed this definition, as it wasn't being used.
| "Name": { | ||
| "readOnly": true, | ||
| "type": "string", | ||
| "description": "The name of the Azure SQL Recommended Elastic Pool being upgraded." |
|
Can we say that |
Also added doc explaining special 0.0.0.0 value.
|
upgradeHint is not showing up in the DatabaseProperties - it seems to have been removed from the API. Removing from databaseProperties. |
| }, | ||
| "responses": { | ||
| "202": { | ||
| "body": { |
There was a problem hiding this comment.
if we're not defining this body in the swagger, because it wasn't useful to the user, we can remove the body from here and will avoid https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204765825#L2578
| } | ||
| } | ||
| }, | ||
| "202": { |
There was a problem hiding this comment.
if the swagger is not doing to define a body for this error code, then the body should be removed from the example https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204765825#L2611
| } | ||
| }, | ||
| "202": { | ||
| "body": { |
There was a problem hiding this comment.
body should be removed from the example is decision is not to model it in the swagger file.
| } | ||
| }, | ||
| "202": { | ||
| "body": { |
There was a problem hiding this comment.
body should be removed from the example is decision is not to model it in the swagger file.
| }, | ||
| "202": { | ||
| "body": { | ||
| "operation": "CreateDatabaseAsCopy", |
There was a problem hiding this comment.
body should be removed from the example is decision is not to model it in the swagger file.
| } | ||
| }, | ||
| "202": { | ||
| "body": { |
There was a problem hiding this comment.
body should be removed from the example is decision is not to model it in the swagger file.
| } | ||
| }, | ||
| "202": { | ||
| "body": { |
| } | ||
| }, | ||
| "202": { | ||
| "body": { |
There was a problem hiding this comment.
body should be removed from the example is decision is not to model it in the swagger file.
| } | ||
| } | ||
| }, | ||
| "responses": { |
There was a problem hiding this comment.
200 & 201 response examples are missing. https://travis-ci.org/Azure/azure-rest-api-specs/jobs/204765825#L2980
| } | ||
| } | ||
| }, | ||
| "/providers/Microsoft.Sql/operations": { |
There was a problem hiding this comment.
is this API documented? works? it looks like each of the swagger files include the same api "/providers/Microsoft.Sql/operations", shouldn't it be different for firewallRules.json, replicationLinks.json, and sql.core.json?
There was a problem hiding this comment.
They're all under the Microsoft.Sql provider, so calling this actually lists all of them. I just confirmed this in Fiddler. Is there a way to define it in one place and reference it from all files?
There was a problem hiding this comment.
I don't think we have a way to reference the operation from one place, so if it lists all the operations and work, the generated SDK code will include the API on each client.
|
No modification for NodeJS |
6 participants
This change Updated with examples and appropriate API changes made while validating the API's through Fiddler and the examples through the openAPI validation tools.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger