Skip to content

Fix: upgrade the @mikro-orm family from v6 to v7 - #797

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/mikro-orm-v7-family-upgrade
Open

Fix: upgrade the @mikro-orm family from v6 to v7#797
AmaadMartin wants to merge 4 commits into
mainfrom
fix/mikro-orm-v7-family-upgrade

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

Related: google#448

  1. Or, if no issue exists, describe the change:

Problem: Dependabot PR google#448 moves only @mikro-orm/sqlite to ^7.1.6. That cannot install: the v7 drivers peer-depend on @mikro-orm/core at the exact string 7.1.11, so a v7 driver beside a v6 core fails with ERESOLVE. The v6 sqlite driver is also how the deprecated tar@6.2.1 reaches this repository, through @mikro-orm/knex -> sqlite3 -> node-gyp -> tar.

Solution: I moved all eight @mikro-orm/* packages to ^7.1.11 together and did the v6 -> v7 API migration. 7.1.11 is the current latest for @mikro-orm/core and satisfies the ^7.1.6 Dependabot asked for. Four v7 changes reach ADK code: the decorators moved to @mikro-orm/decorators, Options now marks defaulted keys as required, SchemaGenerator.updateSchema became update, and the query runner changed from knex to kysely.

The kysely change is the one behaviour fix. appendEvent called txEm.commit() inside an em.transactional callback, which commits the transaction early. em.transactional owns that boundary and commits once the callback resolves. knex tolerated the second commit; kysely throws Transaction is already committed. I deleted the inner commit.

Two new core runtime dependencies:

  • @mikro-orm/decorators — v7 moved @Entity, @PrimaryKey and @Property out of core. The repo compiles with experimentalDecorators, so schema.ts imports the /legacy entry point, not /es.
  • reflect-metadata — an optional peer of @mikro-orm/decorators, but legacy/index.js re-exports legacy/ReflectMetadataProvider.js, whose first statement is import 'reflect-metadata'. The export * evaluates that module, so importing anything from /legacy requires the package. v6 supplied it transitively through @mikro-orm/core; v7 does not. Without it, import '@google/adk' throws ERR_MODULE_NOT_FOUND for both ESM and CommonJS consumers, which I measured against the built core/dist. A comment at the import site in schema.ts records this, because the dependency looks unused from the source alone.

I also removed @mikro-orm/reflection rather than re-pinning it. Nothing in the repo imports it, and it pulled ts-morph and seven more packages into every consumer's tree. Verified by deleting it from node_modules: the package still imports, the SQLite round-trip still passes, and all 146 session tests still pass. This is a deliberate deviation from a mechanical "bump every @mikro-orm/*" reading of the task.

Measured lockfile effect: 90 entries removed, 8 added, 35 version-changed. grep -c '"node_modules/tar"' package-lock.json prints 0, so tar@6.2.1 is gone from the tree; I have not verified which advisories that closes. tarn, tar-fs and tar-stream are unrelated packages and remain. Removed: @mikro-orm/knex, knex, sqlite3, node-gyp, cacache, tar, mariadb, @mikro-orm/reflection, ts-morph and their satellites. Added: @mikro-orm/sql, @mikro-orm/decorators, kysely, better-sqlite3, pg-cursor and three nested entries. Every changed entry traces to the MikroORM subtree.

This is breaking for downstream consumers. The five driver peerDependencies move to ^7.1.11, so an application on @mikro-orm@6 that uses DatabaseSessionService must upgrade its own packages. Consumers also inherit MikroORM v7's engines.node: ">= 22.17.0" (v6 required >= 18.12.0). I did not mark the commit ! or add a BREAKING CHANGE: footer, because that would make release-please cut a major version of @google/adk, which is a maintainer decision.

Data-compatibility note: v7 enables forceUtcTimezone by default for SQL drivers. The datetime columns (StorageSession.createTime/updateTime, StorageAppState.updateTime, StorageUserState.updateTime, StorageEvent.timestamp) are affected, so a MySQL, MSSQL or PostgreSQL database whose datetimes were written in local time under v6 will read back as UTC. I did not set forceUtcTimezone: false, because that is a product decision. SQLite stores millisecond timestamps and is unaffected.

Scope: this supersedes only the @mikro-orm half of google#448. The @opentelemetry/* and esbuild/concurrently bumps in that PR are separate tasks and are untouched here. I deliberately left #448 open for a maintainer to close.

No public ADK API changed. core/dist/types/sessions/database_session_service.d.ts still reads constructor(connectionStringOrOptions: MikroDBOptions | string);; only the alias definition moved to Partial<Options>. The database schema, table names, column names and the 191-character key length are unchanged, so existing databases need no migration.

Collision check: I listed all 400 open PRs on the fork. None upgrades @mikro-orm. Six touch adjacent files without conflicting on this change (#684 forces a patched tar via npm overrides, #500 marks the drivers optional peers, #489, #647, #740, #386). #684 and this PR reach the same tar outcome by different means; this one removes the chain instead of pinning it.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:

[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

I added no new tests. The change adds no production line: the diff is four modified lines plus two import-block rewrites, and the existing suite already covers every one.

The test edits are type-only. Besides updateSchema() -> update(), v7 narrows EntityName<T> to EntityClass | EntityCtor | EntitySchema, so five call sites that named an entity by string stopped typechecking. They now pass the class (em.find(StorageEvent, …), orm.getMetadata().get(StorageEvent)). The class also supplies the real return type, so two as {…} casts became unnecessary and I deleted them. No assertion changed: should align session updateTime with event timestamp and should trim temp state from event before persistence still pass, and I re-proved the first by writing event.timestamp + 1 in the source, which fails it with expected 1234567890001 to be 1234567890000.

tsc --noEmit -p core does not catch those five, because core/tsconfig.json includes only src/**/* and CI does not run ts:check. Measured with the root npm run ts:check: 292 errors / 44 files before the fix, 287 / 42 after, and 287 / 42 is the pre-existing baseline on main. This branch adds no type error.

