Phase 5: CritterMapper implementation#4224
Merged
evanchooly merged 14 commits intomasterfrom Apr 20, 2026
Merged
Conversation
Tests cover runtime model generation, caching, copy semantics, null/non-entity handling, and concurrent mapping. Tests intentionally fail to compile until CritterMapper is implemented. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- testCopyHasIndependentDiscriminatorLookup: assert discriminator lookup instances are not same (not just mapper instances) - testConcurrentMappingProducesSingleModel: collect all futures first, shut down pool before assertions - Replace fully-qualified org.testng.Assert.assertNull calls with static import assertNull - Add explicit no-arg constructor to CritterMapperTestEntity Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds CritterMapper as a hybrid mapper that discovers entity models via: 1. Pre-generated models from the classpath (critter-maven AOT output) 2. Runtime Gizmo+VarHandle generation via CritterGizmoGenerator 3. Reflection-based fallback (standard EntityModel) The copy constructor shares immutable CritterEntityModel references across copies while creating independent DiscriminatorLookup instances. mapEntity is synchronized to prevent concurrent registration of the same type. All 8 TestCritterMapper tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erMapper Add a comment on the copy constructor explaining why super(other) cannot be used (it would call new EntityModel() for CritterEntityModel instances) and the resulting known discrepancy that custom converters registered post- construction are not shared with copies. Add a comment on mapEntity explaining why the method is synchronized (race condition in DiscriminatorLookup). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Make copy constructor public with @MorphiaInternal annotation - Add @hidden and @morphia.internal Javadoc tags to both constructors - Change LOG.error to LOG.warn for graceful fallback paths in tryLoadPregenerated and tryRuntimeGeneration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…correct SmallRye list handling SmallRye Config applies converters per-element for List return types, so the converter returning List<T> caused a List<List<...>> result. Changed to return a single PropertyAnnotationProvider<?> and let SmallRye accumulate the list. Also reverted GeneratorsTestHelper to use ManualMorphiaConfig directly to avoid picking up test environment config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verifies that when runtime Gizmo generation fails (simulated via a CritterClassLoader that refuses to load __morphia classes), mapEntity() falls back to a plain EntityModel rather than a CritterEntityModel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the new CritterMapper and wires it into Morphia so users can select critter-based bytecode model generation (with reflection fallback) via configuration.
Changes:
- Add
CritterMapper(three-tier discovery: pre-generated → runtime generation → reflection) and integrate it intoMorphiaDatastore’s mapper selection. - Rename the reflection mapper type from
LEGACYtoREFLECTIONand update defaults/docs accordingly. - Add/adjust tests covering critter mapper behavior (runtime generation, caching, copy semantics, concurrency) and integration config.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/modules/ROOT/examples/complete-morphia-config.properties | Updates example config defaults/values for the mapper selection. |
| critter/critter-integration-tests/src/test/java/dev/morphia/critter/CritterCrudTest.java | Forces critter mapper usage in integration CRUD test setup. |
| core/src/test/java/dev/morphia/mapping/TestCritterMapper.java | Adds a dedicated test suite for CritterMapper behavior and concurrency. |
| core/src/test/java/dev/morphia/mapping/CritterMapperTestEntity.java | Adds a simple @Entity used by critter mapper tests. |
| core/src/main/java/dev/morphia/mapping/MapperType.java | Renames the reflection mapper enum constant and keeps CRITTER option. |
| core/src/main/java/dev/morphia/mapping/CritterMapper.java | Introduces the new hybrid mapper with three-tier discovery + copy semantics. |
| core/src/main/java/dev/morphia/mapping/AbstractMapper.java | Renames the stored classloader field and uses it for package scanning. |
| core/src/main/java/dev/morphia/critter/CritterClassLoader.java | Marks CritterClassLoader as internal/hidden. |
| core/src/main/java/dev/morphia/config/converters/PropertyAnnotationProviderConverter.java | Changes property-annotation-provider conversion behavior. |
| core/src/main/java/dev/morphia/config/MorphiaConfig.java | Updates mapper default to reflection and documents the new enum constant. |
| core/src/main/java/dev/morphia/config/ManualMorphiaConfig.java | Updates default mapper type and adjusts constructor visibility/annotations. |
| core/src/main/java/dev/morphia/MorphiaDatastore.java | Enables MapperType.CRITTER to instantiate CritterMapper. |
Member
Author
Code reviewFound 1 issue:
morphia/core/src/main/resources/sofia.properties Lines 32 to 34 in 1fae66c 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…tor dropped the field on every chained setter call, the update method mutated immutable lists, and MorphiaPropertyAnnotationProvider was duplicated by the getter+update method combination. Also adds TestPropertyAnnotationProviders covering these cases.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CritterMapper extends AbstractMapperwith three-tier entity model discoveryCritterGizmoGenerator(Gizmo + VarHandle)EntityModelcopy()shares immutableCritterEntityModelreferences; clones reflective fallback modelsConcurrentHashMap.newKeySet()(logs once, not on every access)synchronized mapEntityprevents concurrent duplicate-discriminator race conditionCloses #4187
Test plan
TestCritterMapper— 8 tests: runtime generation, collection name, caching, copy semantics, concurrent access, null/non-entity inputsmorphia-coresuite — 1,231 tests, zero failures🤖 Generated with Claude Code