Native aot logic#629
Conversation
|
Getting a few oddities
var array = new JsonArray { 1, 2, 3 };apparently throws a serialization exception that int hasn't been registered on the serialization context! stack trace: @eiriktsarpalis, can you shed any light on this? Edit: We ended up working around it by using the |
jevansaks
left a comment
There was a problem hiding this comment.
I also see that RuleRegistry is using self-reflection and triggering some AOT warnings. Can you update that to a hard-coded list of the rule types? The attribute fetching logic should be fine.
| if (value.Source != null) | ||
| { | ||
| JsonSerializer.Serialize(writer, value.Source, options); | ||
| JsonSerializer.Serialize(writer, value.Source, LogicSerializerContext.Default.JsonNode); |
There was a problem hiding this comment.
I don't know the context that these JsonConverters are being used, but I think we want a pattern in the Write methods that's similar to the read -- that is, respecting the full TypeInfoResolver chain that's in the JsonSerializerOptions passed in.
By switching from "options" to the JsonSerializerContext from our module only, we prevent a caller from extending the list.
As we discussed in Slack, I think we should have a Write overload that can help validate but that still passes the options along for any nested deserialization.
| else if (node is JsonArray) | ||
| { | ||
| var data = node.Deserialize<List<Rule>>(options)!; | ||
| var data = node.Deserialize(LogicSerializerContext.Default.RuleArray)!; |
There was a problem hiding this comment.
Can we switch to options.Read here too?
| public override void Write(Utf8JsonWriter writer, LiteralRule value, JsonSerializerOptions options) | ||
| { | ||
| JsonSerializer.Serialize(writer, value.Value, options); | ||
| JsonSerializer.Serialize(writer, value.Value, LogicSerializerContext.Default.JsonNode!); |
There was a problem hiding this comment.
Same comment about wanting an options.Write overload here so we get type safety but also preserving options.
| Accumulator = accumulator | ||
| }; | ||
| #pragma warning disable IL2026, IL3050 | ||
| var item = JsonSerializer.SerializeToNode(intermediary, _options); |
There was a problem hiding this comment.
Would a '_options.WriteToNode' helper be useful for type safety in cases like this?
There was a problem hiding this comment.
Yes. Yes it would.
01ff71c to
ef5da49
Compare
a376700 to
bf45df0
Compare
bf45df0 to
e78a132
Compare
|
The only thing this needs fixed is the null deserialization issue that apparently STJ v8 fixes. |
jevansaks
left a comment
There was a problem hiding this comment.
Some comments, but non-blocking if you want to merge this in.
| rule = args is null | ||
| ? (Rule)JsonSerializer.Deserialize("[]", ruleType, options)! | ||
| : (Rule)args.Deserialize(ruleType, options)!; | ||
| ? (Rule)JsonSerializer.Deserialize("[]", ruleType, LogicSerializerContext.Default)! |
There was a problem hiding this comment.
This is technically a behavior change because we're not passing the options along.
I would recommend passing options and suppressing the warning, as unsavory as that sounds.
There was a problem hiding this comment.
I actually just probably need to use the options extension. Likely just missed this.
|
|
||
| static RuleRegistry() | ||
| { | ||
| _rules = new ConcurrentDictionary<string, Type>(new Dictionary<string, Type> |
There was a problem hiding this comment.
Didn't one of the rules have two operator aliases for it? Eyeballing this I don't see any double types in the list.
There was a problem hiding this comment.
Yeah, it's the if rule. It's there.
|
Here's a workaround for the null issue. Update Read in JsonSerializerOptionsExtensions.cs to include this slightly unsavory workaround: |
Depends on #628