Common specs fixes by EvgenyAgafonchikov · Pull Request #1603 · Azure/azure-rest-api-specs

EvgenyAgafonchikov · 2017-08-29T08:54:34Z

Updated descriptions
CheckDnsNameAvailability.domainNameLabel.required: false -> true
Updated usages example, updated usages to suite current server response

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

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.

azuresdkciprbot · 2017-08-29T09:05:34Z

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/network/resource-manager/readme.md
Before the PR: Warning(s): 151 Error(s): 84
After the PR: Warning(s): 151 Error(s): 84

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

fearthecowboy

I've got reservations here -- you're introducing breaking changes (and example data isn't matching types in the spec)

fearthecowboy · 2017-08-29T19:22:34Z

specification/network/resource-manager/Microsoft.Network/2017-08-01/checkDnsAvailability.json

          {
            "name": "domainNameLabel",
            "in": "query",
-            "required": false,


Was this a mistake previously?

If they didn't pass it in would this fail?

This shouldn't be a binary breaking change, but it is technically be a source-level-breaking change (since someone could have omitted the parameter and now would have to pass one).

domainNameLabel is a required property on the service. If users were not passing the parameter, the service would throw an exception.

Yes, it is a breaking change on the client but it is required and i dont expect any customers to get impacted since we were anyway throwing .

This is a bug that we are fixing on the client

I figured as much -- it's not likely to cause a breaking change in the generated code unless someone used the function without the parameter, which would have failed... works for me.

fearthecowboy · 2017-08-29T19:31:24Z

specification/network/resource-manager/Microsoft.Network/2017-08-01/examples/UsageList.json

-            "unit": "Count",
-            "currentValue": 47,
-            "limit": 100,
+            "currentValue": 8.0,


I noticed these are now decimal values.

in usage.json -- these values are listed as int64 format and you're sending in non-integer values.

I don't think that this is OK -- if the values are coming back as non-integers, some deserializers will probably fail. Is there a service change?

"Usage": { "properties": { "unit": { "type": "string", "description": "An enum describing the unit of measurement.", "enum": [ "Count" ], "x-ms-enum": { "name": "UsageUnit", "modelAsString": true } }, "currentValue": { "type": "integer", "format": "int64", "description": "The current value of the usage." }, "limit": { "type": "integer", "format": "int64", "description": "The limit of usage." }, "name": { "$ref": "#/definitions/UsageName", "description": "The name of the type of usage." } },

we will not be sending non-integer values. sending non-integer values does not make sense for this api.

To be honest, the service uses this object Microsoft.Azure.Insights.Models.UsageMetric to populate the response and property definitions are as follows
private double _currentValue;
private string _id;
private double _limit;
private LocalizableString _name;
private string _nextResetTime;
private TimeSpan? _quotaPeriod;
private string _unit;

Even though it says double currentValue, it doesnt make sense to introduce a breaking change on our client (from int64 to double).

The trouble is, some deserializers may not properly handle deserializing values like 50.0 into an integer field, even if the value is actually a whole number. If your service is returning number values with dots in them, that may not be good for some clients.

I'm more concerned with what the service is returning...

at this point, i dont know what we can do here. you might have to take it up with the insight people. All usages implementations use that i believe.

i thikn the right thing to do would be to convert the clients to double but that would be a breaking change :( . do you think we ought to do that. does this warrant a breaking change for the client? i personally dont think we should introduce a breaking change for this...

I think we just stepped into a lose/lose situation.

If the service has been returning floating point numbers all along (even if they are whole numbers) -- that means the clients are broken already.

Which means, we're better off correcting the specification with the correct types and publishing a new version of the package, even if it means that it's a minor breaking change.

Given that the other things in this PR are corrections which may introduce minor breaking effects, my preference is that let's do the right thing and fix the spec and generate the new code.

i agree with you but the not the timing. With ignite close by, i dont want to do this now. can we do this post ignite? this would require changes in PS as well.

@fearthecowboy, i have opened an issue for this that we will get to after ignite: #1624

fearthecowboy · 2017-08-29T19:32:38Z

specification/network/resource-manager/Microsoft.Network/2017-08-01/usage.json

    },
    "Usage": {
      "properties": {
+        "id": {


This will be a breaking change to a generated class.

it is a read only string right? why is that considered a breaking change? what do you suggest we do?

this is a bug fix we are trying to do

If it's a bugfix, I'm not going to flag it-- it's about as harmless as can be, when introducing a new property into a class where it wasn't before. Likely it's not going to screw anything up.

DeepakRajendranMsft · 2017-08-29T23:24:40Z

@fearthecowboy i have replied to your comments. please take a look.

AutorestCI · 2017-08-31T19:59:47Z

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

AutorestCI · 2017-08-31T20:00:02Z

Azure/azure-sdk-for-ruby#943

AutorestCI · 2017-08-31T20:02:08Z

Azure/azure-sdk-for-python#1407

…SaaSMaster Update communication swagger to have async PUT and DELETE

Common specs fixes

d42b195

msftclas added the cla-already-signed label Aug 29, 2017

azuresdkci assigned fearthecowboy Aug 29, 2017

fearthecowboy suggested changes Aug 29, 2017

View reviewed changes

DeepakRajendranMsft approved these changes Aug 29, 2017

View reviewed changes

fearthecowboy approved these changes Aug 31, 2017

View reviewed changes

fearthecowboy merged commit 93a080d into Azure:current Aug 31, 2017

AutorestCI mentioned this pull request Aug 31, 2017

Automatic PR from RestAPI-PR1603 Azure/azure-sdk-for-ruby#943

Closed

AutorestCI mentioned this pull request Aug 31, 2017

[Network] Random fixes Azure/azure-sdk-for-python#1407

Closed

marstr mentioned this pull request Jan 30, 2018

[NRP] Several properties notated as int returning floating point numbers #2366

Closed

EvgenyAgafonchikov deleted the common-fixes branch May 15, 2019 07:20

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

Merge pull request #1603 from jlautman/jelautmaCommunicationAsyncOnRP…

2bef35a

…SaaSMaster Update communication swagger to have async PUT and DELETE

Conversation

EvgenyAgafonchikov commented Aug 29, 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!

azuresdkciprbot commented Aug 29, 2017

Uh oh!

fearthecowboy left a comment

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!

Choose a reason for hiding this comment

Uh oh!

DeepakRajendranMsft Aug 31, 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!

DeepakRajendranMsft commented Aug 29, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EvgenyAgafonchikov commented Aug 29, 2017 •

edited by azuresdkci

Loading

DeepakRajendranMsft Aug 31, 2017 •

edited

Loading