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

Fix test failures and proper fix this time #18

Merged

Conversation

krwq
Copy link
Owner

@krwq krwq commented Jun 22, 2022

No description provided.

@@ -31,6 +31,7 @@ public static partial class JsonSerializer
var reader = new Utf8JsonReader(utf8Json, isFinalBlock: true, readerState);

ReadStack state = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could the verification logic live inside EnsureConfigured? Or could this break other places where configuration is happening?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure this is safe, I think yes but currently this check is not matching all callsites

@@ -728,6 +722,15 @@ internal void VerifyMutable()
}
}

internal void VerifyTypeInfoResolverIsSet()
{
IJsonTypeInfoResolver? resolver = _effectiveJsonTypeInfoResolver ?? _typeInfoResolver;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think we should instead simply check if the options instance is mutable. What should happen if for example we have:

var options = new JsonSerializerOptions();
var resolver = new DefaultJsonTypeInfoResolver();

JsonTypeInfo jti = resolver.GetTypeInfo(typeof(MyEnum), options); 

options.Resolver = resolver;
options.Converter.Add(new JsonStringEnumConverter());

// jti now has inconsistent configuration relative to its encapsulated options instance

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the more I think about it, I think we should make it impossible for JsonTypeInfo to encapsulate mutable options. In other words,

JsonTypeInfo.CreateJsonTypeInfo(typeof(int), new JsonSerializerOptions()); // InvalidOperationException

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we make that check there, we remove the need to do it in the (hot path) source gen serialization methods.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the place where we've originally hit this had locked options

Copy link
Collaborator

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reinstate the ActiveIssue removal. We don't want to have CI issues in the snapped p6 branch.

JsonSerializerOptions options = new JsonSerializerOptions
{
DefaultBufferSize = 1,
TypeInfoResolver = new DefaultJsonTypeInfoResolver()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are being used both in reflection and source gen contexts. By hardcoding the resolver to the reflection converter this is effectively not testing source generation. It should be reverted.

@eiriktsarpalis eiriktsarpalis merged commit 07aede0 into contract-customization Jun 22, 2022
@eiriktsarpalis eiriktsarpalis deleted the contract-customization-fix-test-failures branch June 22, 2022 20:22
krwq added a commit that referenced this pull request Jun 27, 2022
* Initial implementation for contract customization

fix build errors

Move converter rooting to DefaultJsonTypeInfoResolver so that it can be used standalone

Fix ConfigurationList.IsReadOnly

Minor refactorings (#1)

* Makes the following changes:

* Move singleton initialization for DefaultTypeInfoResolver behind a static property.
* Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field.
* Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

* remove testing of removed field

Simplify the JsonTypeInfo.CreateObject implemenetation (#2)

* Simplify the JsonTypeInfo.CreateObject implemenetation

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>

Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>

Tests and fixes for JsonTypeInfoKind.None

TypeInfo type mismatch tests

Allow setting NumberHandling on JsonTypeInfoKind.None

test resolver returning wrong type of options

JsonTypeInfo/JsonPropertyInfo mutability tests

rename test file

Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver (#3)

* Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver

* address feedback

Add simple test for using JsonTypeInfo<T> with APIs directly taking it

fix and tests for untyped/typed CreateObject

uncomment test cases, remove todo

More tests and tiny fixes

Add a JsonTypeInfoResolver.Combine test for JsonSerializerContext (#4)

* Fix JsonTypeInfoResolver.Combine for JsonSerializerContext

* Break up failing test

Fix simple scenarios for combining contexts (#6)

* Fix simple scenarios for combining contexts

* feedback

JsonSerializerContext combine test with different camel casing

Remove unneeded virtual calls & branching when accessing Get & Set delegates (#7)

JsonPropertyInfo tests everything minus ShouldSerialize & NumberHandling

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

throw InvalidOperationException rather than ArgumentNullException for source gen when PropertyInfo.Name is assigned through JsonPropertyInfoValues

tests for duplicated property names and JsonPropertyInfo.NumberHandling

Add tests for NumberHandling and failing tests for ShouldSerialize

disable the failing test and add extra checks

disable remainder of the failing ShouldSerialize tests, fix working one

Fix ShouldSerialize and IgnoreCondition interop

Add failing tests for CreateObject + parametrized constructors

Fix CreateObject support for JsonConstructor types (#10)

* Fix CreateObject support for JsonConstructor types

* address feedback

Make contexts more combinator friendly (#9)

* Make contexts more combinator friendly

* remove converter cache

* redesign test to account for JsonConstructorAttribute

* Combine unit tests

* address feedback

* Add acceptance tests for DataContract attributes & Specified pattern (#11)

* Add private field serialization acceptance test (#13)

* tests, PR feedback (#14)

* PR feedback and extra tests

* Shorten class name, remove incorrect check (not true for polimorphic cases)

* Make parameter matching for custom properties map property Name with parameter (#16)

* Test static initialization with JsonTypeInfo (#17)

* Fix test failures and proper fix this time (#18)

* Fix test failures and proper fix this time

* reinstate ActiveIssueAttribute

* PR feedback and adjust couple of tests which don't set TypeInfoResolver

* fix IAsyncEnumerable tests

* Lock JsonSerializerOptions in JsonTypeInfo.EnsureConfigured()

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants