GeezerVison

I wonder why the fix so high level? It seems ConvertFromJson or just PopulateFromJDictionary is right place to fix.
We should think to return System.Management.Automation.Internal.AutomationNull.

Also please review JsonSerializerSettings.

Ilya (@iSazonov) ConvertFromJson() was already returning null. The problem is that TryConvertToJson() was using null to mean that JSON serialization failed. We want to return null and idealy keep what ever ConvertFromJson() was already returning. But, ConvertFromJson() can return null in situations that don't mean it was JSON null (at least I'm pretty sure it does). In that case we should let Invoke-RestMethod continue with its other serialization tests by returning false. This has to be fixed here because it's a problem with Invoke-RestMethod's logic, not with ConvertFromJson()

Hmm i realize this is probably very confusing. The an API endpoint returns the following:

HTTP/1.1 200 OK
Content-Length: 4
Content-Type: application/json
Date: Sat, 04 Nov 2017 18:49:54 GMT

null

that null is the JSON literal for $null. ConvertFromJson() properly returns $null for that, but Invoke-RestMethod was interpreting that $null as failing to serialize the object. TryConvertToJson() was returning false and Invoke-RestMethod would move on to other serialization methods until ultimately falling back to a string and returning the string null.

ConvertFromJson() returns null for empty strings too, however, in this instance, an empty or whitespace string should be returned as an an empty or whitespace string, not null, because an empty or white space string is not valid JSON. I guess it could be argued that maybe that behavior of ConvertFromJson() should be changed and that it should throw on an empty or whitespace string. But, I don't see that going over well.

In any case, because ConvertFromJson() behaves like that I'm double checking it is a true JSON null.

We have the workarround. We can remove it - the bug was fixed in Newton.Json 10.0.3. But I think we can keep JArray.Parse(input); to throw on any error in Json input string.

Ilya (@iSazonov) this has nothing to do with arrays and that code path is not hit in the instace this is addressing. This is about when null is NOT in a JSON array or object, but is solitary, by it self, a single value and the only value in the JSON string. this is literally the JSON string this PR addresses:

null

This has nothing to do with either of these which already work properly:

[
  null
]

{
  "key": null
}

Removing that workaround in ConvertFromJson() will not affect the logic in this PR.

What about JToken.Parse()? I believe it throw on null string.

And I wonder that ConvertFromJson throw if there is out Error parameter - I expect that it return an ErrorRecord.

Are you suggesting to use JToken.Parse() instead of the regex? I guess we could do that. It does throw on empty and whitespace only strings.

ConvertFromJson() does both. It throws and sets the out Error. Invalid JSON throws, trouble serializing JSON into PowerShell objects gives errors.

Yes, sorry I walk around :-), I think we should use JToken.Parse() - maybe it'll catch other mistakes we don't know yet.

As for ConvertFromJson() - I guess it is common convention - no throw if there is out Error?

As for ConvertFromJson() - I guess it is common convention - no throw if there is out Error?

I'm not sure what you are asking.

Here are the kinds of strings it throws on: {, [, [ null null]
Here is a string it errors on: { "": "value"}

Errors arise from valid JSON that cannot serialized into PowerShell objects while exceptions arise from invalid JSON. The only invalid JSON I'm aware of that doesn't throw an exception is String.Empty and a string with just whitespace. An empty string and whitespace-only string are technically not valid JSON.

In any case, when there is an error it returns null. so we can just ensure that the null it is returning is an actual null literal with JToken.Parse() In Invoke-RestMethod. It might be a bit redundant, but I will run it on a null check only. Otherwise, I'd rather trust everything else that comes from ConvertFromJson(). It's just that the existing Invoke-RestMethod logic was assuming null was always a failure, when null is a legitimate single value for JSON.

But, this does bring to light some shortcomings of ConvertFromJson(). I think you are the better person to decide how serious they are. but if we want/need to change that behavior, we need to do it before 6.0.0 as I doubt we can get away with that kind of breaking change until 7.0.0. What do you think?

I really don't like the API but we already have no time to discuss and make changes in depth. 😕

I'm sure we'll be with CoreFX implementation in 7.0. 😄 So I would document the API as obsolete or make it internal - the later allow us to migrate on CoreFX implementation in any time before 7.0.

After I see the change I'm even more confident that we should make this check in ConvertFromJson - we should remove if ((Regex.Match(input, @"^\s*\[")).Success) and replace JArray.Parse(input); with JToken.Parse(json);. So we'll be closer to JavaScriptSerializer, and we can make it easier to move on one in the future. Also we exclude the same catch on level above.

I review a history of the code and discover that I see ways to clean up and refactor the code. So I agree to merge the PR as is and ready to refactor in days.

Awesome. Just note that just like the change here, making that change ConvertFromJson() will be a breaking change since empty strings and whitespace-only strings will produce errors where as before they returned $null. I think that's appropriate, but the world at large may not. But you are right, we don't have much time. Will you handle the ConvertFromJson() refactor?

I'd refactor the code in any case because it provokes troubles.

As for a breaking change I have the thoughts - should we fix the Issue now or leave the behavior as in Windows PowerShell? Will MSFT fix the Issue in Windows PowerShell too and when? Will MSFT fix the Issue in .Net JavaScriptSerializer and when? Should we request the fix in Newton.Json as we did for JArray?
I think to get sync fixes is better then a breaking change in only PowerShell Core.

Ilya (@iSazonov) Good questions, but we should track that in a separate issue.

Perhaps, add test(s) for the error case where the JSON cannot be parsed?

Travis Plunk (@TravisEz13) if by "error" you mean empty strings or whitspace-only? I can add the tests, but they will just be ensuring a a String.Empty returns a String.Empty and that a whitespace string returns a whitespace string. When that error condition hits, the cmdlet just goes to the next serialization type (xml) and then when that fails it just returns the string.

It also looks like I would need to either add a WebListener controller to another switch option to HttpListener. Currently none of the available tests can return whitespace only responses. All of them truncate to 0 bytes when the body payload is all whitespace.

It would be good to have the test to ensure good coverage, but I think this is adequate.

I've created #5414 to add coverage for that. I have a backlog of test and cleanup activities to do, I'll start on those after RC is underway.

I would prefer to use `n for newline whitespace than rely on the line endings of the file

fixed.