-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Add message type resolver #42428
Add message type resolver #42428
Conversation
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Outdated
Show resolved
Hide resolved
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Outdated
Show resolved
Hide resolved
9ad87f0
to
739f60f
Compare
...anscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/JsonConverterHelper.cs
Show resolved
Hide resolved
Perf with the resolver is much better than the converter.
|
Review please 🙏 @brunolins16 @halter73 @BrennanConroy @captainsafia |
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly fine, I have an issue with the testing though
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Outdated
Show resolved
Hide resolved
...anscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/JsonConverterHelper.cs
Outdated
Show resolved
Hide resolved
""field_name"": ""A field name"" | ||
}"; | ||
|
||
AssertReadJson<HelloRequest>(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike this assert, it's basically testing that manual deserialize (with custom options) and our custom JsonParser deserialize create equal output, which doesn't really mean anything since they should be the same thing essentially. We really should be asserting value(s) on HelloRequest are set to the expected value(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It validates that our JSON functionality matches what is built into Google.Protobuf. The generated objects have an Equals method that does a deep check to validate they're equivalent.
I'll add a couple of manual asserts to sanity check that deserialization is working as expected, but I don't want to assert everything. We know that Google.Protobuf is doing the right thing and if we're equal to it then the code is good.
FieldName = "A field name" | ||
}; | ||
|
||
AssertWrittenJson(helloRequest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with these tests
|
||
foreach (var field in fields) | ||
{ | ||
mappings.Remove(field.JsonName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need add an entry to the dictionary with the JsonName
if that will be discarded here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've expanded the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
JsonConverterHelper.GetFieldType(field), | ||
name); | ||
|
||
if (isWritable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I was expecting the same as @krwq. Maybe change the parameter name to something like isSerializable
739f60f
to
302c9f9
Compare
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Show resolved
Hide resolved
…nscoding/Internal/Json/MessageTypeInfoResolver.cs Co-authored-by: Stephen Halter <[email protected]>
...oding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/MessageTypeInfoResolver.cs
Show resolved
Hide resolved
…nscoding/Internal/Json/MessageTypeInfoResolver.cs
Replaces MessageConverter with contract resolver using dotnet/runtime#63686.
cc @eiriktsarpalis @krwq