update to expose contexts#645
Conversation
| /// <param name="options">An object that specifies serialization options to use.</param> | ||
| void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options); | ||
| /// <param name="typeInfo">The <see cref="JsonTypeInfo"/> for the value being serialized.</param> | ||
| void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options, JsonTypeInfo? typeInfo = null); |
There was a problem hiding this comment.
why not require it for Read too?
There was a problem hiding this comment.
Read is working. I didn't need to update it. I can look at doing that, though.
Now that we are targeting latest System.Text.Json, we can change this parameter on Refers to: JsonLogic/RuleRegistry.cs:118 in 075c705. [](commit_id = 075c705, deletion_comment = False) |
|
|
||
| _keywords = new ConcurrentDictionary<string, Type>(keywordData.ToDictionary(x => x.Item2, x => x.Item1)); | ||
| _keywordTypeInfoResolvers = new ConcurrentDictionary<Type, JsonSerializerContext>(keywordData.ToDictionary(x => x.Item1, _ => (JsonSerializerContext)JsonSchemaSerializerContext.Default)); | ||
|
|
There was a problem hiding this comment.
RuleRegistry did this differently and just returns null for known types. It seems more efficient to not populate this dictionary and just return the default context when the keyword type isn't found.
| return _keywordTypeInfoResolvers.TryGetValue(ruleType, out var context) | ||
| ? context.GetTypeInfo(ruleType)! | ||
| : JsonSchemaSerializerContext.Default.GetTypeInfo(typeof(UnrecognizedKeyword))!; | ||
| } |
There was a problem hiding this comment.
Why is this ignoring ruleType here and passing UnrecognizedKeyword explicitly?
There was a problem hiding this comment.
If the keyword isn't found, it's treated as an unrecognized keyword. This captures the data for the keyword. It doesn't play into validation and just generates and annotation.
| ); | ||
| static JsonSchemaDataSerializerContext() | ||
| { | ||
| CombinedOptions = new JsonSerializerOptions |
There was a problem hiding this comment.
Why do we need to combine options anymore? This new strategy should avoid the need for it.
There was a problem hiding this comment.
This was the workaround for the bug. I need it in the UriIdentifier class where I'm serializing a JsonSchema.
There was a problem hiding this comment.
Which bug? Can you include a comment / pointer to the issue if it's a bug in STJ?
| { | ||
| PropertyNameCaseInsensitive = true | ||
| }); | ||
| var collections = JsonSerializer.Deserialize<List<TestCollection>>(contents, _serializerOptions); |
There was a problem hiding this comment.
Similarly, this call should be able to just pass DataTestsSerializerContext.Default.ListTestCollection, right?
There was a problem hiding this comment.
I wanted to be able to set the options here.
|
closing in favor of #648 which includes these changes. |
Really just want to show the Logic lib. The rest isn't quite ready.
This showcases that we can expose the contexts, which gives the user access to the JTIs for all of the types contained in or used by the lib. We still use the options manager to handle deserializing user-registered rules, but it's kept internal. The hard part is remembering on which context your JTI exists.
Also, because the context is exposed, there's no need to have the
IJsonTypeInfoResolverstatic property.Ultimately, I see this working out for all the libs. We can also create a serializer context in Json.More that exposes common types, like JsonNode. (Really, I think .Net should expose a context for its common types... *cough* @eiriktsarpalis)
We might also be able to drop the
IJsonConverterReadWrite.