Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Mar 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/selfish-insects-fry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@getodk/xforms-engine': minor
---

- Edited instance `instanceID` metadata is transfered to `deprecatedID`
- On forms defining `instanceID` to be computed by `preload="uid"`, edited instance `instanceID` metadata is recomputed after its previous value is transferred to `deprecatedID`
2 changes: 2 additions & 0 deletions packages/common/src/constants/regex.ts
Original file line number Diff line number Diff line change
@@ -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}$/;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI note: I’ve extracted the regex for date time into its own file, since it’s substantial enough to stand alone rather than being combined with other regex patterns in a single file

242 changes: 242 additions & 0 deletions packages/scenario/src/assertion/extensions/submission.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { PRELOAD_UID_PATTERN } from '@getodk/common/constants/regex.ts';
import {
OPENROSA_XFORMS_NAMESPACE_URI,
XFORMS_NAMESPACE_URI,
} from '@getodk/common/constants/xmlns.ts';
import { assertUnknownArray } from '@getodk/common/lib/type-assertions/assertUnknownArray.ts';
import { assertUnknownObject } from '@getodk/common/lib/type-assertions/assertUnknownObject.ts';
import { getBlobText } from '@getodk/common/lib/web-compat/blob.ts';
import type * as CommonAssertionHelpers from '@getodk/common/test/assertions/helpers.ts';
import type { DeriveStaticVitestExpectExtension } from '@getodk/common/test/assertions/helpers.ts';
import {
ArbitraryConditionExpectExtension,
Expand All @@ -11,6 +17,7 @@ import {
} from '@getodk/common/test/assertions/helpers.ts';
import type { SimpleAssertionResult } from '@getodk/common/test/assertions/vitest/shared-extension-types.ts';
import type { AssertIs } from '@getodk/common/types/assertions/AssertIs.ts';
import type { ExpandUnion } from '@getodk/common/types/helpers.js';
import type { InstanceFile, InstancePayload, InstancePayloadType } from '@getodk/xforms-engine';
import { constants } from '@getodk/xforms-engine';
import { assert, expect } from 'vitest';
Expand Down Expand Up @@ -87,6 +94,159 @@ const getInstanceFile = (payload: AnyInstancePayload): InstanceFile => {
return file;
};

const META_NAMESPACE_URIS = [OPENROSA_XFORMS_NAMESPACE_URI, XFORMS_NAMESPACE_URI] as const;

type MetaNamespaceURI = (typeof META_NAMESPACE_URIS)[number];

type AssertEnumeratedString<T extends string> = (actual: unknown) => asserts actual is T;

/**
* @todo This is probably general enough to be exported from {@link CommonAssertionHelpers}
*/
const enumeratedStringAssertion = <T extends string>(
expected: readonly T[]
): AssertEnumeratedString<T> => {
return (actual) => {
assertString(actual);

expect(expected).toContain(actual);
};
};

const assertMetaNamespaceURI: AssertEnumeratedString<MetaNamespaceURI> =
enumeratedStringAssertion(META_NAMESPACE_URIS);

interface SerializedMetaChildValues {
readonly instanceID: string | null;
readonly deprecatedID: string | null;
}

type MetaChildLocalName = ExpandUnion<keyof SerializedMetaChildValues>;

interface SerializedMeta extends SerializedMetaChildValues {
readonly meta: Element | null;
}

type MetaElementLocalName = ExpandUnion<keyof SerializedMeta>;

/**
* @todo this is general enough to go in `common` package, and would probably
* find reuse pretty quickly. Holding off for now because it has overlap with
* several even more general tuple-length narrowing cases:
*
* - Unbounded length -> 1-ary tuple
* - Unbounded length -> parameterized N-ary tuple
* - Unbounded length -> partially bounded (min-N, max-N) tuple
* - Type guards (predicate) and `asserts` equivalents of each
*
* Each of these cases comes up frequently! I've written them at least a few
* dozen times, and always back out to more specific logic for pragmatic
* reasons. But having these generalizations would allow pretty significant
* simplification of a lot of their use cases.
*/
const findExclusiveMatch = <T>(
values: readonly T[],
predicate: (value: T) => boolean
): T | null => {
const results = values.filter(predicate);

expect(results.length).toBeLessThanOrEqual(1);

return results[0] ?? null;
};

const getMetaElement = (
parent: ParentNode | null,
namespaceURI: MetaNamespaceURI,
localName: MetaElementLocalName
): Element | null => {
if (parent == null) {
return null;
}

const children = Array.from(parent.children);

return findExclusiveMatch(children, (child) => {
return child.namespaceURI === namespaceURI && child.localName === localName;
});
};

const getMetaChildValue = (
metaElement: Element | null,
namespaceURI: MetaNamespaceURI,
localName: MetaChildLocalName
): string | null => {
const element = getMetaElement(metaElement, namespaceURI, localName);

if (element == null) {
return null;
}

expect(element.childElementCount).toBe(0);

const { textContent } = element;

assert(typeof textContent === 'string');

return textContent;
};

interface MetaNamespaceOptions {
readonly [key: string]: unknown;
readonly metaNamespaceURI: MetaNamespaceURI;
}

type AssertMetaNamespaceOptions = (value: unknown) => asserts value is MetaNamespaceOptions;

const assertMetaNamespaceOptions: AssertMetaNamespaceOptions = (value) => {
assertUnknownObject(value);
assertMetaNamespaceURI(value.metaNamespaceURI);
};

const getSerializedMeta = (scenario: Scenario, namespaceURI: MetaNamespaceURI): SerializedMeta => {
const serializedInstanceBody = scenario.proposed_serializeInstance();
/**
* Important: we intentionally omit the default namespace when serializing instance XML. We need to restore it here to reliably traverse nodes when {@link metaNamespaceURI} is {@link XFORMS_NAMESPACE_URI}.
*/
const instanceXML = `<instance xmlns="${XFORMS_NAMESPACE_URI}">${serializedInstanceBody}</instance>`;

const parser = new DOMParser();
const instanceDocument = parser.parseFromString(instanceXML, 'text/xml');
const instanceElement = instanceDocument.documentElement;
const instanceRoot = instanceElement.firstElementChild;

assert(
instanceRoot != null,
`Failed to find instance root element.\n\nActual serialized XML: ${serializedInstanceBody}\n\nActual instance DOM state: ${instanceElement.outerHTML}`
);

const meta = getMetaElement(instanceRoot, namespaceURI, 'meta');
const instanceID = getMetaChildValue(meta, namespaceURI, 'instanceID');
const deprecatedID = getMetaChildValue(meta, namespaceURI, 'deprecatedID');

return {
meta,
instanceID,
deprecatedID,
};
};

const assertPreloadUIDValue = (actual: string | null) => {
assert(actual != null, 'Expected preload uid value to be serialized');
expect(actual, 'Expected preload uid value to match pattern').toMatch(PRELOAD_UID_PATTERN);
};

interface EditedMetaOptions extends MetaNamespaceOptions {
readonly sourceScenario: Scenario;
}

type AssertEditedMetaOptions = (value: unknown) => asserts value is EditedMetaOptions;

const assertEditedMetaOptions: AssertEditedMetaOptions = (value) => {
assertMetaNamespaceOptions(value);
assertScenario(value.sourceScenario);
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export const submissionExtensions = extendExpect(expect, {
toHaveSerializedSubmissionXML: new AsymmetricTypedExpectExtension(
assertScenario,
Expand Down Expand Up @@ -152,6 +312,88 @@ export const submissionExtensions = extendExpect(expect, {
return compareSubmissionXML(actualText, expected);
}
),

toHaveComputedPreloadInstanceID: new AsymmetricTypedExpectExtension(
assertScenario,
assertMetaNamespaceOptions,
(scenario, options): SimpleAssertionResult => {
Comment on lines +316 to +319
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

try {
const meta = getSerializedMeta(scenario, options.metaNamespaceURI);

assertPreloadUIDValue(meta.instanceID);

return true;
} catch (error) {
if (error instanceof Error) {
return error;
}

// eslint-disable-next-line no-console
console.error(error);
return new Error('Unknown error');
}
}
),

toHaveEditedPreloadInstanceID: new AsymmetricTypedExpectExtension(
assertScenario,
assertEditedMetaOptions,
(editedScenario, options): SimpleAssertionResult => {
try {
const { metaNamespaceURI, sourceScenario } = options;
const sourceMeta = getSerializedMeta(sourceScenario, metaNamespaceURI);
const editedMeta = getSerializedMeta(editedScenario, metaNamespaceURI);

assertPreloadUIDValue(sourceMeta.instanceID);
assertPreloadUIDValue(editedMeta.instanceID);

expect(
editedMeta.instanceID,
'Expected preloaded instanceID metadata to be recomputed on edit'
).not.toBe(sourceMeta.instanceID);

return true;
} catch (error) {
if (error instanceof Error) {
return error;
}

// eslint-disable-next-line no-console
console.error(error);
return new Error('Unknown error');
}
}
),

toHaveDeprecatedIDFromSource: new AsymmetricTypedExpectExtension(
assertScenario,
assertEditedMetaOptions,
(editedScenario, options): SimpleAssertionResult => {
try {
const { metaNamespaceURI, sourceScenario } = options;
const sourceMeta = getSerializedMeta(sourceScenario, metaNamespaceURI);
const editedMeta = getSerializedMeta(editedScenario, metaNamespaceURI);

assertPreloadUIDValue(sourceMeta.instanceID);
assertPreloadUIDValue(editedMeta.deprecatedID);

expect(
editedMeta.deprecatedID,
'Expected edited deprecatedID metadata to be assigned from source instanceID'
).toBe(sourceMeta.instanceID);

return true;
} catch (error) {
if (error instanceof Error) {
return error;
}

// eslint-disable-next-line no-console
console.error(error);
return new Error('Unknown error');
}
}
),
});

type SubmissionExtensions = typeof submissionExtensions;
Expand Down
Loading
Loading