Add Try methods to JsonObject #111229

Flu · 2025-01-09T10:02:36Z

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 · 2025-01-09T10:03:49Z

@dotnet-policy-service agree

Flu · 2025-01-09T10:09:41Z

@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 · 2025-01-09T11:07:26Z

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 · 2025-01-09T12:03:29Z

@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 · 2025-01-09T13:29:36Z

@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 · 2025-01-10T02:29:18Z

@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 · 2025-01-10T14:28:36Z

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

build.cmd clr+libs -rc release

Flu · 2025-01-10T15:14:14Z

@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 · 2025-01-10T15:49:14Z

And that step is building successfully?

Flu · 2025-01-10T15:57:48Z

Yes

eiriktsarpalis · 2025-01-10T16:33:41Z

@ViktorHofer any ideas what might be missing?

ViktorHofer · 2025-01-10T16:36:54Z

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

eiriktsarpalis · 2025-01-10T16:47:51Z

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 · 2025-01-10T17:23:31Z

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 · 2025-01-11T02:52:43Z

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

Flu · 2025-01-13T13:44:21Z

@eiriktsarpalis I'm ready for a review

eiriktsarpalis · 2025-01-13T14:49:11Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs

@@ -34,6 +34,43 @@ public void Add(string propertyName, JsonNode? value)
            value?.AssignParent(this);
        }

+        /// <summary>
+        ///   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..


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 · 2025-01-13T15:05:28Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs

+                ThrowHelper.ThrowArgumentNullException(nameof(propertyName));
+            }
+
+            var success = Dictionary.TryAdd(propertyName, value, out index);


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 · 2025-01-13T18:54:50Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs

@@ -62,12 +62,16 @@ public bool TryAdd(string propertyName, JsonNode? value, out int index)
                ThrowHelper.ThrowArgumentNullException(nameof(propertyName));
            }

+#if NET10_0_OR_GREATER


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

Suggested change

#if NET10_0_OR_GREATER

#if NET9_0

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

PranavSenthilnathan · 2025-01-13T21:38:22Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs

+            var success = Dictionary.TryAdd(propertyName, value);
+            index = Dictionary.IndexOf(propertyName);


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);

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?

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?

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.

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.

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

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.

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

PranavSenthilnathan · 2025-01-13T21:42:48Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.cs

+            index = Dictionary.IndexOf(propertyName);
            return Dictionary.TryGetValue(propertyName, out jsonNode);


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

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

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;

dotnet-issue-labeler bot added the area-System.Text.Json label Jan 9, 2025

Flu marked this pull request as draft January 9, 2025 10:02

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 9, 2025

Flu added 4 commits January 10, 2025 05:54

Implement TryAdd and TryAdd with index out parameter for JsonObject

2a3106b

Add exception to docs for TryGetPropertyValue without out index

2252001

Add overload for TryGetPropertyValue with out index parameter

76c0f51

Modify TryGetPropertyValue to use its overload with the index param

6121c38

Flu force-pushed the issue-110244 branch from 706bdf2 to 6121c38 Compare January 11, 2025 02:46

Flu added 2 commits January 11, 2025 03:46

Add new functions to ref

3ded485

Add unit tests

1672a5b

Flu marked this pull request as ready for review January 11, 2025 02:51

build-analysis bot mentioned this pull request Jan 11, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

eiriktsarpalis requested review from PranavSenthilnathan and eiriktsarpalis January 13, 2025 14:46

eiriktsarpalis reviewed Jan 13, 2025

View reviewed changes

Fix build for .NET 9

1fa1a6d

eiriktsarpalis reviewed Jan 13, 2025

View reviewed changes

PranavSenthilnathan reviewed Jan 13, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jan 13, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Try methods to JsonObject #111229

Add Try methods to JsonObject #111229

Flu commented Jan 9, 2025

Flu commented Jan 9, 2025

Flu commented Jan 9, 2025

huoyaoyuan commented Jan 9, 2025

Flu commented Jan 9, 2025

eiriktsarpalis commented Jan 9, 2025

Flu commented Jan 10, 2025 •

edited

Loading

eiriktsarpalis commented Jan 10, 2025

Flu commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

Flu commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

ViktorHofer commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

PranavSenthilnathan commented Jan 10, 2025

Flu commented Jan 11, 2025

Flu commented Jan 13, 2025

eiriktsarpalis Jan 13, 2025

eiriktsarpalis Jan 13, 2025

eiriktsarpalis Jan 13, 2025

Flu Jan 13, 2025

eiriktsarpalis Jan 13, 2025

PranavSenthilnathan Jan 13, 2025

eiriktsarpalis Jan 13, 2025

Flu Jan 14, 2025

PranavSenthilnathan Jan 14, 2025

eiriktsarpalis Jan 14, 2025

Flu Jan 14, 2025

eiriktsarpalis Jan 14, 2025 •

edited

Loading

eiriktsarpalis Jan 14, 2025

PranavSenthilnathan Jan 13, 2025

Flu Jan 14, 2025

eiriktsarpalis Jan 14, 2025

	/// 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.

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

		index = Dictionary.IndexOf(propertyName);
		return Dictionary.TryGetValue(propertyName, out jsonNode);

Add Try methods to JsonObject #111229

Are you sure you want to change the base?

Add Try methods to JsonObject #111229

Conversation

Flu commented Jan 9, 2025

Flu commented Jan 9, 2025

Flu commented Jan 9, 2025

huoyaoyuan commented Jan 9, 2025

Flu commented Jan 9, 2025

eiriktsarpalis commented Jan 9, 2025

Flu commented Jan 10, 2025 • edited Loading

eiriktsarpalis commented Jan 10, 2025

Flu commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

Flu commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

ViktorHofer commented Jan 10, 2025

eiriktsarpalis commented Jan 10, 2025

PranavSenthilnathan commented Jan 10, 2025

Flu commented Jan 11, 2025

Flu commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiriktsarpalis Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flu commented Jan 10, 2025 •

edited

Loading

eiriktsarpalis Jan 14, 2025 •

edited

Loading