Add Try methods to JsonObject

[Flu]

As per the API proposed in #110244, the following functions have been added to JsonObject:

public bool TryAdd(string propertyName, JsonNode? value);
public bool TryAdd(string propertyName, JsonNode? value, out int index);
public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode, out int index);

This resolves #110244

[Flu]

@dotnet-policy-service agree

[Flu]

@eiriktsarpalis Sorry to trouble you, I would have written some tests, but at the moment I seem not to be able to run any. I followed the testing guide here: docs/workflow/testing/libraries/testing.md

Neither dotnet build /t:Test nor running the test inside Visual Studio works, it complains about missing the SDK for .net 10. Not sure how to navigate this problem, since the guide says that if you build the clr and the libs with build.cmd -subset clr+libs -rc Release, then it should work. The build itself finishes succesfully, but trying to run the tests fails. Any idea why? Also, prior to trying to run dotnet build /t:Test, I did cd into runtime\src\libraries\System.Text.Json\tests.

[huoyaoyuan]

it complains about missing the SDK for .net 10

Don't use the global dotnet command, but the dotnet.cmd at repository root instead. It will resolves to a local copy of nightly SDK.

[Flu]

@huoyaoyuan then how can I debug in Visual Studio if I need to use dotnet.cmd?

Also, for example, if I needed to execute the tests in ./src/libraries/System.Text.Json/tests/System.Text.Json.Tests/, how would I go about doing that? I tried doing .\dotnet.cmd build /t:Test /p:XUnitOptions="-class System.Text.Json.Tests.JsonNode.ParseTests" but it seems to be executing other tests that are not there, so not sure what the proper usage would be here.

[eiriktsarpalis]

@Flu You can use the -vs flag in build.cmd to launch a solution using the bootstrapping sdk in the environment:

build.cmd -vs src\libraries\System.Text.Json\System.Text.Json.sln

That should make it possible to run tests from the test explorer as normal.

[Flu]

@eiriktsarpalis I am now getting this error when trying to run the tests from inside visual studio after I've used your command:

MSB4018: The "Microsoft.DotNet.ApiCompat.Task.ValidateAssembliesTask" task failed unexpectedly.
System.MissingMethodException: Method not found: 'System.ReadOnlySpan`1<!0> System.Collections.Immutable.ImmutableArray`1.AsSpan()'

This has nothing to do with my changes, as it's happening on the main branch as well, most recent commit.

[eiriktsarpalis]

@Flu did you try building the full runtime+libraries first?

build.cmd clr+libs -rc release

[Flu]

@eiriktsarpalis Yes. I mean, I am adding test.libs as well:

.\build.cmd -subset clr+libs+libs.tests -rc Release /p:RunApiCompat=false

This is the exact command I've been using. Removing the /p:RunApiCompat=false doesn't seem to be making a difference, tried without it too.

[eiriktsarpalis]

And that step is building successfully?

[Flu]

Yes

[eiriktsarpalis]

@ViktorHofer any ideas what might be missing?

[ViktorHofer]

Yes, it looks like APICompat is currently broken on desktop msbuild (which is used inside VS): dotnet/sdk#45004

[eiriktsarpalis]

You shouldn't need to disable ApiCompat to get your dev environment set up though. I would recommend stashing your changes, then build the repo, make sure that tests run from the IDE, and finally pop your changes back in.

[PranavSenthilnathan]

I've been having this issue as well. The workaround that I use is to run the tests from VS test explorer but when the build fails and VS shows the failure dialog, select "Yes" to run tests from last successful build. Everything (e.g. debugging, breakpoints, etc.) works as expected since the API compat step of the build (which is component that is failing) seems to be after the product and test binaries are already generated.

@huoyaoyuan then how can I debug in Visual Studio if I need to use dotnet.cmd?

Also, for example, if I needed to execute the tests in ./src/libraries/System.Text.Json/tests/System.Text.Json.Tests/, how would I go about doing that? I tried doing .\dotnet.cmd build /t:Test /p:XUnitOptions="-class System.Text.Json.Tests.JsonNode.ParseTests" but it seems to be executing other tests that are not there, so not sure what the proper usage would be here.

D:\git\runtime\src\libraries\System.Text.Json\tests\System.Text.Json.Tests> dotnet test /p:xunitoptions="-method System.Text.Json.Tests.Utf8JsonWriterTests.WriteStringValueSegment_Utf8_Split8CodePointsBasic"

You should be able to use dotnet.cmd here as suggested above if this doesn't resolve to the right version.

[Flu]

@PranavSenthilnathan Thank you, it finally worked. Clever solution!

[Flu]

@eiriktsarpalis I'm ready for a review

[eiriktsarpalis]

Suggested change

	/// Adds an element with the provided name and value to the <see cref="JsonObject"/>, if a property named <paramref name="propertyName"/> doesn't already exist..
	/// Adds an element with the provided name and value to the <see cref="JsonObject"/>, if a property named <paramref name="propertyName"/> doesn't already exist.

[eiriktsarpalis]

This is probably failing to build in .NET 9 since that version of OrderedDictionary doesn't have the overload that exposes the index. I would recommend emulating the behavior using a second lookup via IndexOf in .NET 9 targets.

[eiriktsarpalis]

We only need this for .NET 9. Earlier targets polyfill the current OrderedDictionary implementation.

Suggested change

	#if NET10_0_OR_GREATER
	#if NET9_0

[Flu]

I assume it becomes this then?

#if NET9_0
            var success = Dictionary.TryAdd(propertyName, value);
            index = Dictionary.IndexOf(propertyName);
#else
            var success = Dictionary.TryAdd(propertyName, value, out index);
#endif

[eiriktsarpalis]

Right.

[PranavSenthilnathan]

Suggested change

	var success = Dictionary.TryAdd(propertyName, value);
	index = Dictionary.IndexOf(propertyName);
	var success = Dictionary.TryAdd(propertyName, value);
	index = success ? Dictionary.Count : Dictionary.IndexOf(propertyName);

[eiriktsarpalis]

Shouldn't this be Count - 1? Also, this assumes that OD appends new entries at the end, but what if that changes in the future?

[Flu]

Yes, it should be Count - 1.
Even though I don't think it's likely for that to change (it's by definition ordered the way elements are added), the fix to not using Count - 1 is to just leave the code like it is now, right?

[PranavSenthilnathan]

Yes that should've been Count - 1.

Also, this assumes that OD appends new entries at the end, but what if that changes in the future?

Hmm, independent of however it's currently implemented, what invariant is Ordered supposed to guarantee?

An alternative is to do IndexOf first and if it doesn't exist then do d.Insert(d.Count, key, value) (note: not Count - 1) which would add at the end.

Not optimizing this case is also fine.

[eiriktsarpalis]

To be clear, I don't think that it would ever change; I'm just pointing out that it's depending on an invariant from a separate component.

what invariant is Ordered supposed to guarantee?

It's a dictionary that also implements a deterministic IList with $\mathcal O(1)$ access.

[Flu]

So what is the consensus here? Do I leave it as it is?

[eiriktsarpalis]

I'm fine to apply the optimisation provided there is test coverage, e.g. a test that verifies that the returned index is correct in that scenario.

[eiriktsarpalis]

A comment above the line explaining the arrangement might help as well.

[PranavSenthilnathan]

Similar to above, the TryGetValue can be skipped if IndexOf returns false.

[Flu]

Yes, this one can be improved:

index = Dictionary.IndexOf(propertyName);
if (index == -1)
{
    return Dictionary.TryGetValue(propertyName, out jsonNode);
}

jsonNode = null;
return false;

Something like this for the .NET 9 part

[eiriktsarpalis]

Once you have the index you don't need to perform another key-based lookup:

index = Dictionary.IndexOf(propertyName);
if (index < 0)
{
    jsonNode = null;
    return false;
}

jsonNode = Dictionary.GetAt(index);
return true;