Throw on unsupported types in src gen #58775

steveharter · 2021-09-07T23:37:51Z

Addresses #58221 for main. This PR is expected to be ported to 6.0.

Note that there is a single public API method addition for interop between generated code and STJ. This API is not expected to be controversial since it is not used by consumers and is consistent with other interop methods in the same location. It will be covered by @layomia in the next API review.

Also addresses:

Besides IAsyncEnumerable, we also throw on other known invalid types (that the serializer also throws on) including DateOnly, TimeOnly IntPtr, UIntPtr, Type, and SerializationInfo.
Fixes a JsonPath issue with the source generator where the "root" path of "$" was always returned during serialization when configured with a metadata-based context. This code is in the STJ assembly, not the STJ.SourceGeneration assembly.

The existing "disallowed" tests that only ran on the serializer were converted to also run on source gen. The tests were expanded to:

Add IAsyncEnumerable for source-gen tests.
Ensure any user-provided custom converters for these unsupported types will be called and not throw the default NotSupportedException.
Verify Nullable<T> semantics are correct for the unsupported types that are value types.

The exception thrown is the same as what the serializer throws. E.g.:

System.NotSupportedException: 'Serialization and deserialization of 'System.Collections.Generic.IAsyncEnumerable`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]' instances are not supported. The unsupported member type is located on type 'System.Collections.Generic.IAsyncEnumerable`1[System.Int32]'. Path: $.Values.'```

ghost · 2021-09-07T23:37:56Z

Note regarding the new-api-needs-documentation label:

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.

ghost · 2021-09-07T23:50:24Z

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses #58221 for main. This PR is expected to be ported to 6.0.

Note that there is a single public API method addition for interop between generated code and STJ. This API is not expected to be controversial since it is not used by consumers and is consistent with other interop methods in the same location. It will be covered by @layomia in the next API review.

Also addresses:

Other known invalid types (that the serializer knows about) including DateOnly, TimeOnly IntPtr, UIntPtr, Type, and SerializationInfo.
Fixes a JsonPath issue with the source generator where the "root" path of "$" was always returned during serialization when configured with a metadata-based context. This code is in the STJ assembly, not the STJ.SourceGeneration assembly.

The existing "disallowed" tests that only ran on the serializer were converted to also run on source gen. The tests were expanded to:

Add IAsyncEnumerable for source-gen tests.
Ensure any user-provided custom converters for these unsupported types will be called and not throw the default NotSupportedException.
Verify Nullable<T> semantics are correct for the unsupported types that are value types.

The exception thrown is the same as what the serializer throws. E.g.:

System.NotSupportedException: 'Serialization and deserialization of 'System.Collections.Generic.IAsyncEnumerable`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]' instances are not supported and should be avoided since they can lead to security issues. The unsupported member type is located on type 'System.Collections.Generic.IAsyncEnumerable`1[System.Int32]'. Path: $.Values.'```

<table>
  <tr>
    <th align="left">Author:</th>
    <td>steveharter</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-System.Text.Json`, `new-api-needs-documentation`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

layomia

LGTM with minor questions

src/libraries/System.Text.Json/ref/System.Text.Json.cs

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

layomia · 2021-09-07T23:55:32Z

...t.Json/src/System/Text/Json/Serialization/Converters/Value/DisallowedTypeConverterFactory.cs

        {
-            // If a value type is added, also add a test that
-            // shows NSE is thrown when Nullable<T> is (de)serialized.
+            // If a type is added, also add to the souce-gen project.


I think we should update the exception message since not all these types have security issues & could be supported in the future.

BTW, that message is maybe weasel words? eg "since they can lead to security issues" --> "because it is insecure"

Agree with Layomi we should remove references to security from the exception message, since this class has evolved into a general-purpose converter for unsupported types. We might consider special casing the error message for System.Type and SerializationInfo (e.g. by injecting a reason string to the constructed converter) but this is likely overkill.

Removing the last section works for me:

from

"Serialization and deserialization of '{0}' instances are not supported and should be avoided since they can lead to security issues."

to

"Serialization and deserialization of '{0}' instances are not supported."

We might consider special casing the error message for System.Type and SerializationInfo (e.g. by injecting a reason string to the constructed converter) but this is likely overkill.

FWIW there was a discussion on how to scale these exception messages (#42605 (comment), #42605 (comment)). Another approach to consider in the future is to add a JsonTypeDisallowReason enum which is passed to a new converter ctor overload, which would determine what exception message is shown.

add a JsonTypeDisallowReason enum

Unless there is a requirement for the "security issue" in the message, I think this is overkill. It would also require 2 converter instances and add code\tests to separate the unsupported types into "security" and "non security".

cc @GrabYourPitchforks

src/libraries/System.Text.Json/tests/Common/UnsupportedTypesTests.cs

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

eiriktsarpalis · 2021-09-08T13:40:37Z

src/libraries/System.Text.Json/tests/Common/UnsupportedTypesTests.cs

+        }
+
+        [Fact]
+        public async Task CompileTimeConverterIsSupported()


Would it be possible to also cover the IAsyncEnumerable case in this test? Ditto for runtime converter case.

I'll add a test to ensure the converter gets called and doesn't throw NSE, but I think it's outside the scope here to verify actual IAsyncEnumerable semantics of serializer\deserialize; not sure if you wanted that as well.

steveharter · 2021-09-08T17:59:30Z

@layomia please merge when API approved and green.

eiriktsarpalis · 2021-09-10T13:28:29Z

@layomia should be ready to merge, can you take another look?

layomia

LGTM, merging to avoid conflicts with other PRs. Offline API review still pending.

eiriktsarpalis · 2021-09-11T11:45:10Z

/backport to release/6.0

github-actions · 2021-09-11T11:45:20Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1224251530

Throw on unsupported types in src gen

7f0369a

steveharter requested review from eiriktsarpalis and layomia as code owners September 7, 2021 23:37

ghost added area-System.Text.Json new-api-needs-documentation labels Sep 7, 2021

steveharter self-assigned this Sep 7, 2021

steveharter added this to the 6.0.0 milestone Sep 7, 2021

layomia approved these changes Sep 8, 2021

View reviewed changes

eiriktsarpalis reviewed Sep 8, 2021

View reviewed changes

eiriktsarpalis approved these changes Sep 8, 2021

View reviewed changes

steveharter added 3 commits September 8, 2021 11:02

Misc non-functional changes and feedback

02d83be

Fix typo

5639e23

Change disallowed terminology to unsupported

ac44525

eiriktsarpalis requested a review from layomia September 10, 2021 13:28

layomia approved these changes Sep 10, 2021

View reviewed changes

layomia merged commit c8c044f into dotnet:main Sep 10, 2021

github-actions bot mentioned this pull request Sep 11, 2021

[release/6.0] Throw on unsupported types in src gen #58983

Merged

steveharter deleted the ThrowOnIAsyncEnumerable branch September 13, 2021 14:28

eiriktsarpalis mentioned this pull request Sep 15, 2021

Add DateOnly and TimeOnly string body parameter support #59118

Closed

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Throw on unsupported types in src gen #58775

Throw on unsupported types in src gen #58775

Uh oh!

Conversation

steveharter commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 7, 2021

Uh oh!

ghost commented Sep 7, 2021

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter commented Sep 8, 2021

Uh oh!

eiriktsarpalis commented Sep 10, 2021

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

eiriktsarpalis commented Sep 11, 2021

Uh oh!

github-actions bot commented Sep 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

steveharter commented Sep 7, 2021 •

edited

Loading