Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't throw if a property is missing from DynamicJson, to allow for optional property checks #34026

Merged
merged 7 commits into from
Feb 11, 2023

Conversation

annelo-msft
Copy link
Member

In order to write code against optional properties in DynamicJson in the same way we do in our strongly-typed models, we need to be able to check the properties for null to determine whether they are present or absent in the deserialized JSON.

An example of such a check can be seen in the ConfidentialLedger HelloWorld sample.

This PR implements the change to DynamicJson to enable this feature.

Addresses #33856

@annelo-msft annelo-msft requested a review from jsquire February 9, 2023 01:05
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@jsquire
Copy link
Member

jsquire commented Feb 9, 2023

Do we have a way to ask "does this property exist"? Otherwise, I'm not sure how I'd be able to distinguish between "this property was in the JSON and the value was null for this call" versus "this property was not in the JSON and the value will always be null. I probably made a mistake and should fix it."

@annelo-msft
Copy link
Member Author

Do we have a way to ask "does this property exist"? Otherwise, I'm not sure how I'd be able to distinguish between "this property was in the JSON and the value was null for this call" versus "this property was not in the JSON and the value will always be null. I probably made a mistake and should fix it."

Yes, great question.

MutableJsonElement has a TryGetProperty() method (like JsonElement), which allows this. The challenge with DynamicJson is that we don't want to add much to the public API. This is because in earlier UX studies, people were confused about when to use the object in it's strongly-typed form vs. as a dynamic type. Minimizing the public API surface is our way of steering away from this problem -- people should only use it as dynamic.

Given this, I've been trying to use the guiding principle that the dynamic type should behave like one of our generated models (we have a fixed set of transforms from the REST API description to a model type built into the generator, so this is predictable).

You're right that the result is we can't tell the difference between the underlying JSON having the property and it being null, vs. the property being absent from the JSON. I believe this is also a current limitation of our model types, so we're maintaining parity there.

We could say that to disambiguate this, you need to convert to MutableJsonDocument or JsonDocument.

@jsquire
Copy link
Member

jsquire commented Feb 9, 2023

We could say that to disambiguate this, you need to convert to MutableJsonDocument or JsonDocument.

I've no issue with it being off the main path like this, so long as we have a way to do so. I can see that being a frequently used troubleshooting step for incoming issues.

@annelo-msft annelo-msft mentioned this pull request Feb 11, 2023
41 tasks
@annelo-msft annelo-msft enabled auto-merge (squash) February 11, 2023 00:42
@annelo-msft annelo-msft merged commit 2606ffd into Azure:main Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants