Skip to content

feat: migrate audit to query-lib schema objects - #206

Open
abnegate wants to merge 3 commits into
mainfrom
codex/audit-query-lib-publish-20260905
Open

feat: migrate audit to query-lib schema objects#206
abnegate wants to merge 3 commits into
mainfrom
codex/audit-query-lib-publish-20260905

Conversation

@abnegate

@abnegate abnegate commented Sep 4, 2026

Copy link
Copy Markdown
Member

Summary

Canonical continuation of utopia-php/audit#133 in packages/audit, following the approved move to the monorepo.

  • Return typed Attribute and Index schema objects and use Collection for database setup.
  • Adapt ClickHouse schema, DDL and encoding paths to value objects while preserving existing fields, indexes, null handling and tenant behavior.
  • Require the genuine Database dev-feat-query-lib VCS branch without an inline alias; declare PDO MySQL and Swoole test extensions.
  • Cover nullable logs, schema defaults and string filtering.

Verification

Both registry and linked modes passed:

  • Unit: 22 tests, 106 assertions.
  • E2E: 92 tests, 1,023 assertions, including MariaDB and ClickHouse.

Pint, PHPStan, Rector, Composer validation and monorepo validation passed.

Hosted CI and PR review remain pending. No package split, release, tag or merge has been performed.

@abnegate

abnegate commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

@greptile-apps review

@abnegate

abnegate commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

@greptileai review the current head 394aa7f. The September 4 request using the older bot handle did not start a review; this uses the currently documented trigger.

@abnegate

abnegate commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

@greptileai review

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge because no new actionable finding remains outside the previously resolved threads.

Summary

  • Converts shared and ClickHouse-specific schema metadata to query-lib value objects.
  • Updates database collection creation and ClickHouse DDL, encoding, and column-selection paths.
  • Adds behavioral MariaDB coverage for nullable/default fields, full-length user IDs, and string filtering.
  • Updates package dependency and test-extension metadata.

Comment thread packages/audit/tests/Audit/Adapter/DatabaseTest.php Outdated
Comment thread packages/audit/composer.json
The schema assertions pinned attribute counts, array positions, column
types, sizes, filters, index types and index column ordering straight off
the adapter's getAttributes()/getIndexes(). That mirrors the source rather
than testing it: the repository requires observable behaviour, and these
lines only ever compared the schema definition against a second copy of
itself.

Every deleted property is already covered through the public API, proven
by breaking each one and watching a behavioural test go red: dropping the
json filter on data, dropping the datetime filter on time, making
idx_userId_event unique and making userId required each fail the existing
write/read tests. Index column ordering is the one property with no
observable behaviour at all -- swapping it reaches the real DDL and every
test still passes -- so no assertion can guard it.

userId's size was the only property not otherwise reachable, so it is
replaced rather than dropped: testUserIdAcceptsFullKeyLength writes a
key-length user id and reads it back, which fails once the column shrinks.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@abnegate

abnegate commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

@greptileai review

@abnegate
abnegate force-pushed the codex/audit-query-lib-publish-20260905 branch from d7bd655 to 4e53dd5 Compare September 9, 2026 10:14
@abnegate

abnegate commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Both findings addressed at 4e53dd58.

Finding 1 — tests mirror schema internals: fixed.

DatabaseTest::testSchemaValueObjectsAndNullableLog and ClickHouseTest::testPremiumGeoAttributesAreOptionalStrings are gone. Before deleting anything I broke each property in the schema and checked a behavioural test actually goes red, so the coverage is proven and not assumed:

schema change outcome
drop filters: ['json'] on data 17/17 DatabaseTest red — Attribute "data" has invalid type
drop filters: ['datetime'] on time testLogByBatch red
idx_userId_event Index::keyIndex::unique 17/17 red (createLogs() writes duplicate userId+event pairs)
userId required: falsetrue 17/17 red
userId size 255 → 128 testUserIdAcceptsFullKeyLength red
idx_userId_event attributes ['userId','event']['event','userId'] green

That last row is the interesting one. The swap reaches the real DDL (KEY idx_userId_event (event, userId)) and all 16 remaining tests still pass, because index column order is a query-planner concern with no behaviour observable through the audit API. No test can ever guard it — which is exactly why asserting it only compared the schema against a second copy of itself.

One property was not otherwise reachable, so it is replaced rather than dropped: userId's size is now covered by testUserIdAcceptsFullKeyLength, which writes a key-length user id and reads it back.

The ClickHouse geo flags are fully subsumed by the existing testPremiumGeoRoundTrip, which writes all nine geo fields and asserts each round-trips unchanged; required is covered by every test's setUp(), which writes logs carrying no geo fields at all.

Finding 2 — ext-swoole: this one is a false positive, and I've left the requirement in place.

The premise is right (nothing under packages/audit mentions Swoole — grep -rniE 'swoole|Coroutine|WaitGroup|\bgo\(' over src/, tests/ and the README returns nothing) but the requirement is transitive, not local. utopia-php/database@dev-feat-query-lib calls it unguarded:

// vendor/utopia-php/database/src/Database/Database.php:1442
private function getEventContext(): int
{
    $context = Coroutine::getCid();

getEventContext() backs silent() / areEventsSilenced(), which deleteDocuments() uses — the path Audit::cleanup() takes, which every test hits in setUp(). Note line 2848 in the same file does guard with extension_loaded('swoole'); line 1442 does not, and utopia-php/database declares no ext-swoole of its own.

I pushed the removal first and CI proved it: test (audit) went to 17 errors, Class "Swoole\Coroutine" not found, and it reproduces locally with the extension unloaded. Dropping the requirement doesn't make the package installable without Swoole, it just converts a clear resolution-time error into 17 runtime fatals — so the declaration stays, per the house preference for declaring real ext-* requirements over --ignore-platform-reqs.

The real fix belongs upstream (guard line 1442 like 2848, or declare ext-swoole in utopia-php/database); tracking that separately rather than in this PR.

Verified at 4e53dd58: unit OK (22 tests, 106 assertions), e2e OK (92 tests, 956 assertions), and bin/monorepo check audit — pint, phpstan level 5, rector — all pass.

@abnegate

abnegate commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

@greptileai review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant