Skip to content
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
27 changes: 27 additions & 0 deletions .changeset/olive-pears-invent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
---

Declare the batch write-warning's unattributed-strip placeholder in
`@object-ui/data-objectstack`, and pin what it is (objectui#7160).

`notifyBatchDroppedFields` resolves the object a cross-object strip is about
from the wire entry's `object`, else from the operation its `index` addresses.
When neither channel answers, it wrote a bare `''` — a value satisfying the
spec's required `object: string` while naming no object at all. That literal is
now the named, documented `UNATTRIBUTED_STRIP_OBJECT`, carrying the reachability
argument, why such a strip is still emitted rather than refused, and why the
placeholder must stay falsy. A stale comment claiming an unattributable strip
"reads as an update" is corrected to what the code does (it lands on `create`,
tracked as objectui#7170).

No published behaviour changes and no released surface moves: the value is
unchanged, and the new boundary suite passes identically against the pre-change
source. The emitted `dist/index.d.ts` is byte-identical at both shas, with a
live control (one temporary exported const) proving the comparison detects a
real surface change.

`node scripts/check-changeset-presence.mjs` on this tree, verbatim:

> ✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/olive-pears-invent.md.
> Every one of them has an EMPTY frontmatter — declared as releasing nothing, which
> is the explicit exemption and a complete answer to this gate.
Original file line number Diff line number Diff line change
@@ -0,0 +1,226 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* What the batch write-warning says about a strip it CANNOT attribute to an
* operation (objectui#7160).
*
* `notifyBatchDroppedFields` names the object a cross-object strip is about
* from the wire entry's `object`, else from the operation its `index`
* addresses. When the wire named none AND the index addresses no operation in
* the request we sent, there is nothing left to name it with, and the adapter
* writes its `UNATTRIBUTED_STRIP_OBJECT` placeholder onto both the notice's
* `object` and the event's `resource`.
*
* Third sibling to `droppedFieldsReason.boundary.test.ts` (objectui#4934) and
* `droppedFieldsShape.boundary.test.ts` (objectui#6889), and deliberately a
* DIFFERENT disposition from both, because the fact is different:
*
* - `reason` from the future is the producer running AHEAD of us — expected
* version skew, so it earns an explicit arm carrying the wire value verbatim.
* - a non-string `fields` element is off-spec input that would reach a
* consumer typed as a field name — refused here, fixed at the producer.
* - an unattributable strip supplies NO producer value at all. There is
* nothing to keep or drop; the only question is what we write in its place.
*
* So this suite pins the placeholder rather than removing it, and pins the two
* properties that make it safe:
*
* - the warning is still EMITTED. Refusing it would trade a truthful,
* user-visible warning about fields the server really did strip for silence
* — objectui#3484's failure. Measured end to end for the PR: the user still
* gets the save acknowledgement, the field list and the reason sentence,
* with fields named by api key instead of label.
* - the placeholder is FALSY. That is load-bearing, not incidental: the sole
* consumer (`app-shell`'s `writeWarningToast`) gates label resolution on
* `adapter && ev.resource`, so an empty resource skips the schema lookup and
* names fields by their api key. A truthful-but-truthy sentinel would send
* that consumer to `getObjectSchema('objectui:...')` instead. Nothing pinned
* this before, so a "cleanup" replacing `''` could have broken the consumer
* with every adapter test still green.
*
* REACHABILITY. Only from a response that is off-spec twice over: the spec's
* `CrossObjectBatchDroppedFieldsSchema` declares BOTH `object: z.string()` and
* `index: z.number()` required, and documents `results` as index-aligned with
* the request's `operations`. Nothing in this repo emits that shape, and
* whether a deployed backend does is not answerable from here. What is pinned
* is the DISCRIMINATION, not a population — every zero below keeps a live
* control beside it so a green run cannot be a silently-empty one.
*/
import { describe, it, expect, vi } from 'vitest';
import { ObjectStackAdapter } from './index';
import type { WriteWarningEvent } from './index';

function makeDS(stub: Record<string, any>) {
const ds: any = new ObjectStackAdapter({
baseUrl: 'http://test.local',
fetch: vi.fn(async () =>
new Response(JSON.stringify({ success: true, data: { capabilities: {}, routes: {} } }), {
status: 200,
headers: { 'Content-Type': 'application/json' },
}),
),
});
ds.connected = true;
ds.connectionState = 'connected';
ds.client = { data: stub };
ds.atomicBatchCapability = true;
return ds;
}

/**
* One two-op batch — the shape the console's record form saves a master-detail
* record with. Operation 0 creates an `account`, operation 1 updates an
* `invoice`, so a resolvable `index` has a distinct object to name.
*/
async function emitOnBatch(droppedFields: unknown[]): Promise<WriteWarningEvent[]> {
const batchTransaction = vi.fn().mockResolvedValue({
results: [{ id: 'acc1' }, { id: 'inv1' }],
droppedFields,
});
const ds = makeDS({ batchTransaction });
const events: WriteWarningEvent[] = [];
ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
await ds.batchTransaction([
{ object: 'account', action: 'create', data: { name: 'Acme' } },
{ object: 'invoice', action: 'update', id: 'inv1', data: { tax_rate: 7 } },
]);
return events;
}

describe('an unattributable batch write-strip (#7160)', () => {
it('is still emitted — it does not recreate #3484 silence', async () => {
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 99 },
]);

expect(events).toHaveLength(1);
// Everything the response DID establish survives: the user is told which
// fields did not take effect, and why.
expect(events[0].droppedFields).toHaveLength(1);
expect(events[0].droppedFields[0].fields).toEqual(['tax_rate']);
expect(events[0].droppedFields[0].reason).toBe('readonly_when');
});

it('names the object with the falsy placeholder on BOTH the notice and the event', async () => {
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 99 },
]);

// One spelling for both, as objectui#6889 unified them.
expect(events[0].resource).toBe('');
expect(events[0].droppedFields[0].object).toBe('');
// The property the only consumer actually depends on. `writeWarningToast`
// gates label resolution on `adapter && ev.resource`, so a truthy
// placeholder would send it to `getObjectSchema(<placeholder>)`.
expect(events[0].resource).toBeFalsy();
expect(events[0].droppedFields[0].object).toBeFalsy();
// Still a string: `DroppedFieldsEvent.object` is `z.string()` in the spec
// and `WriteWarningEvent.resource` is `string`. The placeholder satisfies
// the declared type — which is exactly what makes it a placeholder needing
// a declaration rather than a type change.
expect(typeof events[0].resource).toBe('string');
expect(typeof events[0].droppedFields[0].object).toBe('string');
});

it('CONTROL — an index that DOES resolve names the object, truthily', async () => {
// Same driver, same entry but for the index: a live non-zero reading, so
// the falsy assertions above are a measurement and not a broken harness.
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
]);

expect(events).toHaveLength(1);
expect(events[0].resource).toBe('invoice');
expect(events[0].droppedFields[0].object).toBe('invoice');
expect(events[0].resource).toBeTruthy();
});

it('CONTROL — a wire `object` still wins even when the index resolves to nothing', async () => {
// The placeholder is reached only when BOTH channels fail. A server that
// honours the spec's required `object` never reaches it, whatever its index
// says.
const events = await emitOnBatch([
{ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when', index: 99 },
]);

expect(events[0].resource).toBe('invoice');
expect(events[0].droppedFields[0].object).toBe('invoice');
});

it('reaches the placeholder for every unresolvable `index`, missing one included', async () => {
// `operations[i]` is `undefined` for an absent, out-of-range, negative or
// non-integer index alike; none of them is a spec-conformant value.
const indices: Array<Record<string, unknown>> = [
{},
{ index: 99 },
{ index: -1 },
{ index: 1.5 },
{ index: Number.NaN },
];
for (const tag of indices) {
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', ...tag },
]);
expect(events, JSON.stringify(tag)).toHaveLength(1);
expect(events[0].resource, JSON.stringify(tag)).toBe('');
}
});

it('omits `id` rather than inventing one — the honest arm of the same trigger', async () => {
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 99 },
]);

expect('id' in events[0]).toBe(false);

// CONTROL on the same instrument: a resolvable index carries the op's id.
const control = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
]);
expect(control[0].id).toBe('inv1');
});

it('RECORDS (does not bless) the `operation: create` claim — objectui#7170', async () => {
// `operation` is picked as `(op?.action ?? 'create') === 'create' ? ... `,
// so a strip with no operation to read lands on `create` — a second
// fabrication under the same trigger, filed separately because there is no
// correct value to fall back to and `WriteWarningEvent.operation` is a
// REQUIRED `'create' | 'update'` on a published type. Pinned so whichever
// disposition triage picks arrives as a visible diff instead of silently.
const events = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 99 },
]);
expect(events[0].operation).toBe('create');