I proved each existing test can fail by reverting each production hunk one at a time:

Mutation Result
Restore await txEm.commit() in appendEvent 13 of 26 fail: Transaction is already committed
Restore orm.schema.updateSchema({safe: true}) operations_test.ts fails: TypeError: orm.schema.updateSchema is not a function
Restore the decorator import from @mikro-orm/core tsc -p core fails (3 x TS2305/TS2724 plus 2 cascading TS2345); tests fail at collection: TypeError: (0 , PrimaryKey) is not a function
Delete node_modules/reflect-metadata import '@google/adk' fails for ESM and CommonJS: ERR_MODULE_NOT_FOUND … imported from @mikro-orm/decorators/legacy/ReflectMetadataProvider.js
Delete node_modules/@mikro-orm/reflection Nothing fails, which is why I removed the dependency

Commands run on the pushed commit:

npx tsc --noEmit -p core                                              # 0 errors
npm run ts:check                                                      # 287 errors / 42 files == main baseline
npx vitest run --project unit:core core/test/sessions                 # 146 passed
npx vitest run --project integration tests/integration/lazy_load_db_drivers   # 5 passed
npx vitest run --project integration tests/integration/build_setup    # 20 passed, 4 skipped
npm run build && npm run lint && npm run format:check && npm run docs:check   # all exit 0

The build_setup run matters beyond the stub rename: js_commonjs and ts_commonjs prove that pure-ESM @mikro-orm/core@7 is still reachable from CommonJS through Node's require(esm).

CI is green on ubuntu, macOS and Windows. The windows-latest leg needed two re-runs and failed on a different unrelated test each time: a 5s timeout in unsafe_local_code_executor_test.ts, then listen EACCES: permission denied ::1:49740 in webui_test.ts. Neither test is in my diff, and the first push of this branch carried the same dependency and production code through a green Windows leg.

Manual End-to-End (E2E) Tests:

Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

I ran a scratch script against the built core/dist, not against source, with a real SQLite file and no mocks:

  1. npm ci && npm run build.
  2. Construct new DatabaseSessionService('sqlite://<tmpfile>') and createSession with app:, user:, temp: and plain state keys.
  3. appendEvent twice with state deltas.
  4. Construct a second DatabaseSessionService on the same file and getSession.
  5. Assert the merged state, the temp: exclusion and the event order survived the instance boundary.

It passes, and it fails with Transaction is already committed when I restore txEm.commit() and rebuild. The second instance is the point: it opens a new ORM connection, so it only sees data the transaction actually committed to disk.

I did not commit that script as a test. Closing the ORM needs a cast to the private orm field, and PR #386 is removing exactly that pattern from the session tests; I did not want to add a new instance of it. The 13 existing tests already fail on the same mutation.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 7, 2026 23:35
The v7 drivers peer-depend on @mikro-orm/core at an exact version, so the
family has to move in lockstep. Adds @mikro-orm/decorators (v7 moved the
decorators out of core) and reflect-metadata (v7 core no longer pulls it in,
but @mikro-orm/decorators/legacy imports it unconditionally).

The regenerated lockfile drops the sqlite3 -> node-gyp -> tar native build
chain, which removes tar@6.2.1 from the tree.
Three v7 API changes reach this code:

- The decorators moved out of @mikro-orm/core into @mikro-orm/decorators.
  The repo compiles with experimentalDecorators, so it imports the /legacy
  entry point rather than /es.
- Options now marks every defaulted key as required, so the caller-facing
  shape is Partial<Options>. The local MikroORMOptions/MikroDBOptions aliases
  keep the exported signatures textually unchanged.
- SchemaGenerator.updateSchema is now update.

appendEvent also called txEm.commit() inside an em.transactional callback,
which commits the transaction early. em.transactional owns that boundary and
commits once the callback resolves. Under knex the second commit was silently
tolerated; the v7 kysely runner throws "Transaction is already committed".
Amaad Martin added 2 commits August 8, 2026 00:35
v7 narrows EntityName<T> to EntityClass | EntityCtor | EntitySchema, so the
five test call sites that named an entity by string stopped typechecking.
Passing the class also gives the real return type, so two casts are no longer
needed.

tsc --noEmit -p core missed this because core/tsconfig.json includes only
src/**/*. The root ts:check covers the test files: it reported 292 errors
before this commit and 287 after, which is the pre-existing baseline.
Partial<Options> was declared twice under two names with the same doc comment.
operations.ts already returns the type from getConnectionOptionsFromUri, so it
owns the definition now and database_session_service.ts imports it. The import
keeps the local name, so the emitted constructor signature is unchanged.

Also drop @mikro-orm/reflection. Nothing imports it, and it pulled ts-morph
and 7 more packages into every consumer's tree. Verified by removing it: the
package still imports, the sqlite round-trip still passes and all 146 session
tests still pass.

reflect-metadata stays. The decorators/legacy barrel re-exports
ReflectMetadataProvider, which starts with a bare import of it, so without the
dependency `import '@google/adk'` throws ERR_MODULE_NOT_FOUND for both ESM and
CommonJS consumers. A comment at the import site records why.
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