Autorest violations fixes in RecoveryServices.Backup #1805
Conversation
lmazuel
changed the title
|
@lmazuel Do you know why version.py has empty version string? |
|
@DheerendraRathor Yes, I'm change it back to next release version. |
| 'can_re_register': {'key': 'canReRegister', 'type': 'bool'}, | ||
| 'container_id': {'key': 'containerId', 'type': 'str'}, | ||
| 'protected_item_count': {'key': 'protectedItemCount', 'type': 'long'}, | ||
| 'dpm_agent_version': {'key': 'dpmAgentVersion', 'type': 'str'}, | ||
| 'dpm_servers': {'key': 'DPMServers', 'type': '[str]'}, | ||
| 'upgrade_available': {'key': 'UpgradeAvailable', 'type': 'bool'}, | ||
| 'dpm_servers': {'key': 'dpmServers', 'type': '[str]'}, |
There was a problem hiding this comment.
@DheerendraRathor Could you confirm this is the actual EXACT case used by your endpoint?
We have some issues with the Swagger linter right now, where people fix the Swagger when they shouldn't and I become very careful when I see a case change.
If this is not the the EXACT case (case sensitive), a new PR needs to be made to fix the Swagger.
There was a problem hiding this comment.
Service endpoints are case-insensitive, though these will be the actual cases returned by service.
Btw how deserialization is handled in msrest? Is it case-sensitive?
There was a problem hiding this comment.
Deserialization is case-sensitive in all SDK language except C#.... This is the expected behavior for a JSON return (JSON is case-sensitive).
We have trouble currently because:
- C# doesn't care about case sensitivity (that's a C# design flaw, not a feature)
- The linter screams if this is not camelCase, mixing-up promoting good behavior (when you still can change your server) and asking Swagger change. If your server is already impossible to change, the right fix is not to change the Swagger, but to tell the linter to surpress the warnings because it's too late to change the server.
TL;DR; If this is output from Azure, and if you return "DPMServers" in the actual JSON, it won't deserialize with "dpmServers".
There was a problem hiding this comment.
In this case, server will return "dpmServers". Though old SDKs will be broken then.
Regarding #1, I'll call that a feature as that is the common implementation across libraries is to be case insensitive. Since Python json library, deserializes into dict and not class instance, that creates problem in python (standard implementation).
There was a problem hiding this comment.
Maybe it's common in the C# world to consider JSON case-insensitive, but not in other language :). I insist on others, it's Python, NodeJS and more language.
Note too that it's a requirement of Swagger:
https://swagger.io/specification/#format-11
- All field names in the specification are case sensitive.
There was a problem hiding this comment.
So other than this, PR looks good to me for merge. Tests are failing which I'll update in separate PR.
Regarding deserialization of JSON in rest api - I'm speaking with my experience on Newtonsoft, Golang std JSON unmarshalling and Python framework Django-Rest-Framework. They all have fallback from case-sensitivity to case-insensitivity if there is no exact case match.
There was a problem hiding this comment.
@DheerendraRathor I can't merge this PR with the broken tests, this would break my "master" CI. I gave you permissions to push in that branch:
https://github.com/AutorestCI/azure-sdk-for-python/invitations
You can also work from your fork and cherry-pick the generation from here, at your convenience.
Codecov Report
@@ Coverage Diff @@
## master #1805 +/- ##
==========================================
+ Coverage 52.8% 52.84% +0.04%
==========================================
Files 4577 4663 +86
Lines 112520 113888 +1368
==========================================
+ Hits 59412 60184 +772
- Misses 53108 53704 +596
Continue to review full report at Codecov.
|
|
@lmazuel Updated tests! |
|
@DheerendraRathor Awesome!
|
|
@lmazuel Sorry for late update, I'll come up with proper changelog soon after discussing it with manager and PMs. |
lmazuel
changed the base branch from
master
to
restapi_auto_recoveryservicesbackup/resource-manager
lmazuel
merged commit a20ec9b
into
Azure:restapi_auto_recoveryservicesbackup/resource-manager
|
Merge into RP branch #2033 |
* Generated from ee4507d * Update version.py * Updated tests and recordings for recoveryservices.backup
* Generated from ee4507d * Update version.py * Updated tests and recordings for recoveryservices.backup
Generated from RestAPI PR: Azure/azure-rest-api-specs#2208