[Edits] Support for `deprecatedID` and `instanceID` edit semantics #352

eyelidlessness · 2025-03-18T20:14:23Z

I have verified this PR works in these browsers (latest versions):

I'm not entirely sure how I could validate this in-browser, without either a host app integration or a simulation of one. In any case, I don't think there's anything that should be browser specific here!

What else has been done to verify that this works as intended?

Leans heavily on testing (which caught a slightly embarrassing mistake dealing with namespaces!) and to some extent types (which guided most of the deprecatedID logic).

Why is this the best possible solution? Were any other approaches considered?

Most of this is expanding on prior work (#349 and prior), so there wasn't much new to consider.

There's a big open question about a naming pattern introduced in this change. I'd like us to address it in review.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I can't think of any meaningful regression risks off the top of my head, but I might update this if any come to mind later.

Do we need any specific form for testing your changes? If so, please attach one.

N/A

What's changed

When editInstance is used to initialize instance state:

If the incoming instance XML has an instanceID metadata element, its value is populated in a deprecatedID metadata element...
- ... creating one if it doesn't already exist
- ... updating its value otherwise
  - ... forming a linked list of sequenced edits
  - ... forming a tree of concurrent edits
- ... retaining the namespace URI of its meta parent and instanceID sibling
If the form defines a preload="uid" [implied: for the instanceID metadata element], a new value is computed on load...
- ... but not for "restore" (not an implementation change, newly covered by tests)
- ... after the incoming instanceID value is captured as deprecatedID (implicitly covered by the combined set of tests covering the above)

changeset-bot · 2025-03-18T20:14:27Z

🦋 Changeset detected

Latest commit: edca125

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@getodk/xforms-engine	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

… children This splits the current `children.ts` module roughly into: - Branchy node-specific construction logic that already existed prior to #336. - Coordination of input to those node constructors introduced in #336. This logic is hopefully a little bit clearer now too. The latter module’s logic will be expanded in the next commit, to include said special case logic for instance meta children.

eyelidlessness · 2025-03-20T21:02:17Z

packages/scenario/src/assertion/extensions/submission.ts

+	assertMetaNamespaceOptions(value);
+	assertScenario(value.sourceScenario);
+};
+


Most of the above has been extracted from submission.test.ts, where Past Me was sorely lacking in foresight:

Normally this might be implemented as a custom "matcher" (assertion). But it's so specific to this sub-suite that it would be silly to sprawl it out into other parts of the codebase!

The intent of implementing these assertion extensions now is because Past Me was obviously wrong about how specific the logic was/would be to that sub-suite. With the benefit of hindsight, custom assertions are exactly how we usually avoid "sprawl[ing] out into other parts of [scenario]". They are our primary mechanism for sharing test logic across modules/suites.

There are probably going to be a bunch of new concepts to take in during review. I'm happy to answer any questions to help build context and confidence in these changes. As a starting point:

I tried my best to leverage as much from existing concepts as I could (either existing as foundations of our many other custom assertions, or existing as the logic previously shared across tests in submission.test.ts). So this is mostly just moving stuff around, even if it looks like a lot is new.

The substantive changes from submission.test.ts to here are either...

mechanical: adapting the logic to shared assertion extension APIs we've been using since the scenario package's early days; these APIs have been pretty well battle tested against the scope of many other assertion extensions, across a wide variety of tests (and providing consistency with assertions derived from JavaRosa)

semantic clarification: splitting out the original assertion logic to identify inputs and outputs more clearly, and to apply that clarity for a wider range of assertions

eyelidlessness · 2025-03-20T21:03:05Z

packages/common/src/constants/regex.ts

@@ -0,0 +1,2 @@
+export const PRELOAD_UID_PATTERN =
+	/^uuid:[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/;


This module is named generally, because I anticipate at least a few other patterns moving here.

Question:

What other patterns do you anticipate? Are those related to the Edit feature?

Not related to edit, at least that I'm aware of. I would probably have moved those now if I expected to use them right away.

A few that come to mind:

XML/XPath names

Date/time (I think you've linked others at some point as well)

There may be others that aren't bubbling up in my memory, my recall isn't exactly at its best right now!

eyelidlessness · 2025-03-20T21:13:24Z

packages/scenario/src/assertion/extensions/submission.ts

+	toHaveComputedPreloadInstanceID: new AsymmetricTypedExpectExtension(
+		assertScenario,
+		assertMetaNamespaceOptions,
+		(scenario, options): SimpleAssertionResult => {


Hopefully a helpful hint for new concepts...

Each of our assertion extensions follow a similar pattern:

Treat the assertion's inputs (e.g. arg0 and arg1 corresponding to the call site shape: expect(arg0).assertionName(arg1)) as unknown

Parse-validate each input from unknown to whatever actual type (runtime and static in tandem) is expected for the actual assertion logic

Perform assertion logic, returning:

true -> Vitest considers this a passing assertion, test proceeds

Error -> Vitest considers this a failed assertion, reports as test failure

Static types are derived from the parse-validate logic, and used to extend the static types of Vitest itself. This is the best way we've found to ensure expect(...).toWhatever(...) type checks correctly (and stays in sync with the custom assertion if/whenever it changes)

eyelidlessness · 2025-03-20T21:24:08Z

packages/xforms-engine/src/instance/children/collectChildInputs.ts

+ *   already have private constructors). Then downstream isn't even a switch
+ *   statement, it's just a lookup table.
+ */
+export interface InstanceNodeChildInput {


Naming: everything to do with "child input"s felt right at the start (as a naming convention deriving from "instance input"). In hindsight, this name and many of the others following from it are confusing!

I'm not going to delay other aspects of review for this, but I want to make sure to highlight that: I haven't addressed this, I know it's awkward, I want to change it, and I'm open to suggestions!

Ok, sounds good, thanks for the highlight

So I've put in some off-screen time letting this marinate, and I think the shape of a compelling answer is forming.

Hypothesis: what this concept wants to be named is something with an "options" suffix. This would be along the lines of the very common naming convention we use for "an object of named properties used to provide a set of multiple named arguments to thing". Here it might be named something more like DescendantNodeOptions, reflecting the (abstract) "options" parameter general across all child node types (concrete subclasses of DescendantNode).

Not immediately: playing that out to its logical conclusion would probably come to look like taking on many of the ideas outlined in this interface's JSDoc. Maybe all of them. That suggests a bunch of refactoring which would inflate the size of this PR, and burden it with changes well outside its scope.

Probably immediately: if we're generally satisfied with this PR's approach aside from naming, we can almost certainly live with an intermediate solution moving toward/preparing for the more ideal refactored solution.

So then, an intermediate solution would go something like:

Agree that the outlined approach sounds good in principle.

Revise names in this PR to reflect it as a starting point for that approach, to be pursued as priorities reasonably allow.

Expand the JSDoc here to reflect these decisions.

Add a JSDoc link from DescendantNode to this same interface as an additional reinforcement.

I think that last point would be valuable because this is all starting to solidify what I was already wanting to start doing when each of the concrete DescendantNode subclasses' signatures changed in previous edit-supporting PRs. This was all more vague in my mind in the rush to build out the feature, and these signatures have shifted a lot over several recent changes. I think we may finally be getting a clearer picture of what their stable (internal) interface should eventually be.I think now would be a great time to capture that at a place where we'd start looking to slow down the churn, next time we want to revise the same signatures.

I'm going to go ahead and start down this intermediate-solution path now. If I'm happy with where that goes, I'll push it up in a single commit, so we can reference that commit, which we can then evaluate together for whether it accomplishes the near-term goals we want for this PR and establishes a good foundation for the longer-term goals discussed above. If it doesn't scratch those itches, we can trivially drop the commit/explore other ways to improve this!

Yeah, this definitely feels like the right way to go. I've pushed a commit which looks more or less as I described above. A minor difference is that I decided to use DescendantNodeInitOptions as a (temporary) name, to disambiguate from a current interface already named DescendantNodeOptions (local to DescendantNode.ts).

After I made the changes I took a quick sample of the current node constructor signatures, and I wasn't surprised to see that the object looks almost exactly like what those nodes already take, only they're currently positional. The main exception is repeats, which take N instanceNodes rather than one optional instanceNode. It wouldn't be too big a stretch to unify the signatures on a shared options parameter now, while keeping the other improvements for a later scope.

But I'm already much happier with the naming changes, and some other improvements to code clarity along the way. So I'd also be happy to leave it pretty much as it stands now, if that's where we land in the next review pass!

eyelidlessness · 2025-03-20T21:42:33Z

packages/xforms-engine/src/instance/children/normalizeChildInputs.ts

+): StaticLeafElement => {
+	const { qualifiedName, nodeset } = parent.definition;
+	const { namespaceURI, prefix } = qualifiedName;
+	const { root } = new StaticDocument({


Note: it's a bit weird to new up a StaticDocument and immediately discard it in favor of its root. The alternatives would be:

New up a StaticElement directly, referencing parent... as its parent. But this would create a hierarchical relationship which doesn't make sense! The newly created element would not be a child of its parent.

Create a concept in "static-dom" similar to DocumentFragment. This is compelling, but I didn't want to stray into new concepts. If we did ever do this, we'll have to make some choices about how it would interop with xpath (because it wouldn't be a valid context node in strict XPath semantics).

latin-panda

I couldn't finished it today, there are couple of pieces I need to dig more to fully understand them but that'll be on Monday.

latin-panda · 2025-03-21T02:04:05Z

packages/common/src/constants/regex.ts

@@ -0,0 +1,2 @@
+export const PRELOAD_UID_PATTERN =
+	/^uuid:[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/;


Question:

What other patterns do you anticipate? Are those related to the Edit feature?

latin-panda · 2025-03-21T02:28:42Z

packages/scenario/test/instance-edit.test.ts

+ * modes; and for I/O which is exercised as a prerequisite for the "edit" subset
+ * of that suite.
+ */
+describe('Instance edit semantics', () => {


Comment:

This test suite helped me to put some pieces together from the new code! It was easy to follow

latin-panda · 2025-03-21T02:39:09Z

packages/xforms-engine/src/lib/reactivity/createInstanceValueState.ts

@@ -9,6 +9,15 @@ const isInstanceFirstLoad = (context: InstanceValueContext) => {
 	return context.rootDocument.initializationMode === 'create';


Suggestion:

When reading the new code in this file for edit feature, I was wondering about what the difference is between first load vs initial load

What about renaming isInstanceFirstLoad to isCreateInitialLoad? It'd help to understand better the code for the different modes (edit, create)

isEditInitialLoad
isCreateInitialLoad

This is actually an intentional naming dichotomy.

isInstanceFirstLoad is a reference to the ODK XForms specified odk-instance-first-load event. While we haven't yet implemented actions and events, the same event is referenced by preload="uid". In the spec, this is a cue to its ordering semantics on form/instance load. In our code, it's a cue to the spec.

The intent of this name is to express the spec semantics, both as they've been implemented and as the remaining aspects of actions/events will be introduced¹.

isEditInitialLoad is a special case, which doesn't correspond to any specified event (as its JSDoc suggests). That special case is preload="uid". This isn't actually expressed anywhere in the ODK XForms spec (as far as I know), but it's the core assumption for the second half of this PR's semantics (first: instanceID -> deprecatedID; second: new UUID -> instanceID).

The intent of this name is to express... the absence of isInstanceFirstLoad's intent. In other words, the intent is to specifically call it out as a special case, and disconnect the concept from any implication that it has a basis in spec event naming, or any parity with isInstanceFirstLoad².

Footnotes

Note: I think it's likely many of these aspects will be moved out of this module when we do implement actions/events. The naming as prep for that is still valuable because it leaves a clear breadcrumb for ordering of computations, which is an important detail we'll want to preserve, especially as the code is refactored. ↩

Other than their shared is prefix: they do have parity in being predicates, but any other parity between them is incidental. ↩

latin-panda · 2025-03-21T03:01:31Z

packages/xforms-engine/src/instance/children/collectChildInputs.ts

+ * - Update this type to be a union of those interfaces
+ * - Implement that in {@link collectChildInputs}
+ * - Update downstream construction to switch over whatever narrows the union
+ * - Bonus points: revise eeach concrete {@link DescendantNode} to use a common


Suggested change

* - Bonus points: revise eeach concrete {@link DescendantNode} to use a common

* - Bonus points: revise each concrete {@link DescendantNode} to use a common

latin-panda · 2025-03-21T03:02:42Z

packages/xforms-engine/src/instance/children/collectChildInputs.ts

+ *   already have private constructors). Then downstream isn't even a switch
+ *   statement, it's just a lookup table.
+ */
+export interface InstanceNodeChildInput {


Ok, sounds good, thanks for the highlight

latin-panda · 2025-03-21T03:14:15Z

packages/xforms-engine/src/instance/children/normalizeChildInputs.ts

+
+const isMetaParent = (parent: GeneralParentNode): parent is MetaParent => {
+	const { nodeType } = parent;
+	switch (parent.nodeType) {


Comment:

I started to get a bit lost around here and couple of pieces below. I'll need to read this file again.

Quick (maybe silly) question to double check I'm not misunderstanding this. Does the "meta" in this file refer to this section from XML? Or a different meta object ?

<meta> <instanceID/> </meta>

Yes. To be precise, the "meta" naming pattern throughout the module refers to that <meta> element subtree, including its descendants, as "metadata about an instance" broadly. E.g.

<meta> -> MetaParent (more in next paragraph)

<meta><instanceID/> (and <meta><deprecatedID/>) -> "meta child"

I guess I should add to my "names I'm not thrilled about" list: MetaParent. What I was looking for here was something referring to the <meta> element... which is a subtree, but at this point in the flow we use the name "subtree" to distinguish from "group". The existing naming is probably a mistake (or at least, probably isn't clear enough), and my reluctance to address that now (out of scope, keeping scope smaller) leads straight to the awkward naming here.

Aside(?): "meta" naming convention thoughts

Another candidate for the MetaParent name, probably the most obvious, would be MetaElement (or MetaNode). Unfortunately, those create ambiguity where the "meta" naming pattern applies also to its descendants.

Since I'm starting to ponder other names this morning, I'll start here: MetaSubroot might be more clear.

Some other options:

Accept that upstream naming is the problem and fix it now:

Rename SubtreeNode (client interface) and Subtree (internal implementation) to ModelSubtreeNode and ModelSubtree respectively

Rename MetaParent to its more natural name: MetaSubtree (which could be either a "model subtree" or a "group", doesn't matter, it's still a "subtree")

MetaParent -> MetaElement + some other naming for descendants

Aside(?): useless switch statement

I'm not sure if the confusion here began with this useless switch statement that I forgot to delete. But that sure confused me seeing it with fresh eyes! I'm removing it now. In the future, I wouldn't be surprised if eslint-plugin-unicorn/no-useless-switch-case could have caught this.

I don't know if there are other (erm) cases like this in the project. Seems unlikely, but there might be! I often try out a switch first, before starting to write an if statement like the one below (like the one below == "I know it will need to narrow cases of a union type" + "has a set of additional constraints on some narrowed subset of that union"). I just usually remember to back out of that when it doesn't seem like it'll add clarity. This is definitely an artifact of my brain being exhausted!

Note: the (recommended) orx-namespaced case passes. The unprefixed (supported default) case fails. Fixed in next commit.

…ris form’s This is a broad (correct) but hacky fix for the narrow failure in the previous commit

…estore

This prepares for testing 2+ sequenced edits of the same base instance. The tests themselves (to be committed next) exercise edit **chaining** semantics (by relating input -> output identifiers) at a high level. This change itself allows those high level tests to claim that they also exercise the lower level mechanical concern that `deprecatedID` is appended only once, and its value updated in subsequent edits.

…identifiers

eyelidlessness · 2025-03-21T18:25:01Z

packages/scenario/test/instance-edit.test.ts

+			// Typical pattern: chained sequential edits form a linked list
+			it('creates a chain of edited deprecatedID -> source instanceID references over subsequent edits', () => {
+				const { source, edit1, edit2 } = scenarios;
+
+				// Prerequisite
+				assertChained(source, edit1);
+
+				// Sanity/meaningfulness of test: edit2 is not chained from source
+				assertNotChained(source, edit2);
+
+				// Assert: edit2 is chained from edit1
+				assertChained(edit1, edit2);
+			});
+
+			// Spec design/intent: chained branched edits form a tree
+			it('creates a tree of edited deprecatedID -> source instanceID references over subsequent edits of a common ancestor instance', async () => {
+				const { source, edit1: branch1, edit2: leaf1 } = scenarios;
+
+				const branch2 = await source.proposed_editCurrentInstanceState();
+				const leaf2 = await branch2.proposed_editCurrentInstanceState();
+
+				assertChained(source, branch1);
+				assertChained(source, branch2);
+				assertChained(branch1, leaf1);
+				assertChained(branch2, leaf2);
+
+				assertNotChained(branch1, leaf2);
+				assertNotChained(branch2, leaf1);
+			});


When I first wrote up this PR, I noticed I had forgotten to test that we don't append multiple deprecatedID elements. As I went to test that, I realized that what was really lacking was this pair of spec semantics:

sequenced edits produces a linked list

concurrent edits produces a tree

I'm pretty sure this was discussed in a team meeting, around the time we started queueing up edit feature work. I hope these two tests clarify those cases!

eyelidlessness mentioned this pull request Mar 18, 2025

[Edits] Engine I/O support for editing submitted instances #349

Merged

5 tasks

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch 2 times, most recently from 13a1be0 to c0be03e Compare March 19, 2025 14:21

eyelidlessness changed the title ~~[Edits] Support for deprecatedId and instanceId edit semantics~~ [Edits] Support for deprecatedID and instanceID edit semantics Mar 19, 2025

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch from 638f156 to 4794cc8 Compare March 19, 2025 22:25

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch 2 times, most recently from 3e7244c to 87b49f7 Compare March 20, 2025 20:43

eyelidlessness commented Mar 20, 2025

View reviewed changes

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch from 87b49f7 to 4a8cadd Compare March 20, 2025 21:35

eyelidlessness marked this pull request as ready for review March 20, 2025 21:36

eyelidlessness requested a review from latin-panda March 20, 2025 21:37

eyelidlessness commented Mar 20, 2025

View reviewed changes

latin-panda reviewed Mar 21, 2025

View reviewed changes

eyelidlessness added 5 commits March 21, 2025 08:23

engine: populate deprecatedID with instanceID value on edit init

480033d

engine: recompute preload=“uid” on edit init

22f38cc

scenario: test edit transfer of instanceID value to deprecatedID

35a9aa2

Note: the (recommended) orx-namespaced case passes. The unprefixed (supported default) case fails. Fixed in next commit.

engine (fix): instance XML with no default namespace declaration inhe…

13d67b3

…ris form’s This is a broad (correct) but hacky fix for the narrow failure in the previous commit

scenario: cover recomputation of edited instanceID (preload=“uid”)

f648f26

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch from 4a8cadd to 2262b23 Compare March 21, 2025 15:48

eyelidlessness added 4 commits March 21, 2025 10:20

scenario: explicitly test edit-specific semantics as exclusive from r…

cd8212d

…estore

Changeset

bc23f85

scenario: test edit semantics of chained (sequential, tree) instance …

fc02d78

…identifiers

eyelidlessness force-pushed the edit-epic/deprecatedId-instanceId-switcheroo branch from 2262b23 to fc02d78 Compare March 21, 2025 18:16

eyelidlessness commented Mar 21, 2025

View reviewed changes

engine: edit meta naming & code comprehension improvements

edca125

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Edits] Support for `deprecatedID` and `instanceID` edit semantics #352

[Edits] Support for `deprecatedID` and `instanceID` edit semantics #352

eyelidlessness commented Mar 18, 2025 •

edited

Loading

changeset-bot bot commented Mar 18, 2025 •

edited

Loading

eyelidlessness Mar 20, 2025

eyelidlessness Mar 20, 2025

latin-panda Mar 21, 2025

eyelidlessness Mar 21, 2025

eyelidlessness Mar 20, 2025

eyelidlessness Mar 20, 2025

latin-panda Mar 21, 2025

eyelidlessness Mar 21, 2025

eyelidlessness Mar 21, 2025

eyelidlessness Mar 20, 2025

latin-panda left a comment

latin-panda Mar 21, 2025

latin-panda Mar 21, 2025

latin-panda Mar 21, 2025

eyelidlessness Mar 21, 2025

latin-panda Mar 21, 2025

latin-panda Mar 21, 2025

latin-panda Mar 21, 2025

eyelidlessness Mar 21, 2025

eyelidlessness Mar 21, 2025

		@@ -0,0 +1,2 @@
		export const PRELOAD_UID_PATTERN =
		/^uuid:[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/;

		@@ -9,6 +9,15 @@ const isInstanceFirstLoad = (context: InstanceValueContext) => {
		return context.rootDocument.initializationMode === 'create';

	* - Bonus points: revise eeach concrete {@link DescendantNode} to use a common
	* - Bonus points: revise each concrete {@link DescendantNode} to use a common

[Edits] Support for deprecatedID and instanceID edit semantics #352

Are you sure you want to change the base?

[Edits] Support for deprecatedID and instanceID edit semantics #352

Conversation

eyelidlessness commented Mar 18, 2025 • edited Loading

I have verified this PR works in these browsers (latest versions):

What else has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

What's changed

changeset-bot bot commented Mar 18, 2025 • edited Loading

🦋 Changeset detected

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

latin-panda left a 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

Footnotes

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

[Edits] Support for `deprecatedID` and `instanceID` edit semantics #352

[Edits] Support for `deprecatedID` and `instanceID` edit semantics #352

eyelidlessness commented Mar 18, 2025 •

edited

Loading

changeset-bot bot commented Mar 18, 2025 •

edited

Loading