// CONTROL: the same instrument reports `update` when the op resolves, so
// the line above is reading the fabrication and not a constant.
const control = await emitOnBatch([
{ fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
]);
expect(control[0].operation).toBe('update');
});

it('leaves the single-record path alone — its fallback is always a real name', async () => {
// There is no unattributable case there: `asDroppedFieldsNotice`'s fallback
// is the resource the caller passed to create/update, so the placeholder is
// structurally unreachable on that path.
const create = vi.fn().mockResolvedValue({
record: { id: 'r1' },
droppedFields: [{ fields: ['type'], reason: 'readonly' }],
});
const ds = makeDS({ create });
const events: WriteWarningEvent[] = [];
ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
await ds.create('andon', { title: 'T' });

expect(events).toHaveLength(1);
expect(events[0].resource).toBe('andon');
expect(events[0].droppedFields[0].object).toBe('andon');
});
});
83 changes: 80 additions & 3 deletions packages/data-objectstack/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1612,6 +1612,68 @@ function asDroppedFieldsNotice(
};
}

/**
* What the batch path writes for `object` / `resource` when the response did
* not let it attribute the strip to an operation (objectui#7160).
*
* {@link ObjectStackAdapter.notifyBatchDroppedFields} resolves the object a
* cross-object strip is about from the wire entry's own `object`, else from the
* operation its `index` addresses. When the wire named no object AND the index
* addresses no operation in the request WE sent, nothing is left to name it
* with: the adapter knows every operation in its own batch, so an index outside
* that list cannot be healed the way a missing `object` is healed on the
* single-record path (there the fallback is the resource the caller passed in,
* which is always a real name).
*
* REACHABILITY — only from a response that is off-spec twice over. The spec's
* `CrossObjectBatchDroppedFieldsSchema` declares BOTH `object: z.string()` and
* `index: z.number()` REQUIRED, and the batch response documents `results` as
* index-aligned with the request's `operations`. A conformant server therefore
* cannot produce this entry; it takes one that omits (or non-strings) `object`
* AND sends an index naming no operation, in the same entry. Nothing in this
* repo emits that shape, and whether a deployed backend does is not answerable
* from here. Unlike objectui#6889's exotic case this is not structurally
* impossible — the payload arrives as parsed JSON, and a non-conformant server
* can send it.
*
* WHY THE ENTRY IS STILL EMITTED rather than refused. Measured end to end
* through the real chain (`onWriteWarning` into `app-shell`'s
* `emitWriteWarning`, with the real `t` and the real `fieldLabel`): the user
* still gets the whole warning — the save acknowledgement, the field list and
* the reason sentence — with fields named by their api key instead of their
* label. Refusing the entry would replace a truthful, useful warning with
* silence for a strip the server really did report, which is objectui#3484's
* failure and the reason neither `object` nor `reason` is gated on above.
*
* WHY IT IS NOT WIDENED AWAY EITHER. Letting the notice say "no object" means
* making `object` optional on {@link DroppedFieldsNotice}, whose canonical arm
* IS the spec's `DroppedFieldsEvent` (objectui#3160). That writes "servers may
* omit `object`" into our published client type to accommodate a producer
* violating two REQUIRED spec fields — the lenient consumer-side fallback
* AGENTS.md #0.1 bans, fossilising the producer's bug into a second de-facto
* contract. The contract-first repair for an off-spec response is at the
* producer.
*
* So this is neither the skew arm's "tolerate" (objectui#4934 — a `reason` from
* the future is the producer running AHEAD of us, expected version skew) nor
* `fields`' "refuse" (objectui#6889 — an off-spec element that would otherwise
* reach a consumer typed as a field name). There is no producer value to keep
* or drop here: the question is only what WE write when the response supplied
* nothing. The answer is a DECLARED placeholder rather than a bare literal that
* reads as a name.
*
* IT MUST STAY FALSY, and that is why it is not exported. The sole consumer
* (`app-shell`'s `writeWarningToast`, reached through `AdapterProvider`) gates
* label resolution on `adapter && ev.resource`, so an empty resource skips the
* schema lookup and names fields by their api key — the truthful fallback. A
* namespaced sentinel like {@link UNRECOGNIZED_DROP_REASON} would be TRUTHFUL
* but TRUTHY, and would send that consumer to `getObjectSchema('objectui:...')`
* and `fieldLabel('objectui:...', ...)`. No consumer should branch on this
* value's identity; the falsiness check is the whole correct handling, and
* `droppedFieldsUnattributed.boundary.test.ts` pins both halves.
*/
const UNATTRIBUTED_STRIP_OBJECT = '';

/**
* Emitted after a create/update whose response carried `droppedFields`
* (framework #3431/#3455). The write SUCCEEDED — this is a warning that some
Expand Down Expand Up @@ -2675,12 +2737,16 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// `resource` — they used to be computed separately, so a wire entry with
// a non-string `object` could put one value on the notice and another on
// the event describing it.
//
// The last arm is NOT a name — see {@link UNATTRIBUTED_STRIP_OBJECT} for
// what makes it reachable, why such a strip is still emitted rather than
// refused, and why the placeholder is not widened away (objectui#7160).
const object =
typeof e.object === 'string'
? e.object
: typeof op?.object === 'string'
? op.object
: '';
: UNATTRIBUTED_STRIP_OBJECT;
// Same no-op suppression as the single-record path (#3484). The echoed
// row for the originating op is the "stored" side; when the batch echoed
// nothing usable, `withoutNoOpDrops` keeps every field.
Expand All @@ -2699,8 +2765,19 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
stored,
);
if (!live) continue;
// `delete` never drops fields; anything unexpected reads as an update,
// which is the truthful default for a batch that echoed a strip.
// An op carrying any action other than `create` reads as an update:
// `delete` never drops fields, so that is the truthful default for a
// batch that echoed a strip.
//
// An entry whose `index` resolved to NO operation has no action to read
// at all, and lands on `create` — a claim nothing establishes, the same
// trigger as `UNATTRIBUTED_STRIP_OBJECT` one field over. It is tracked
// separately (objectui#7170) rather than repaired here: unlike the object
// name there is no correct value to fall back to, and `operation` is
// REQUIRED `'create' | 'update'` on the published `WriteWarningEvent`, so
// saying "unattributed" would move that surface. Today's sole consumer
// does not read `operation`, and the boundary suite pins the current
// value so a change to it cannot land unnoticed.
const operation: 'create' | 'update' = (op?.action ?? 'create') === 'create' ? 'create' : 'update';
this.emitWriteWarning({
operation,
Expand Down
Loading