- Notifications
You must be signed in to change notification settings - Fork5.2k
[JSON source gen 3/3] Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata#51528
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedApr 19, 2021
Tagging subscribers to this area:@eiriktsarpalis,@layomia |
ghost commentedApr 19, 2021
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
b4feeff to908ba2cCompareUh oh!
There was an error while loading.Please reload this page.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.Net.Http.Json/tests/FunctionalTests/JsonContext/String.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
steveharter left a comment
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.
LGTM; some nits and a couple questions.
src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpContentJsonExtensions.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...xt.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/JsonContext/HighLowTemps.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...ext.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/JsonContext/JsonContext.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| privatestaticJsonPropertyInfo[]PersonPropInitFunc(JsonSerializerContextcontext) | ||
| { | ||
| JsonContextjsonContext=(JsonContext)context; |
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.
Can this cast ever fail?
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 should never, unless we have a bug (e.g. a weird cross-pollination of options instances being used). The serializer should only pass the same context instance that the type info instance was a property on.
I'll think about this some more. We could be more defensive in the gen'd code and throwInvalidOperationException instead of leakInvalidCastException.
To avoid any potential issues here entirely, we could make the method non-static and reference the context directly (instead of taking it as an argument), but I want to be sure of any perf implications first.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ghost commentedApr 20, 2021
Hello@layomia! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me ( |
layomia commentedApr 20, 2021
/backport to release/6.0-preview4 |
Started backporting to release/6.0-preview4:https://github.com/dotnet/runtime/actions/runs/768780136 |
@layomia backporting to release/6.0-preview4 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patchApplying: Add new methods to JsonSerializer and System.Net.Http.Json APIs that taketype metadata.git/rebase-apply/patch:2505: trailing whitespace. .git/rebase-apply/patch:2538: trailing whitespace. .git/rebase-apply/patch:2551: trailing whitespace. .git/rebase-apply/patch:2826: trailing whitespace. .git/rebase-apply/patch:2859: trailing whitespace. warning: squelched 6 whitespace errorswarning: 11 lines add whitespace errors.Using index info to reconstruct a base tree...Msrc/libraries/System.Text.Json/ref/System.Text.Json.csMsrc/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.csMsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.csMsrc/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.csAsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/DeserializationWrapper.csAsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.csAsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.csAsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/SerializationWrapper.csAsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csprojFalling back to patching base and 3-way merge...Auto-merging src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csprojCONFLICT (rename/rename): Rename"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs"->"src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs"in branch"HEAD" rename"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs"->"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.JsonMetadataServices.cs"in"Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata"Auto-merging src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.cs and src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.JsonMetadataServices.cs, both renamed from src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.csCONFLICT (rename/rename): Rename"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs"->"src/libraries/System.Text.Json/tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs"in branch"HEAD" rename"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataServicesTests/MetadataServicesTests.Options.cs"->"src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs"in"Add new methods to JsonSerializer and System.Net.Http.Json APIs that take type metadata"Auto-merging src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.csAuto-merging src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.csAuto-merging src/libraries/System.Text.Json/ref/System.Text.Json.csCONFLICT (content): Merge conflictin src/libraries/System.Text.Json/ref/System.Text.Json.cserror: Failed to mergein the changes.hint: Use'git am --show-current-patch=diff' to see the failed patchPatch failed at 0001 Add new methods to JsonSerializer and System.Net.Http.Json APIs that taketype metadataWhen you have resolved this problem, run"git am --continue".If you prefer to skip this patch, run"git am --skip" instead.To restore the original branch and stop patching, run"git am --abort".Error: The process'/usr/bin/git' failed withexit code 128 Please backport manually! |
ericstj commentedApr 20, 2021
I think the backport is failing because the 2/3 PR hasn't been merged yet |
layomia commentedApr 20, 2021
/backport to release/6.0-preview4 |
Started backporting to release/6.0-preview4:https://github.com/dotnet/runtime/actions/runs/768827649 |
tdykstra left a comment
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.
Everything in 2/3 and 3/3 LGTM, aside from one nit.
...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
danmoseley commentedApr 21, 2021
@tdykstra@carlossanlop I think it's great that docs folks are able to review the docs in the exact same PR that we have implementation and tests. That has to be so much more efficient long term. |
Uh oh!
There was an error while loading.Please reload this page.
Contributes to#45448.
PR 1 -#51149
PR 2 -#51300
PR 3 - this PR
This PR completes the initial implementation of the source generator needed for community feedback (preview 4) and for Blazor to adopt the new pattern. cc@CoffeeFlux,@eerhardt,@ericstj,@Anipik.
#51544 tracks the remaining S.N.H.J APIs we need to add.
cc@jozkee for System.Net.Http.Json changes.