Azure-rest-api-specs: [NRP] Several properties notated as `int` returning floating point numbers

[marstr]

This issue originally stems from PR #2271, where @nathantypanski fixed an issue where he observed floating point numbers being returned where an int was expected. Obviously, this breaks several SDKs, including our Go SDK. Commit https://github.com/Azure/azure-rest-api-specs/pull/2271/commits/92464eceefb5f32a04cf8466533df058a5f9583f fixed the instances he had run into.

Whether this needs to be fixed on the server or in the REST API Spec is unclear to me at this time.

[marstr]

@DeepakRajendranMsft would you mind helping me triage this bug?

[marstr]

@nathantypanski, have you experienced this behavior in more than one API Version? I see the original fix was targeted at the 2017-11-01 API Version.

[nathantypanski]

Sorry, I did not intend to target just 2017-11-01. That did not include my (rebased out of the PR) changes from before I tried to make a new API version.

What I noticed was by looking at the examples of API responses:

1. specification/network/resource-manager/Microsoft.Network/stable/2017-06-01/examples/UsageList.json shows integer values as responses.
2. The very next version, specification/network/resource-manager/Microsoft.Network/stable/2017-08-01/examples/UsageList.json, and all following versions, contain floating-point responses.

Basically, 2017-08-01 and all future versions are broken.

As discussed here, the server never actually replies with "non-integer values". It replies with "integer values represented by floating point JSON numbers." I think the ideal fix would be to coerce the server to represent its usage responses in integer format, for all API versions. Then you don't break any SDK versions, and you don't have to change the type output from the network usage API into float (which doesn't make much sense - you'd probably have to change it back to integers again once the network server starts returning properly formatted integer values). Of course, only the spec is public, so my pull request was an attempt to make the spec reflect "reality".

Does that make sense? Hopefully it points you in the right direction for how to fix this.

[marstr]

Thanks @nathantypanski, that is very illuminating. That makes this issue effectively a duplicate of #1624. However, I'm tempted to close #1624 in favor of this one because we aren't prescribe a breaking change for all clients.

@DeepakRajendranMsft, is there a reason we would be unable to modify the server to start returning these numbers without being in floating-point representation? I struggle to imagine that there is a floating point parser that couldn't interpret 12, but clearly an int parser will break when trying to deal with 12.0. What don't I see here?

edit: grammar

[marstr]

Okay, digging through the conversation in #1603, @fearthecowboy and @DeepakRajendranMsft have an important conversation that implies that @DeepakRajendranMsft doesn't have the ability to control the service's behavior in this instance. They conclude that the best course of action is to submit breaking changes to all of the SDKs.

[marstr]

After digging around a little internally, I have decided that the Networking team does have the ability to intercept the Insights data and cast it to the appropriate type before it hits the wire.

However, before I ask the service team to make a change, I'm going to ask around to folks on the ARM team what the expected behavior is of the "Usage" operations. This has been a problem for months, so while I'd like to get it dealt with expediently, I also don't feel the pressure to rush into breaking changes.

[EvgenyAgafonchikov]

@marstr, I've performed some investigations, please find the results below:

General:

1. Server responds with array of (used ARM client to check):

{
    "currentValue": 0.0,
    "limit": 1.0,
...
}

And here we have non-integer values

1. No real specs change was in this scope between 2015-06-15 and 2018-04-01, so here we cannot state that it was broken on some API version. It looks like it was implemented like so from the very beginning. Please use similar command to check: azure-rest-api-specs\specification\network\resource-manager\Microsoft.Network\stable>git diff --no-index 2015-06-15\usage.json 2018-04-01\usage.json

2. There was change in example examples\UsageList.json to correspond real server response and that has no influence on the resulting SDK

SDK

In case we need to update this in specs to "number:double", this would likely be breaking change for SDKs (long replaced with double) and we need to confirm this with code owners.
Adding @MikhailTryakhov as Network API specs code owner to review the above.

PS

Also, I've checked what happens in Azure PowerShell if we update these values to double, and it still works. Likely this is because there is double to long conversion/mapping on some stage in PS (these values are of long type in the corresponding PS model). So it look like there should be no breaking change for Azure PS. @cormacpayne as FYI.

@MikhailTryakhov, could you please help with reaching other clients and SDKs owners to continue discussion of the change suggested. There could be issues with conversion unlike of Azure PS.

[jhendrixMSFT]

Is there any update here? We've had another bug report against this in the Go SDK (https://github.com/Azure/azure-sdk-for-go/issues/2340). If you don't want to make this breaking change in existing API versions can you at least fix it in the next one?

[mblaschke]

For us (Daimler) it's one of our blocker as this prevents the use of some APIs. Right now it's a litle bit disappointed that these APIs are not usable only because of a wrong definition :worried:

Will this be fixed at least this year or do we have to fork the official API and fix it there?
Please give us a estimation when this bug will be fixed.

[marstr]

I'm reaching out internally again to restart this conversation, I'll try to get an answer to you ASAP @mblaschke.

[marstr]

I'm still working on routing this internally, @mblaschke. I'll report status again tomorrow, regardless of whether or not things have changed.

[mblaschke]

@marstr any progress?

[marstr]

Sorry I didn't report back when I said I would, @mblaschke!

@MikhailTryakhov is driving the resolution of this issue from the Networking team. I'll hand this thread off to him. @MikhailTryakhov, have any news on this?

[mblaschke]

@MikhailTryakhov any news? Can we expect a solution this year?

[MikhailTryakhov]

@mblaschke it's fixed, waiting for deployment. Will be a part of NRP 110 or NRP 111

[marstr]

@MikhailTryakhov, is this deployed?

[MikhailTryakhov]

@marstr thanks for reminding
correct, it's deployed in PROD, can we give a try?