fix: elide unset url and dep in hand DTOs

[claude]

Requested by mwfarb · Slack thread

Before

ArenaHandLeftJson and ArenaHandRightJson published "url": null and "dep": null on every message, whether or not the client had set them:

{"object_type":"handLeft","url":null,"dep":null}
{"object_type":"handLeft","position":{"x":1.0,"y":2.0,"z":3.0},"url":null,"dep":null}

ARENA reads a null attribute value as delete this attribute, so a hand update could clear a url or dep the client never touched. It was also the one place in Runtime/Schemas/ that broke the documented "send only changed fields" convention (Runtime/Schemas/README.md).

After

Unset members stay off the wire; populated ones are unchanged:

{"object_type":"handLeft"}
{"object_type":"handLeft","position":{"x":1.0,"y":2.0,"z":3.0}}

How

url and dep were plain public auto-properties — no [JsonProperty], no default, no guard — and Newtonsoft's default resolver serializes unattributed public members, so nothing elided them. Both now use the same idiom as the rest of the schema set (private static default + ShouldSerializeX() comparing against it), as in Runtime/Schemas/Attribute/ArenaPhysxJointJson.cs:55-63. Member names are unchanged, so ArenaLocalHand.PublishCreateUpdate() compiles as before. Two files, +34 −4.

On autogeneration: Runtime/Schemas/README.md says everything in Object/ and Attribute/ is generated from arenaxr/arena-schemas. These two hand files are the only 2 of the 78 files in Object/ that carry no per-file // CAUTION: This file is autogenerated ... header, so they look hand-maintained and this local fix should survive. If handLeft/handRight are generated upstream, the real fix belongs in the generator too — otherwise the next regeneration reintroduces the nulls. Worth a matching upstream change either way, since the same guard is what every generated DTO already emits.

Closes #184

Verification, and the merge order this needs

The Unity EditMode and pure .NET suites live on test/unit-test-suite-ci (#183), not on main, so this branch has nowhere to add a test. Both were still executed, against the real DTO sources.

1. Scratch harness (real NUnit 3.14.0, real Newtonsoft.Json 13.0.3, the two DTO sources compiled unmodified with a minimal UnityEngine shim for [Tooltip]/Debug.LogWarning). The same 7 test cases, run before and after the fix:

	before fix	after fix
HandDtos_ElideUnsetUrlAndDep (left, right)	FAIL — Expected: null / But was: <empty>	PASS
HandLeft/HandRight_StillPublishesUrlAndDepWhenSet	PASS	PASS
HandDtos_RoundTripUrlAndDepFromTheWire	PASS	PASS
PIN_HandDtos_EmitUrlAndDepAsExplicitNulls (left, right)	PASS	FAIL — url is emitted even though it is unset / Expected: not null / But was: null

2. Trial merge with #183. This branch merges into test/unit-test-suite-ci (at 8f0bcec) with no conflicts. On the merged tree:

• pure .NET suite (Tests~/DotNet/ArenaUnity.PureTests): 273 / 273 pass, unaffected;
• EditMode contract suite (Tests/Editor/ArenaSchemaContractTests.cs, run through an equivalent .NET runner): 523 pass, 2 fail.

The 2 failures are by design. #183 added HandDtos_EmitUrlAndDepAsExplicitNulls_PinsCurrentBuggyBehaviour (Tests/Editor/ArenaSchemaContractTests.cs:258) to pin this bug as current behaviour, and its own doc comment says the expectations become Is.Null once a fix lands. Nothing else in either suite is affected: the generic contract tests (Dto_EveryWireMemberHasAShouldSerializeMethod, Dto_FreshInstanceSerializesExactlyItsRequiredMembers, Dto_EmitsNoUndeclaredMembers, Dto_EveryDefaultBackingFieldHasAMatchingMember) derive their expectations by reflection and stay green, because url and dep are now attributed fields with matching guards.

Required merge order — either order works, but the second one to merge must carry the pin flip:

• If this PR merges first: test: add unit test suite and pull_request CI #183 must include the flip below before it merges, or its CI goes red on those 2 cases.
• If test: add unit test suite and pull_request CI #183 merges first: apply the flip below as part of merging this PR (or immediately after), or main goes red on those 2 cases.

The flip is one hunk in one test file, plus a stale-comment fix. Applied on the merged tree it turns 523 pass / 2 fail into 526 / 526 pass, and the flipped test still fails on pre-fix DTO sources (Expected: null / But was: <empty>) — so it is a real regression test for this bug, not a rubber stamp.

Pin-flip diff for Tests/Editor/ArenaSchemaContractTests.cs (verified on the trial merge)

@@ -56,13 +56,12 @@ namespace ArenaUnity.Tests
         /// <summary>
         /// Everything Newtonsoft will actually put on the wire for these DTOs, which is a
         /// wider set than the [JsonProperty] fields: the default contract resolver also
-        /// serializes public members that carry no attribute at all. Two conventions in
-        /// this schema set rely on that, so a test that only looked at [JsonProperty]
-        /// fields would call both of them a defect:
+        /// serializes public members that carry no attribute at all. One convention in
+        /// this schema set relies on that, so a test that only looked at [JsonProperty]
+        /// fields would call it a defect:
         ///
         /// - the `object_type` discriminator (e.g. ArenaSphereJson.object_type), a public
-        ///   readonly field with no attribute and no ShouldSerialize, emitted on purpose;
-        /// - the `url` / `dep` auto-properties on ArenaHandLeftJson and ArenaHandRightJson.
+        ///   readonly field with no attribute and no ShouldSerialize, emitted on purpose.
         ///
         /// [JsonIgnore] members (e.g. the `attributeName` markers) are excluded, matching
         /// what the resolver does.
@@ -238,34 +237,48 @@ namespace ArenaUnity.Tests
         }
 
         /// <summary>
-        /// PINS CURRENT BEHAVIOUR (bug): ArenaHandLeftJson and ArenaHandRightJson declare
-        /// `url` and `dep` as plain auto-properties
-        /// (Runtime/Schemas/Object/ArenaHandLeftJson.cs:21-22) rather than the
-        /// [JsonProperty] + ShouldSerializeX pattern every other DTO in this schema set
-        /// uses. They therefore go on the wire on every publish even when unset, as
-        /// explicit JSON nulls.
-        ///
-        /// That is not cosmetic: ARENA reads a null attribute value as "delete this
-        /// attribute", so a hand update can clear a url the client never set.
-        ///
-        /// CORRECT BEHAVIOUR is to follow the generator convention:
-        ///     [JsonProperty(PropertyName = "url")] public string url = null;
-        ///     public bool ShouldSerializeurl() { return (url != null); }
-        /// When that lands, these two expectations become Is.Null.
+        /// The wire-economy contract for the two hand DTOs, which used to be the one
+        /// exception to it: `url` and `dep` on ArenaHandLeftJson / ArenaHandRightJson were
+        /// plain auto-properties with no [JsonProperty] and no ShouldSerialize guard, so
+        /// both went on the wire on every publish, as explicit JSON nulls when unset
+        /// (arenaxr/arena-unity#184). ARENA reads a null attribute value as "delete this
+        /// attribute", so a hand update could clear a url the client never set. Both
+        /// members now follow the generator convention and are elided when unset.
         /// </summary>
         [TestCase(typeof(ArenaHandLeftJson), "handLeft")]
         [TestCase(typeof(ArenaHandRightJson), "handRight")]
-        public void HandDtos_EmitUrlAndDepAsExplicitNulls_PinsCurrentBuggyBehaviour(
-            Type type, string objectType)
+        public void HandDtos_ElideUrlAndDepWhenUnset(Type type, string objectType)
         {
             JObject emitted = JObject.Parse(JsonConvert.SerializeObject(Activator.CreateInstance(type)));
 
             Assert.That(emitted["object_type"].Value<string>(), Is.EqualTo(objectType),
                 "precondition: the discriminator is emitted");
-            Assert.That(emitted["url"], Is.Not.Null, "url is emitted even though it is unset");
-            Assert.That(emitted["url"].Type, Is.EqualTo(JTokenType.Null));
-            Assert.That(emitted["dep"], Is.Not.Null, "dep is emitted even though it is unset");
-            Assert.That(emitted["dep"].Type, Is.EqualTo(JTokenType.Null));
+            Assert.That(emitted["url"], Is.Null, "unset url must stay off the wire");
+            Assert.That(emitted["dep"], Is.Null, "unset dep must stay off the wire");
+        }
+
+        /// <summary>
+        /// The other half of the same contract: eliding the unset case must not stop a
+        /// populated url or dep from being published.
+        /// </summary>
+        [Test]
+        public void HandDtos_StillPublishUrlAndDepWhenSet()
+        {
+            JObject left = JObject.Parse(JsonConvert.SerializeObject(new ArenaHandLeftJson
+            {
+                url = "/static/models/hands/valve_index_left.gltf",
+                dep = "camera_1234_bob",
+            }));
+            JObject right = JObject.Parse(JsonConvert.SerializeObject(new ArenaHandRightJson
+            {
+                url = "/static/models/hands/valve_index_right.gltf",
+                dep = "camera_1234_bob",
+            }));
+
+            Assert.That(left["url"].Value<string>(), Is.EqualTo("/static/models/hands/valve_index_left.gltf"));
+            Assert.That(left["dep"].Value<string>(), Is.EqualTo("camera_1234_bob"));
+            Assert.That(right["url"].Value<string>(), Is.EqualTo("/static/models/hands/valve_index_right.gltf"));
+            Assert.That(right["dep"].Value<string>(), Is.EqualTo("camera_1234_bob"));
         }
 
         // ==================================================== enum wire values

Nothing was pushed to the #183 branch; the trial merge was local only.

---

Generated by Claude Code