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

ArgumentNullException in Json serialization #35422

Closed
iSazonov opened this issue Apr 24, 2020 · 12 comments
Closed

ArgumentNullException in Json serialization #35422

iSazonov opened this issue Apr 24, 2020 · 12 comments

Comments

@iSazonov
Copy link
Contributor

In PowerShell repo we get an exception for null parameter:

PS > $null | ConvertTo-Json -Compress
ConvertTo-Json: Value cannot be null. (Parameter 'type')
PS > Get-Error

Exception             :
    Type       : System.ArgumentNullException
    Message    : Value cannot be null. (Parameter 'type')
    ParamName  : type
    TargetSite :
        Name          : VerifyValueAndType
        DeclaringType : System.Text.Json.JsonSerializer
        MemberType    : Method
        Module        : System.Text.Json.dll
    StackTrace :
   at System.Text.Json.JsonSerializer.VerifyValueAndType(Object value, Type type)
   at System.Text.Json.JsonSerializer.Serialize(Object value, Type inputType, JsonSerializerOptions options)
   at Microsoft.PowerShell.Commands.JsonObject.ConvertToJson2(Object objectToProcess, ConvertToJsonContext& context) in
 C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\WebCmdlet\J
sonObject.cs:line 538
   at Microsoft.PowerShell.Commands.ConvertToJsonCommand2.EndProcessing() in C:\Users\1\Documents\GitHub\iSazonov\Power
Shell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\WebCmdlet\ConvertToJsonCommand.cs:line 252
   at System.Management.Automation.Cmdlet.DoEndProcessing() in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\Syst
em.Management.Automation\engine\cmdlet.cs:line 187
   at System.Management.Automation.CommandProcessorBase.Complete() in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\s
rc\System.Management.Automation\engine\CommandProcessorBase.cs:line 590
    Source     : System.Text.Json
    HResult    : -2147467261
CategoryInfo          : NotSpecified: (:) [ConvertTo-Json], ArgumentNullException
FullyQualifiedErrorId : System.ArgumentNullException,Microsoft.PowerShell.Commands.ConvertToJsonCommand2
InvocationInfo        :
    MyCommand        : ConvertTo-Json
    ScriptLineNumber : 1
    OffsetInLine     : 9
    HistoryId        : 1
    Line             : $null | ConvertTo-Json -Compress
    PositionMessage  : At line:1 char:9
                       + $null | ConvertTo-Json -Compress
                       +         ~~~~~~~~~~~~~~~~~~~~~~~~
    InvocationName   : ConvertTo-Json
    CommandOrigin    : Internal
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1

I tried to directly reference System.Text.Json 5.0.0-preview.2.20160.6 but get the same exception.
I wonder to see JsonSerializer.VerifyValueAndType() because it was removed it in #2259

Now we moved to 5.0 Preview3 with the same error.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 24, 2020
@ghost
Copy link

ghost commented Apr 24, 2020

Tagging subscribers to this area: @jozkee
Notify danmosemsft if you want to be subscribed.

@layomia
Copy link
Contributor

layomia commented Apr 29, 2020

@iSazonov it is indeed odd that you are seeing VerifyValueAndType in the stacktrace using a preview 3 build. Can you share a simple repro of the issue? Is any caller of the serializer passing in null as an argument for the inputType to any of the Serialize method overloads?

@layomia layomia added needs more info and removed untriaged New issue has not been triaged by the area owner labels Apr 29, 2020
@layomia layomia added this to the 5.0 milestone Apr 29, 2020
@iSazonov
Copy link
Contributor Author

Today we moved to 5.0 Preview4 in PowerShell repo and I will re-check.

Is any caller of the serializer passing in null as an argument for the inputType to any of the Serialize method overloads?

Yes, we are trying to serialize null. It worked before but not in 5.0.

@iSazonov
Copy link
Contributor Author

@layomia After we update to latest .Net 5.0 preview I don't see VerifyValueAndType but exception present.

Exception             :
    Type       : System.ArgumentNullException
    Message    : Value cannot be null. (Parameter 'inputType')
    ParamName  : inputType
    TargetSite :
        Name          : Serialize
        DeclaringType : System.Text.Json.JsonSerializer
        MemberType    : Method
        Module        : System.Text.Json.dll
    StackTrace :
   at System.Text.Json.JsonSerializer.Serialize(Object value, Type inputType, JsonSerializerOptions options)

PowerShell 6.0 and 7.0 use NewtonSoft Json.Net and follow works:

> $null | ConvertTo-Json -Compress
null

But after we move to .Net 5.0 Json API we get the exception.
It is single regression we see currently (.Net team great work!).

JsonSerializer.Serialize() processes null-s well in general but does not allow null-s in its signature (it explicitly check and throw on inputType == null).

Expectation is that follow works:

JsonSerializer.Serialize(null, null, null)    // returns "null"
JsonSerializer.Serialize(null, typeof(string), null)    // returns "null"

@layomia
Copy link
Contributor

layomia commented Apr 29, 2020

It is single regression we see currently (.Net team great work!).

Glad to hear! Thanks.

Expectation is that follow works:

JsonSerializer.Serialize(null, null, null)    // returns "null"
JsonSerializer.Serialize(null, typeof(string), null)    // returns "null"

So the expectation is that when the instance to serialize is null, passing null as the input type doesn't cause the ArgumentNullException to be thrown.

Why pass a null inputType in the first place? Which scenarios require this?

@iSazonov
Copy link
Contributor Author

So the expectation is that when the instance to serialize is null, passing null as the input type doesn't cause the ArgumentNullException to be thrown.

Yes.
I look only from PowerShell side and I do not know if there are problems for other scenarios/applications.
I could do a check for null before calling this method, but my fear is that there may also be such limitations inside the method.

Why pass a null inputType in the first place? Which scenarios require this?

PowerShell scenario $variable | ConvertTo-Json -Compress that is "get an input object and convert to JSON" - so we should get the target type at runtime. This is implemented as:

JsonSerializer.Serialize(objectToProcess, objectToProcess?.GetType(), options);

Again, my request comes from only the scenario and I could create a workaround (use typeof(string) if null or address the scenario before the call as mentioned above) if MSFT team confirm that the API design is ideal.

@layomia
Copy link
Contributor

layomia commented Apr 30, 2020

if MSFT team confirm that the API design is ideal.

Yes, we consider passing a null input type invalid, and the API docs call this out.

I could do a check for null before calling this method

Doing a null check here is probably the best option, as the serializer will simply write "null" if the root object is null anyway. Looks like you're using a string-returning overload, so you could just write the null by hand.

I'll go ahead and close this issue, since it is not actionable as is. Please feel free to re-open if you still face issues.

@layomia layomia closed this as completed Apr 30, 2020
@iSazonov
Copy link
Contributor Author

@layomia Thanks! I will implement the way. I hope it will be in next PowerShell Preview and we will get more feedback.

@danmoseley
Copy link
Member

@layomia since ASP.NET had to react to that, do you think it is worth a breaking change note? so it appears in https://docs.microsoft.com/en-us/dotnet/core/compatibility/3.1-5.0#core-net-libraries

@layomia
Copy link
Contributor

layomia commented Nov 5, 2020

@danmosemsft - yes, it is a breaking change and has been shown to be significant. The change was made in #528. Will file a breaking change note.

@danmoseley
Copy link
Member

thanks!

@layomia
Copy link
Contributor

layomia commented Nov 9, 2020

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants