Repository navigation
Conversation
…ma_defined tables describe_table on a schema_defined: true table only ever reported the declared attributes, even when stored records carried additional fields written directly (schema_defined tables do not auto-register attributes discovered from writes, and are not implicitly sealed, so this can happen silently). There was no signal that the schema and the data disagreed. schema-only scoping in schema_defined mode is intentional and unchanged by this fix -- describe_table still reports only declared attributes in `attributes`. What is new: a bounded sample (up to 100 records, capped on both lmdb-js and rocksdb-js since only lmdb-js honors getRange limit) is scanned for attributes absent from the declared schema, surfaced as a new `undeclared_attributes` field on the response. Logged at trace (not warn) since the response field is the actual signal and this condition persists across repeated polls (e.g. Studio/monitoring) until someone declares the attribute. skip_record_count also skips this scan. Refs #664 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to detect and surface undeclared attributes on schema_defined tables by sampling a small set of records during a describe_table operation, and adds integration tests to verify this behavior. The review feedback suggests using optional chaining on the logger call to prevent potential runtime errors from disrupting the table description process, and updating the test file to import node:assert/strict instead of node:assert to align with testing standards.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed; no blockers found. |
…rmission suppression - logger.trace -> logger.trace?.(), matching the optional-chaining convention used elsewhere (Table.ts) so a logger without .trace can't abort the surrounding try block and skip the other table-stats fields. - Convert assert.equal/deepEqual to assert.strictEqual/deepStrictEqual in the new integration test, matching this repo's current testing convention. - Add real test coverage for the attribute-permission-suppression branch: a role with attribute_permissions restricting the Account table to `id` only, a user bound to that role, and an assertion that describe_table's response for that user never carries an `undeclared_attributes` key at all (not even empty) -- undeclared attributes have no permission entry to check them against, so surfacing their names would leak metadata the role can't see. Sanity-checked the new test is real: temporarily inverted the `tableObj.schemaDefined && !tableAttrPerms` guard to `tableObj.schemaDefined`, confirmed the new test fails with a clear AssertionError, then restored the correct guard and confirmed all 8 tests pass again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // `limit` is honored by lmdb-js (a free read-side optimization) but ignored by rocksdb-js, so | ||
| // the explicit break below still bounds the scan on both backends. | ||
| for (const { value: record } of tableObj.primaryStore.getRange({ | ||
| start: true, |
There was a problem hiding this comment.
Low: sampling from the start of the key range biases against detecting recently-introduced drift.
start: true scans the first N records in key order. For an incrementing/ordered primary key that is oldest-first, so the common schema-drift case — a field that begins appearing in newer writes on a table with more than UNDECLARED_ATTRIBUTE_SAMPLE_SIZE rows — is never observed, and undeclared_attributes silently stays empty. Issue #664's own repro (uniform extra attributes across all 211k records) is fine, but drift added over time on a large ordered-PK table would be missed, giving false confidence that schema and data agree.
Sampling from the end (reverse: true) would favor recent writes, or sampling both ends (as getRecordCount already does for its estimate) would catch either pattern. At minimum, worth noting in the field's contract that absence of undeclared_attributes is a best-effort signal, not a guarantee.
—
Generated by Barber AI
|
Closing per Kris's review: on further look, the current behavior is correct — |
2 participants
Summary
Fixes #664.
describe_tableon aschema_defined: truetable only ever reported the declared attributes, even when stored records carried additional fields written directly — with no signal that the schema and the data disagreed.Resolution path (of the two the issue offered)
Kept
schema_defined: true's scoping intentional and unchanged —attributesstill reports only the declared schema. Went with the "surface a clear warning/signal" path instead of merging schema + data-derived attributes: a bounded sample (up to 100 records, explicitly capped since only lmdb-js honorsgetRange'slimit, not rocksdb-js) is scanned for attributes absent from the declared schema, surfaced as a newundeclared_attributesfield on thedescribe_table/describe_allresponse. Logged attrace(notwarn) since the response field is the actual actionable signal, and this condition persists across repeated polls (e.g. Studio/monitoring) until someone declares the attribute — awarnwould just repeat noisily.skip_record_countalso skips this scan.Testing
New integration test (
integrationTests/apiTests/describe-table-undeclared-attributes.test.mjs, 5 cases): create aschema_definedtable with only a PK declared, insert a record with extra fields, confirmdescribe_tablestill reports only the declared attribute inattributes, confirmundeclared_attributessurfaces the actual mismatch, confirmskip_record_countomits the scan.🤖 Generated with a dev-agent worker + finished/verified by Claude Code (kris-dev-manager EM session) after the worker's session was interrupted mid-review (killed background Codex review task, harness safety-net flagged
needs-input). Build, tests (5/5), lint, and format all verified clean before pushing.