Name the cause when critter runtime generation can't access an entity by evanchooly · Pull Request #4365 · MorphiaOrg/morphia

evanchooly · 2026-10-05T14:28:11Z

Runtime critter generation defines property accessors as hidden nestmates (privateLookupIn(...).defineHiddenClass(..., NESTMATE)), which needs full privilege access to the entity, i.e. the entity must be in Morphia's module (on the classpath: the same class loader). In app-server shared-lib, isolated-loader, or JPMS layouts this failed with a raw IllegalAccessException message in the fallback warning.

CritterGenerator.defineNestmate wraps an IllegalAccessException from privateLookupIn/defineHiddenClass (only those calls) in a new internal NestmateAccessException.
CritterMapper.tryRuntimeGeneration looks for it in the cause chain and logs a clear warning: the entity isn't in Morphia's module/class loader, falling back to reflection, pre-generate with critter-maven. Other failures are logged as before.
Tests: an entity compiled at test time and loaded by its own URLClassLoader falls back to a reflective EntityModel and logs the new message once; a unit test covers the cause-chain classification.
Updated .claude/skills/diagnose/SKILL.md.

Docs: MorphiaOrg/morphia-docs#20

Core suite: critter 1301 run, 0 failures/errors, 16 skipped; reflection 1301 run, 0 failures/errors, 16 skipped.

Merge order

This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order:

This PR: no dependencies. #4368 conflicts with it in TestCritterMapper.java, so merge this before #4368.

Runtime generation defines hidden nestmate accessors, which needs full privilege access to the entity: it must be in Morphia's module (on the classpath, the same class loader). The generator now reports that failure as a NestmateAccessException, and CritterMapper logs a warning that names the cause and points to critter-maven instead of passing on the raw IllegalAccessException message. Fixes #4357

Copilot

Copilot review overview

🟡 Changes recommended

The warning-capture test can collect unrelated events from concurrent tests and fail intermittently.

Review effort: Balanced
Findings: 1

Open (1)

Restrict warning capture to the test thread · New

What changed in this PR

Improves diagnostics when Critter runtime generation cannot access an entity, while preserving reflection fallback.

Changes:

Wraps nestmate access failures in a dedicated internal exception.
Logs deployment guidance for module/class-loader access failures.
Adds regression tests and updates troubleshooting documentation.

File	Description
core/src/test/java/dev/morphia/mapping/TestCritterMapper.java	Tests isolated-loader fallback and failure classification.
core/src/main/java/dev/morphia/mapping/CritterMapper.java	Adds targeted fallback warnings.
core/src/main/java/dev/morphia/critter/parser/generator/NestmateAccessException.java	Identifies nestmate access failures.
core/src/main/java/dev/morphia/critter/parser/generator/CritterGenerator.java	Wraps lookup and hidden-class access failures.
.claude/skills/diagnose/SKILL.md	Documents access restrictions and AOT guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

evanchooly · 2026-10-05T15:11:46Z

+
+        Logger logger = (Logger) LoggerFactory.getLogger(CritterMapper.class);
+        Level level = logger.getLevel();
+        ListAppender<ILoggingEvent> appender = new ListAppender<>();


Confirmed: core runs test classes concurrently, so another class's CritterMapper warning could land in this appender while it's attached. Fixed in c6401d1: the test now counts only WARN events logged on its own thread (mapping and the warning are synchronous on the calling thread) whose message mentions app.IsolatedEntity.

The setLevel(Level.WARN) stays: logback-test.xml sets dev.morphia to ERROR, so without it the warning is dropped before reaching any appender. The only side effect while it's raised is that other threads' CritterMapper warnings also print to the console; the level is restored in finally.

Test classes run concurrently, so another class's CritterMapper warning could reach the test's appender while it's attached.

The `JdkTests` job's matrix lists `java: [ 17, 21, 25 ]`, but it passed a hard-coded `java: 17` to the shared workflow. So all three legs (times both mappers) ran on JDK 17, and 21 and 25 were never tested. It now passes `${{ matrix.java }}`. The legs still use the build compiled on JDK 17 (`reuseBuild: true`), which is what we want: JDK 17 bytecode running on newer JVMs. Expect this PR's CI to be the first real run on 21 and 25, so any failures there are newly visible, not newly introduced. Found while working on #4360. ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies. Merge any time.

…4360) Refs #4353 ## What - **`pom.xml`**: new `final-field-mutation` profile. It activates only when **both** the JDK is 26+ (`<jdk>[26,)</jdk>`) **and** `-Dfinal.field.mutation=<mode>` is set. It sets the `argLine` property to `--illegal-final-field-mutation=<mode>`, so surefire passes it to the forked test JVM. On older JDKs the profile stays off, because those JVMs reject the option with `Unrecognized option` and won't start. It uses the `argLine` property instead of surefire `<configuration>`, so it still works with jacoco's `prepare-agent` in the `coverage` profile. - **`.github/workflows/build.yml`**: new `FinalFieldMutationTests` job. It runs `reflection` and `critter` on JDK 26 with `-Dfinal.field.mutation=deny`, using `reuseBuild` like the other legs. It is **`optional: true`** (`continue-on-error`) and is not in `Release.needs`. Option and values come from [JEP 500](https://openjdk.org/jeps/500): `--illegal-final-field-mutation=allow|warn|debug|deny`. The JDK 26 default is `warn`. With `deny`, `Field::set` on a final field throws `IllegalAccessException`. ## Why non-blocking This leg is **expected to fail** for now. Morphia still mutates final fields in the reflection mapper's `FieldAccessor`, critter's runtime nestmate accessors, critter's AOT `__writeXxx` for final fields, and `ConstructorCreator`'s re-set after construction (#4352). The leg's job is to list every remaining dependency and catch new ones. Once those paths are removed or put behind an explicit opt-in, drop `optional: true` so it blocks. This PR uses `Refs` rather than `Fixes` because #4353 also asks for a decision on entities whose finals can only be set reflectively. ## Reproduce locally (JDK 26+) ``` ./mvnw install -DskipTests cd core && ../mvnw surefire:test -Dmorphia.mapper=critter -Dfinal.field.mutation=deny ``` ## Notes - I had no JDK 26 locally, so I haven't yet seen which tests fail under `deny`. The first CI run of this leg will be the first list. - Separate issue, not changed here: the existing `JdkTests` matrix defines `java: [17, 21, 25]` but passes `java: 17` to the reusable workflow, so every JDK leg currently runs on 17. ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies. The new JDK 26 leg is non-blocking and expected to fail until the reflective final-field writes are removed (#4362 removes the most common one).

) Fixes #4352 ## Change `ConstructorCreator.set()` used to queue every decoded property into `pendingModels`/`pendingValues`, and `getInstance()` re-set all of them after construction, including the ones the constructor had just received. For an immutable, constructor-mapped entity that meant a redundant reflective `Field.set` on each `final` field for every decoded document. JDK 26 warns about that under [JEP 500](https://openjdk.org/jeps/500). Now only properties with no matching constructor parameter are queued. Properties the chosen constructor doesn't take, like the extra fields when `bestConstructor` picks a partial constructor, are still set after construction, as before. ## Behavior change If a constructor transforms its argument (normalizes, copies, wraps it in an unmodifiable collection, etc.), the constructor's result is kept. Before, the raw decoded value silently overwrote it. This is almost certainly what users expect, but it is a visible change for any entity that relied on the overwrite. ## Tests - New `ConstructorCreatorTest#constructorArgumentsAreNotResetAfterConstruction`: an immutable entity whose constructor lowercases/trims `name` and sorts `tags` into an unmodifiable list, plus a non-constructor field `note`. It checks that the normalized values survive the decode and that `note` is still set. With the fix reverted it fails under both mappers (`expected: <mixed case> but was: < MiXeD Case >`). - Full `morphia-core` suite: - critter: 1300 run, 0 failures, 0 errors, 16 skipped - reflection: 1300 run, 0 failures, 0 errors, 16 skipped ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies. **#4366 depends on it**; merge this first.

…models (#4363) `CritterMapper(CritterMapper other, MorphiaConfig config)` reused `other.runtimeModels` even when `config` changed settings that runtime generation bakes into bytecode (collection/property naming, discriminator function/key, property discovery, annotation providers). Entities first mapped through the copy got classes generated under the original's settings. The copy now keeps the original's runtime models when the config is the same instance or both configs have equal generation keys. Otherwise it uses `RuntimeModels.forConfig(config, classLoader)`, which means unshared models when the key is null (custom strategies) or the parent is a `CritterClassLoader`. Entities already mapped are still cloned from the original as before. The common copy paths (`copy()`, the datastore copy, and copies that change only the database name) still share generated classes. Tests: - `testCopyWithDifferentNamingGeneratesItsOwnModels`: fails without the fix (`expected: <Name> but was: <name>`) - `testCopyWithDifferentDatabaseSharesRuntimeModels`: a copy that changes only the database still reuses the generated class Core suite: 1301 tests, 0 failures, 16 skipped, with both critter and reflection mappers. Fixes #4356 ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies. **#4368 conflicts with it** in `TestCritterMapper.java`, so merge this before #4368.

…or (#4364) Fixes #4355 The `__writeXxx` method that `AddFieldAccessorMethods#writeFinalField` weaves for final fields called `Field.set` without handling its checked `IllegalAccessException`, so a failure surfaced as an undeclared checked exception. The `Field.set` call now sits in a `trying(...)` block that catches `Exception` and rethrows `new RuntimeException("Failed to set final field '<name>'", e)`. This is the same type, message and catch scope as the runtime path in `NestmateAccessorGenerator`, so both tiers fail the same way. The catch covers all `Exception`s, not only `IllegalAccessException`, to match the runtime tier, and every failure names the field. The Field lookup and caching stay outside the try. **Test:** `TestGeneration#testGeneratorFinalFieldWriteFailureIsWrapped` puts a Field that was never made accessible into the woven class's `__fieldCode` cache. That makes `Field.set` throw `IllegalAccessException` through the AOT accessor. The test then checks the exception type, message and cause. With the fix reverted, the test fails because the raw `IllegalAccessException` reaches the caller. **Results:** core 1300 tests (16 skipped), 0 failures under both critter and reflection. critter-maven verify passes (6 unit tests and 3 invoker tests). ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies; merges cleanly with the rest of the batch.

Adds round-trip tests (save, then find through the Datastore) for Java records mapped as entities. Covers #4354. **Depends on #4352.** These tests fail under every mapper and tier until `ConstructorCreator` stops re-setting properties it already passed to the constructor. ## What's covered `dev.morphia.test.records.TestRecordEntities`: - a record `@Entity` with an `@Id`, a `String`, an `int` renamed with `@Property("years")`, and a `List<String>`. The test checks the stored document's shape, then finds by `_id` and by the renamed component. - a record embedded in a regular class entity, both as a single field and in a list. Each scenario runs against two sets of fixtures: | Fixtures | Package | Critter tier | |---|---|---| | `AotRecordPerson` / `AotRecordAddress` / `AotRecordHolder` | `dev.morphia.test.models.records` (under `morphia.packages`) | AOT, generated by critter-maven `generate-test-models` | | `RuntimeRecordPerson` / `RuntimeRecordAddress` / `RuntimeRecordHolder` | `dev.morphia.test.records` (outside `morphia.packages`) | runtime generation | Every test asserts which tier built the model: a reflection `EntityModel` under `reflection`, and under `critter` a `CritterEntityModel` whose class loader is or isn't a `CritterClassLoader`. If critter quietly falls back to another tier, the test fails rather than passing against the wrong tier. ## Current failure (all 4 tests, both mappers) Saving works. Decoding fails in `ConstructorCreator.getInstance()`. The canonical constructor builds the record correctly, then the `pendingModels` loop calls `PropertyModel.setValue` on every component again. `Field.set` always throws for record fields, even after `setAccessible(true)`: - reflection: `FieldAccessor.set` -> `IllegalAccessException: Can not set final ... AotRecordPerson.id` - critter AOT: the generated `IdAccessor.set` -> the injected `AotRecordPerson.__writeId` -> `Field.set` -> same exception - critter runtime: `Failed to set final field 'id'` I checked this locally. Queuing only the properties that have no constructor parameter (the #4352 fix) makes all 4 tests pass under both `critter` and `reflection`, with the tier assertions passing too. That change is not in this PR. Critter generation needed no fixes: AOT and runtime both generate models for the records without complaint. ## Full core suite (on this branch, without the #4352 fix) - critter: 1303 run, 0 failures, 4 errors (the new tests), 16 skipped - reflection: 1303 run, 0 failures, 4 errors (the new tests), 16 skipped Fixes #4354 ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** #4362 is merged and this branch includes it, so it's ready to merge. ### CI note Pull-request CI runs `install` without critter-maven's `generate-test-models`, so there are no pre-generated models there and the `Aot*` fixtures get runtime-generated models. The tier check expects AOT exactly when the pre-generated model is on the classpath (push builds and local runs that generate them) and runtime generation otherwise. With the check forced to expect runtime, the AOT tests fail when models are present, so it still catches a silent tier fallback. Follow-up for the CI gap: #4369.

…cs (#4367) Fixes #4358 - **`CritterClassLoader.findClass`**: the name is now added to `definedTypes` only after `defineClass` succeeds, so a `ClassFormatError`/`VerifyError` from bad bytes no longer leaves `getResource` hiding the parent's `.class` resource. The removed bytes are deliberately not restored: a retry would define the same bytes and fail the same way. - **`CritterMapper`**: the class-level and `tryRuntimeGeneration` javadocs now say runtime generation uses hidden nestmate accessors, not VarHandles. - **`CritterClassLoader.shouldRegister`**: the comment now explains that `NestmateAccessorRegistry`'s map, though keyed per `CritterClassLoader`, is a static field, so the generator (which registers through the parent's copy of the class) and the generated models (which look accessors up) must see the same `Class`. New test `CritterClassLoaderTest.failedDefinitionsDoNotHideResources` registers invalid bytes for `dev.morphia.critter.Critter`, expects `ClassFormatError` on load, then asserts the `.class` resource is visible again. It fails without the fix. Core suite: 1300 tests, 0 failures, 16 skipped with both `-Dmorphia.mapper=critter` and `reflection`. ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** no dependencies; merges cleanly with the rest of the batch.

…se (#4368) Fixes #4359 Test-only changes in `TestCritterMapper`: - **Tier assertions.** New `assertRuntimeGenerated(EntityModel)` helper checks the model is a `CritterEntityModel` whose class was loaded by a `CritterClassLoader`. It's used in every test meant to exercise runtime generation, so they fail loudly instead of quietly testing AOT models if `dev.morphia.mapping` is ever added to the test `morphia.packages`. It isn't used in the reflection-fallback, imported-model (`register`), or null/non-entity tests. - **Weak-cache release.** `testRuntimeClassLoaderReleasedWithItsMappers` maps an entity through two sharing mappers (plus a copy) inside a helper that returns only a `WeakReference` to the generated loader, then runs `System.gc()` in a bounded loop (up to 10s) and asserts the reference clears. The config uses a discriminator key no other test uses, so no other mapper in the JVM can share and pin that `RuntimeModels`. This covers both `CritterMapper.RuntimeModels.SHARED` and `NestmateAccessorRegistry`. No leak found: the loader is collected. I checked the test can fail by temporarily holding the mapper in a static field, and the test then failed as expected. - **Concurrency.** `testConcurrentMappingAcrossSharingMappers`: 8 threads, each with its own `CritterMapper` using the same config (a fresh key, so generation actually races), released together by a `CountDownLatch`. All succeed, all models share one generated class, each mapper registers its own model instance. Full core suite: 1301 tests, 0 failures, 0 errors, 16 skipped, with both `-Dmorphia.mapper=critter` and `-Dmorphia.mapper=reflection`. ## Merge order This PR is one of a batch that came out of the critter review issues (#4352–#4359). Suggested merge order: 1. #4361, #4360 (CI only, independent) 2. #4362, then #4366 (#4366 depends on #4362) 3. #4364, #4363, #4365 (+ MorphiaOrg/morphia-docs#20), #4367 (independent, any order) 4. #4368 last (conflicts with #4363 and #4365 in `TestCritterMapper.java`; rebase after they merge) **This PR:** **merge last.** It conflicts with #4363 and #4365 in `TestCritterMapper.java` (all three add tests there); rebase onto master after they merge and re-run the suite.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 14:28

Copilot started reviewing on behalf of evanchooly October 5, 2026 14:33

View session

This was referenced Oct 5, 2026

Strengthen critter mapper tests: tier assertions and weak-cache release #4368

Merged

Document critter runtime generation's class loader requirement MorphiaOrg/morphia-docs#20

Open

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Count only this thread's warnings in the class loader fallback test

c6401d1

Test classes run concurrently, so another class's CritterMapper warning could reach the test's appender while it's attached.

evanchooly merged commit a8ff65a into master Oct 6, 2026
58 checks passed

evanchooly deleted the critter-runtime-loader-warning branch October 6, 2026 03:23

evanchooly added this to the 3.0.0 milestone Oct 6, 2026

Navigation Menu

Repository navigation

Name the cause when critter runtime generation can't access an entity - #4365

Name the cause when critter runtime generation can't access an entity#4365
evanchooly merged 2 commits into
masterfrom
critter-runtime-loader-warning

evanchooly commented Oct 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

evanchooly Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

evanchooly commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge order

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

evanchooly Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

evanchooly commented Oct 5, 2026 •

edited

Loading