Skip to content

fix(web_core): reject writes through a primitive value consistently #2499

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.

Sign up for GitHub

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

Merged
gspencergoog merged 4 commits into from
Sep 4, 2026
+40 −1
Merged

fix(web_core): reject writes through a primitive value consistently #2499

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4802696
fix(dart): reject writes through a primitive instead of dropping them
diegolopezrm Sep 3, 2026
2034591
fix(web_core): reject writes through a primitive root, keeping the root
diegolopezrm Sep 3, 2026
b024f77
Merge remote-tracking branch 'upstream/main' into fix-datamodel-primi…
diegolopezrm Sep 3, 2026
1da404c
Merge branch 'main' into fix-datamodel-primitive-writes
diegolopezrm Sep 4, 2026
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
14 changes: 14 additions & 0 deletions dart/a2ui_core/test/data_model_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,20 @@ void main() {
expect(model.get('/'), isEmpty);
});

test('rejects writes through a primitive value', () {

@gspencergoog gspencergoog Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a Dart test, but the rest of the code changes are web_core changes. Did you mean to separate it like that?

@diegolopezrm diegolopezrm Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair question — it did not start that way, and the split is a leftover from a rebase.

The PR originally fixed the same bug in both: Dart's DataModel.set walked to the container for the last segment, found neither a map nor a list, and returned normally having written nothing, so the agent got no error and the value simply was not there. web_core had two variants of the same hole — a raw TypeError on a primitive root, and a root holding false, 0 or '' being silently discarded, because the guard was if (!this.data).

Then #2439 landed with the same Dart fix, at the same branch, raising the same A2uiDataError. I dropped my version of that hunk and kept its wording, which left what you are looking at: the web_core fix, plus the Dart tests, which now cover #2439's fix rather than my own. They assert the error type rather than the message, so they passed against that wording unmodified — and the case they pin, a write when the root itself is a primitive, has no test on main.

So the Dart test is deliberate but not original intent. Happy to split it into its own PR if you would rather review them separately; it is two tests and it moves cleanly.

@gspencergoog gspencergoog Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, as long as it is adding to the Dart test corpus, and not a mistake, then this is fine to keep here.

final model = DataModel();
model.set('/a/b', 's');
expect(() => model.set('/a/b/c', 1), throwsA(isA<A2uiDataError>()));
expect(model.get('/a/b'), 's');
});

test('rejects writes when the root itself is a primitive', () {
final model = DataModel();
model.set('/', 1);
expect(() => model.set('/a', 2), throwsA(isA<A2uiDataError>()));
expect(model.get('/'), 1);
});

test('rejects excessively large list indices to prevent OOM', () {
final model = DataModel();
expect(
Expand Down
17 changes: 17 additions & 0 deletions renderers/web_core/src/v0_9/state/data-model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import * as assert from 'node:assert';
import {describe, it, beforeEach} from 'node:test';
import {DataModel} from './data-model.js';
import {A2uiDataError} from '../errors.js';

describe('DataModel', () => {
let model: DataModel;
Expand Down Expand Up @@ -446,6 +447,22 @@ describe('DataModel', () => {
assert.strictEqual(model.get('/items/invalid'), undefined);
});

it('rejects writes when the root itself is a primitive', () => {
const primitiveRoot = new DataModel({});
primitiveRoot.set('/', 1);
assert.throws(() => primitiveRoot.set('/a', 2), A2uiDataError);
assert.strictEqual(primitiveRoot.get('/'), 1);
});

it('keeps a falsy primitive root instead of replacing it', () => {
for (const root of [false, 0, '']) {
const falsyRoot = new DataModel({});
falsyRoot.set('/', root);
assert.throws(() => falsyRoot.set('/a', 2), A2uiDataError);
assert.strictEqual(falsyRoot.get('/'), root);
}
});

it('rejects leading-zero array indices (RFC 6901)', () => {
assert.throws(() => {
model.set('/items/01', 'value');
Expand Down
10 changes: 9 additions & 1 deletion renderers/web_core/src/v0_9/state/data-model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,16 @@ export class DataModel {
const segments = this.parsePath(path);
const lastSegment = segments.pop()!;

if (!this.data) {
// Only an absent root is replaced with a container. A primitive root is a
// value the caller put there, and writing a path through it is the same
// error as writing through a primitive at any other depth.
if (this.data === undefined || this.data === null) {
this.data = {};
} else if (typeof this.data !== 'object') {
throw new A2uiDataError(
`Cannot set path '${path}': the data model root is a primitive value.`,
path,
);
}
let current: any = this.data;
for (let i = 0; i < segments.length; i++) {
Expand Down
Loading