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

Do not prematurely set Converter for Reader/Writer if CanRead/CanWrite is false #2692

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

forteddyt
Copy link

Issue:

Overlapping converters are overridden, even when one of the converters isn't applicable (via CanRead/CanWrite).

Real World Scenario:

My team has a custom JsonConverter that gets applied dynamically to properties via a ContractResolver, based on which api version the request/response is and if the property is "nullable" (as defined by Microsoft's Azure Resource Manager: x-nullable). This converter acts as a validator of sorts, returning 4xx when an explicit null ("foo": null) is present on a non-nullable property. As such, this converter should have CanWrite set to false. However, other converters - namely a StringEnumConverter derivative we use - no longer get applied to properties who have this custom JsonConverter dynamically applied to them.

Root Cause:

The issue is that the JsonSerializerInternalReader and JsonSerializerInternalWriter converter-choosing-logic takes the first non-null converter, without checking if that converter CanRead or CanWrite (respectively). If a non-null converter isn't applicable to that Reader or Writer, then it shouldn't be taken.

Workaround:

My team isn't completely blocked without this PR. A workaround that works for us is to leave the customer converter's CanWrite to true but then invoke the serializer's Serialize function:

// Base JsonConverter has CanWrite = true
// public override bool CanWrite => true;

public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
    serializer.Serialize(writer, value);
}

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.

None yet

1 participant