Skip to content

Critter clean-ups: CritterClassLoader.findClass ordering and stale docs - #4367

Merged
evanchooly merged 1 commit into
masterfrom
critter-cleanups
Oct 6, 2026
Merged

Critter clean-ups: CritterClassLoader.findClass ordering and stale docs#4367
evanchooly merged 1 commit into
masterfrom
critter-cleanups

Conversation

@evanchooly

@evanchooly evanchooly commented Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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. Run each JDK test leg on its own JDK #4361, Add a non-blocking CI leg that denies final field mutation (JEP 500) #4360 (CI only, independent)
  2. Don't re-set properties already passed to an entity's constructor #4362, then Test record entities with both mappers #4366 (Test record entities with both mappers #4366 depends on Don't re-set properties already passed to an entity's constructor #4362)
  3. Wrap failures in the woven final-field writer like the runtime accessor #4364, Give mapper copies with different mapping settings their own runtime models #4363, Name the cause when critter runtime generation can't access an entity #4365 (+ Document critter runtime generation's class loader requirement morphia-docs#20), Critter clean-ups: CritterClassLoader.findClass ordering and stale docs #4367 (independent, any order)
  4. Strengthen critter mapper tests: tier assertions and weak-cache release #4368 last (conflicts with Give mapper copies with different mapping settings their own runtime models #4363 and Name the cause when critter runtime generation can't access an entity #4365 in TestCritterMapper.java; rebase after they merge)

This PR: no dependencies; merges cleanly with the rest of the batch.

@evanchooly
Critter clean-ups: CritterClassLoader.findClass ordering and stale docs
1b985aa 
Only mark a class as defined once defineClass succeeds, so a failed define
no longer hides the parent's .class resource. Fix the CritterMapper javadoc
that still described VarHandle accessors, and explain why
NestmateAccessorRegistry must still come from the parent loader.

Fixes #4358
Copilot AI balanced review requested due to automatic review settings October 5, 2026 14:29
This was referenced Oct 5, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Copilot AI reviewed Oct 5, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation matches the stated behavior and includes focused regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes failed class-definition cleanup and updates Critter runtime-generation documentation.

Changes:

  • Marks classes defined only after successful bytecode definition.
  • Adds regression coverage for resource visibility after failure.
  • Corrects hidden nestmate accessor documentation.
File Description
CritterClassLoader.java Fixes definition tracking and registry-loading comments.
CritterMapper.java Corrects runtime-generation Javadocs.
CritterClassLoaderTest.java Tests resource visibility after invalid bytecode.

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

evanchooly added a commit that referenced this pull request Oct 5, 2026
@evanchooly
Run each JDK test leg on its own JDK (#4361)
e00c403 
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.
evanchooly added a commit that referenced this pull request Oct 5, 2026
@evanchooly
Add a non-blocking CI leg that denies final field mutation (JEP 500) (#…
d1da0d0 
…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).
evanchooly added a commit that referenced this pull request Oct 5, 2026
@evanchooly
Don't re-set properties already passed to an entity's constructor (#4362
63eb281 
)

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.
evanchooly added a commit that referenced this pull request Oct 6, 2026
@evanchooly
Give mapper copies with different mapping settings their own runtime …
d3e93eb 
…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.
evanchooly added a commit that referenced this pull request Oct 6, 2026
@evanchooly
Wrap failures in the woven final-field writer like the runtime access…
9e477cd 
…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.
evanchooly added a commit that referenced this pull request Oct 6, 2026
@evanchooly
Name the cause when critter runtime generation can't access an entity (…
a8ff65a 
…#4365)

Fixes #4357

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:

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.
@evanchooly evanchooly added this to the 3.0.0 milestone Oct 6, 2026
evanchooly added a commit that referenced this pull request Oct 6, 2026
@evanchooly
Test record entities with both mappers (#4366)
9cc2a11 
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.
@evanchooly
evanchooly merged commit 882c4d6 into master Oct 6, 2026
58 checks passed
@evanchooly
evanchooly deleted the critter-cleanups branch October 6, 2026 03:30
evanchooly added a commit that referenced this pull request Oct 6, 2026
@evanchooly
Strengthen critter mapper tests: tier assertions and weak-cache relea…
a0266da 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

Small critter clean-ups: CritterClassLoader.findClass ordering and stale docs

2 participants

@evanchooly