Reflect the service nullability behavior of Error and endTime #10262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
christothes
requested review from
heaths,
jlichwa and
RandalliLama
as code owners
|
Azure Pipelines successfully started running 1 pipeline(s). |
christothes
changed the title
Azure CLI Extension Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-go - Release
|
azure-sdk-for-java - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-js - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-net - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Trenton Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
|
Can one of the admins verify this patch? |
heaths
reviewed
Jul 27, 2020
| @@ -67,7 +67,8 @@ | |||
| "$ref": "#/definitions/Error" | |||
| } | |||
| }, | |||
| "description": "The key vault server error." | |||
| "description": "The key vault server error.", | |||
| "x-nullable": true | |||
There was a problem hiding this comment.
@johanste is this correct? I thought x-nullable would be applied where this type is returned.
There was a problem hiding this comment.
Does the service actually return null here? If so, why? The property is optional. And we really don't want to have semantically meaningful null values for content-type application/json payloads.
There was a problem hiding this comment.
In my testing the service returns null for properties of the Error model, including innererror, which is of type Error.
There was a problem hiding this comment.
If the intention is to indicate that fields of the Error type can be null then you want to apply x-nullable to the applicable fields (like you did in backuprestore.json).
There was a problem hiding this comment.
The error value itself is returned as null. I think the service should clarify which of the types and properties are expected to be nullable before finalizing on the correct swagger change.
Currently, marking just the Error properties results in a null ref exception in generated code, but marking the whole Error type does not.
There was a problem hiding this comment.
Did you check with @pakrym - or even just try it with local swaggers - if setting x-nullable: true on the model itself even works as intended?
There was a problem hiding this comment.
Managed HSM usage of Error is that when there is no error in a correct case we are returning null to indicate the same.
There was a problem hiding this comment.
The service should not return a null value on the wire. You should omit the key/value pair altogether. Is there a reason that the service is sending null on the wire?
There was a problem hiding this comment.
In speaking with @vasanthrajams previously, this is consistent with existing Key Vault api behavior, so changing it now could be breaking.
vasanthrajams
approved these changes
Jul 27, 2020
heaths
approved these changes
Jul 29, 2020
jhendrixMSFT
approved these changes
Aug 10, 2020
azure-resource-manager-schemas - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python-track2 - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
maorleger
added a commit
to Azure/azure-sdk-for-js
that referenced
this pull request
Apr 21, 2021
This PR includes regenerated code from the stable 7.2 swagger. Up until now it was in preview, but now that it's stable we're regenerating (and expected no meaningful changes). But there were meaningful changes! Recently our code generator started emitting `null` based on the x-nullable spec which unfortunately expanded CertificateOperation.error and ErrorModel.innerError to be `ErrorModel | undefined | **null**` Since this data is going from server to client we can only narrow it, we can't expand it without a breaking change. This is made worse because we re-exported ErrorModel as-is in 4.1. I looked into it and realized that this is done for historical reasons and not semantically meaningful see: Azure/azure-rest-api-specs#10262 (comment). So I am deprecating `ErrorModel` since we should use the `CertificateOperationError` type anyway, ensuring that the existing ErrorModel does not change, and applying the transformation from null to undefined in the convenience layer to keep the API the same.
jay-most
pushed a commit
to jay-most/azure-sdk-for-js
that referenced
this pull request
Apr 26, 2021
This PR includes regenerated code from the stable 7.2 swagger. Up until now it was in preview, but now that it's stable we're regenerating (and expected no meaningful changes). But there were meaningful changes! Recently our code generator started emitting `null` based on the x-nullable spec which unfortunately expanded CertificateOperation.error and ErrorModel.innerError to be `ErrorModel | undefined | **null**` Since this data is going from server to client we can only narrow it, we can't expand it without a breaking change. This is made worse because we re-exported ErrorModel as-is in 4.1. I looked into it and realized that this is done for historical reasons and not semantically meaningful see: Azure/azure-rest-api-specs#10262 (comment). So I am deprecating `ErrorModel` since we should use the `CertificateOperationError` type anyway, ensuring that the existing ErrorModel does not change, and applying the transformation from null to undefined in the convenience layer to keep the API the same.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
Please follow the link to find more details on PR review process.