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

Guard against invalid TypeToConvert args to read methods of object and collection converter #36605

Closed
layomia opened this issue May 17, 2020 · 6 comments
Labels
area-System.Text.Json bug enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented May 17, 2020

Depending on the scenario, we could fail with a NRE:

[Fact]
public static void ConverterRead_VerifyInvalidTypeToConvertFails()
{
    var options = new JsonSerializerOptions();
    Type typeToConvert = typeof(KeyValuePair<int, int>);
    byte[] bytes = Encoding.UTF8.GetBytes(@"{""Key"":1,""Value"":2}");

    JsonConverter<KeyValuePair<int, int>> converter =
        (JsonConverter<KeyValuePair<int, int>>)options.GetConverter(typeToConvert);

    // Baseline
    var reader = new Utf8JsonReader(bytes);
    reader.Read();
    KeyValuePair<int, int> kvp = converter.Read(ref reader, typeToConvert, options);
    Assert.Equal(1, kvp.Key);
    Assert.Equal(2, kvp.Value);

    // Test
    reader = new Utf8JsonReader(bytes);
    reader.Read();
    try
    {
        converter.Read(ref reader, typeof(Dictionary<string, int>), options);
    }
    catch (Exception ex)
    {
        if (!(ex is InvalidOperationException))
        {
            throw ex; // NullReferenceException thrown.
        }
    }
}

Validation should take place in this method:

public sealed override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)

The ReadStack Initialize method called downstream may also need some validation logic.

EDIT: the validation may need to take place in the individual object/collection converters.

@layomia layomia added this to the 5.0 milestone May 17, 2020
@layomia layomia self-assigned this May 17, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 17, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label May 17, 2020
@layomia layomia changed the title Guard against invalid TypeToConvert args to read methods for object and collection converter Guard against invalid TypeToConvert args to read methods of object and collection converter May 17, 2020
@layomia layomia removed their assignment Aug 7, 2020
@layomia
Copy link
Contributor Author

layomia commented Aug 27, 2020

Moving this to 6.0 since this is not a regression and will not be experienced if a compatible typeToConvert is passed to the converter.

@layomia layomia modified the milestones: 5.0.0, 6.0.0 Aug 27, 2020
@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Aug 27, 2020
@ShreyasJejurkar
Copy link
Contributor

Hi, @layomia So, in this should we throw an exception like stating that type mismatched or just return null?
In case of throwing an exception, what would be appropriate exception type should be thrown (InvalidOperationException?) and with which message?

@layomia
Copy link
Contributor Author

layomia commented Oct 13, 2020

@MCCshreyas I believe an InvalidOperationException with a message along the lines of "Type 'NameOfType' is not compatible with converter 'ConverterName'." is appropriate.

@ShreyasJejurkar
Copy link
Contributor

OK @layomia, thanks for clarification! I will get that implemented!

@eiriktsarpalis
Copy link
Member

Moving to Future, as we won't have time to work on this in the .NET 7 timeframe.

@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Sep 2, 2022
rhirano0715 added a commit to rhirano0715/dotnet_runtime that referenced this issue Dec 7, 2023
Added guard to throw InvalidOperrationException if the argument typeToConvert and Converter are incompatible.
The exception message is "Type 'NameOfType' is not compatible with converter 'ConverterName'." and "Type 'NameOfType' is not compatible with converter 'ConverterName'.

NOTES:
The test threw a NotSupportedException on the following line and could not reproduce this issue, so the JsonSerializerOptions instantiation was changed.
OptionsTests.ConverterRead_VerifyInvalidTypeToConvertFails() lineno 1432
KeyValuePair<int, int> kvp = converter.Read(ref reader, typeToConvert, options);

Fix dotnet#36605
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Dec 7, 2023
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Dec 7, 2023

Closing in favor of #50205 which describes the wider problem that is at play here. TL;DR built-in object converter cannot function independently of the correct JsonTypeInfo metadata associated with the type. This would need to be fixed whenever #63791 is implemented.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json bug enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants