Add Azure.Core JsonPatchDocument to DigitalTwins project to replace UpdateOperationsUtility#16129
Conversation
Since Azure.Core has not shipped this in a public Nuget yet, we will be taking a project dependency in the meantime
timtay-microsoft
requested review from
KrzysztofCwalina,
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms,
tg-msft and
vinagesh
as code owners
ghost
added
the
timtay-microsoft
changed the title
abhipsaMisra
left a comment
abhipsaMisra
left a comment
There was a problem hiding this comment.
Nice!
Did the Azure SDK board sign-off on exposing JsonPatchDocument in our public API? I had thought that we would use this as a helper, but the public API would still take in a string.
I'll bring it up in the board review on Thursday, good question |
| public virtual System.Threading.Tasks.Task<Azure.Response> UpdateDigitalTwinAsync(string digitalTwinId, string jsonPatch, Azure.DigitalTwins.Core.UpdateDigitalTwinOptions options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual Azure.Response UpdateRelationship(string digitalTwinId, string relationshipId, string jsonPatch, Azure.DigitalTwins.Core.UpdateRelationshipOptions options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual System.Threading.Tasks.Task<Azure.Response> UpdateRelationshipAsync(string digitalTwinId, string relationshipId, string jsonPatch, Azure.DigitalTwins.Core.UpdateRelationshipOptions options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual Azure.Response UpdateComponent(string digitalTwinId, string componentName, Azure.JsonPatchDocument jsonPatchDocument, Azure.DigitalTwins.Core.UpdateComponentOptions options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
There was a problem hiding this comment.
Curious we chose to require this specific data type, rather than a string that has json. My reasoning being that a customer wanted to use their own library, they can't do that now. Thoughts?
There was a problem hiding this comment.
That's a fair point. I can revert these APIs back to taking strings rather than the JsonPatchDocument itself for now. I'll add a note in our board review to talk about this further
Scratch that, the JsonPatchDocument class allows for users to set their own ObjectSerializer, so this isn't an issue. I'd still like to use this class directly in our APIs, especially if we are getting away from representing untyped json as strings
There was a problem hiding this comment.
I'd recommend leaving the way you have it for now, reach out to Krzysztof directly for direction advice, and then mention it in the board review.
There was a problem hiding this comment.
@drwill-ms just in case you signed off thinking I would revert the API shape back to taking a string. Thoughts on taking the JsonPatchDocument knowing that it takes an ObjectSerializer that allows for users to provide their favorite json library?
There was a problem hiding this comment.
Nope, I didn't sign off with that understanding. I'd have been more explicit if that were the case.
| </ItemGroup> | ||
|
|
||
| <Import Project="$(RepoRoot)\sdk\core\Azure.Core\src\Azure.Core.props" /> | ||
| <!--Uncomment the below to take reference from Azure.Core based on how Azure.Core.props defines how libraries should take reference--> |
There was a problem hiding this comment.
Can you elaborate on the problem here?
There was a problem hiding this comment.
I can add more comments to the code itself, too, but the issue here is that the Azure.Core library hasn't had a release since they added the JsonPatchDocument class. So the only way we'll get access to it is to take a project reference instead.
This all becomes moot if we don't want the JsonPatchDocument in our API shape, though
There was a problem hiding this comment.
Gotcha. Might put it in those words as the existing comment didn't make sense to me.
|
@KrzysztofCwalina any thoughts on us using the JsonPatchDocument type directly in our API vs having our API take a string, and having our samples use the JsonPatchDocument to create that string? |
…pdateOperationsUtility (Azure#16129) Since Azure.Core has not shipped this in a public Nuget yet, we will be taking a project dependency in the meantime
4 participants
Since Azure.Core has not shipped this in a public Nuget yet, we will be taking a project dependency in the meantime