GORM Native Datastore Id Support - #16222
Conversation
A domain class that declares no id has always been given a Long one. That
is right for Hibernate and wrong for MongoDB, where a String id holding a
generated ObjectId needs no sequence collection and shards cleanly.
Declaring String id gets that, but ties the source to MongoDB: the same
class compiled against Hibernate would need Long id instead.
Add a build setting that asks each domain class for the identity type of
the GORM implementation it is mapped with:
grails {
gorm {
defaultIdType = 'native'
}
}
GormEntityTransformation already resolved that implementation at compile
time, in pickGormEntityTrait, from mapWith plus the GormEntityTraitProviders
on the compilation classpath. That resolution now also supplies the identity
type, through a new default method on the SPI, so the trait and the id type
come from a single decision and cannot disagree. MongoEntityTraitProvider
returns String; Hibernate and Neo4j inherit Long.
The setting reaches the compiler as a system property published by the
Gradle plugin, the same way grails { compileStatic { } } already does, with
the effective value exposed as an @input so that changing it invalidates the
compile task. The default is 'long', which is the behaviour of every earlier
release.
EntityASTTransformation now runs the discovered domain injectors before
DefaultGrailsDomainClassInjector. It ran that injector first, and it
unconditionally adds a Long id, so GORM never got to decide the type on the
@grails.persistence.Entity path. Each of the default injector's injections
is guarded on the property not already being present, so it still fills in
everything GORM did not.
An action parameter typed Serializable was treated as a command object type. Being an interface it could not be one, so it produced the warning "Interface types and abstract class types are not supported as command objects. This parameter will be ignored" and bound to null. Serializable is the type a domain class identifier is declared as when the action does not know the type itself - Long under Hibernate, String under MongoDB - and it is what GormEntity.get(Serializable) accepts. Bind it the way a String parameter is bound: the raw request parameter, which is a String and so a Serializable. GORM converts it to the identity type on the way into get(), returning null for a value that cannot be converted, so a non-numeric id against a Long-id domain still gives a 404 rather than an error. The dispatch compares the declared type exactly rather than testing assignability, so a command object that implements Serializable - as many do - is still data bound as a command object.
The generated controllers declared show, edit, update and delete as taking a Long id, which is only right for a domain class whose identifier is a Long. A MongoDB domain class declaring String id - the form the GORM for MongoDB guide recommends - bound null instead and every one of those actions returned notFound(). Declare the parameter Serializable, which binds the raw request value and lets GORM convert it to whatever the domain class declares: Long, String or ObjectId. The generated services already declared get(Serializable id) and delete(Serializable id), so this makes the controller agree with the service it calls.
codeconsole
changed the title
The provider was tested in isolation and the entity transformation was
tested against the system property, but nothing exercised the step between
them: the plugin attaching the provider to compileGroovy. Deleting that line
left every test passing while the setting stopped reaching the compiler and
every domain class silently kept a Long id.
Add a TestKit consumer project that states the setting the way an
application does, through grails { gorm { defaultIdType } }, and reads the
compiler worker JVM arguments back off compileGroovy. Removing the wiring
now fails two of its four cases.
The Grails Gradle plugin publishes the property name from BuildSettings and the entity transformation reads it from a constant of its own. They are in separate Gradle builds that cannot reference each other, so the name is duplicated - as the compileStatic artefact opt-ins already duplicate theirs. Nothing stopped the two drifting apart. The Gradle side asserts the literal already; the transformation side referred to its constant symbolically, so renaming that constant broke the feature with every test still passing, and a build publishing a name the compiler no longer reads fails silently. Assert the literal on the transformation side too, so a rename on either side fails.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16222 +/- ##
==================================================
+ Coverage 54.7116% 54.7268% +0.0151%
- Complexity 20402 20429 +27
==================================================
Files 2098 2101 +3
Lines 100931 100978 +47
Branches 17900 17907 +7
==================================================
+ Hits 55221 55262 +41
- Misses 37847 37852 +5
- Partials 7863 7864 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for this — commits 2 and 3 (binding a Serializable action parameter, and generating scaffolded controllers with Serializable id) look like solid, independent bug fixes. Both hit any domain class that already follows the current GORM for MongoDB guide (i.e. one that already declares String id), so I'd support merging those regardless of what happens below.
My concern is with commit 1 (defaultIdType): the type of an id already has a knob, and it's the declaration in the domain class itself. String id is one line, per-entity, visible in source, works under IDE compilation (which this system-property mechanism doesn't, per the PR's own limitations), and is what the GORM for MongoDB guide has recommended all along. The implicit-Long path this compensates for only works because GORM for MongoDB fakes auto-increment through an internal sequence/counter collection — the same shard-hostile bottleneck this PR's description calls out. As far as I can tell, the only concrete behavior change here is that an undeclared id on a MongoDB-mapped class gets String instead of Long; Hibernate's and Neo4j's trait providers are untouched.
So the effect of the new SPI method (getDefaultIdentityType()), the Gradle plugin class, and the system property threaded through the compiler is to let the build config supply, invisibly and at a distance, a fact that belongs in the source of truth — at the cost that reading the domain class no longer tells you its id type (you need the build config plus the store the class resolves to), plus the IDE/Gradle divergence and the plugin-jar freezing noted in the limitations. Unlike id generator:, which is a real degree of freedom the type can't express, the id type has an existing, better home.
I'd rather see a compile-time warning when a MongoDB-mapped domain class has no declared id — nudging the developer to write String id (or whatever they intend) explicitly — than machinery that makes the omission work.
Requesting changes on commit 1 pending discussion of the above; happy to be convinced there's a use case that needs automatic inference rather than a warning-and-explicit-declare approach.
@borinquenkid I think the use case is that a domain class in a plugin can be used by both hibernate and mongodb. |
|
@borinquenkid The key use case is reusable domain classes supplied by a plugin. Such a class cannot declare |
|
@matrei @borinquenkid I guess the only weakness in this design is that it does not work for binary plugins, but this wasn't my intention. The plugins I use this for are built with the application. I am going to explore the options for binary plugins. I am looking at... bundling source, building variants, serializable id support, app side ast transformations, etc. |
borinquenkid
left a comment
borinquenkid
left a comment
There was a problem hiding this comment.
One naming nit before merge: the new runtime property grails.gorm.default.idType sits a little too close to the existing compile-time DSL grails { gorm { defaultIdType } } (which is itself published to the compiler as grails.compile.gorm.default.id.type) — three similarly-shaped names for two different mechanisms invites someone transposing them from memory, and both default to 'long' so the mistake would fail silently. Since the code already has a name for this concept internally (portableIdentityType, resolvePortableIdentityType, "portable identity" in the docs), I'd rename the property to grails.gorm.default.portableIdType and let that vocabulary do the disambiguating work instead of "default id type" meaning two different things.
Approving with that suggestion — not blocking.
|
@borinquenkid I agree on the confusion. I just collapsed all 3 into the same settings and they are applied in the context in which they are used. gradle: compile, runtime: runtime. |
This comment has been minimized.
This comment has been minimized.
@codeconsole If this does not work with binary plugins, I think the feature becomes of limited use. It would be great if your investigation uncovers a solution for that. Otherwise, I'm hesitant to add the feature, since I don't think we should introduce different behavior depending on how a plugin is distributed. |
@matrei you are reading an old message without seeing the updates after. It works in binary plugins now with the commit "Support native IDs for compiled plugin domains" |
3 participants
A domain class that declares no
idhas always been given aLongone. That is right for Hibernate and wrong for MongoDB, where aString idholding a generatedObjectIdneeds no sequence collection and shards cleanly. DeclaringString idgets that, but ties the source to MongoDB — the same class compiled against Hibernate would needLong idinstead.This adds a build setting that lets each domain class take the identity type of the GORM implementation it is actually mapped with:
Compiled with GORM for MongoDB,
Personis given aString id. Compiled with Hibernate, it keepsLong. In an application using both, each domain class gets the right type from the single setting, resolved from itsmapWithproperty. A domain class that declares anidkeeps the type it declares.The default is
defaultIdType = 'long', which is the behaviour of every earlier release.Commit 1 — the identity type
How it resolves
GormEntityTransformationalready worked out which GORM implementation an entity belongs to, at compile time, inpickGormEntityTrait— frommapWithplus theGormEntityTraitProviders on the compilation classpath. That resolution now also supplies the identity type, through a new default method on the SPI:MongoEntityTraitProviderreturnsString. Hibernate and Neo4j inheritLong. Since the trait and the identity type now come from a single resolution, the two cannot disagree.The setting reaches the compiler as a system property published by the Gradle plugin, the same way
grails { compileStatic { } }already does — aCommandLineArgumentProvideronGroovyCompile.groovyOptions.forkOptions.jvmArgumentProviders, with the effective value exposed as an@Inputso that changing it invalidates the compile task. A stale class file would otherwise keep the type the previous setting asked for.EntityASTTransformationnow runs the discovered domain injectors beforeDefaultGrailsDomainClassInjector. It ran that injector first, and it unconditionally adds aLongid, so GORM never got to decide the type on the@grails.persistence.Entitypath. Each of the default injector's injections is guarded on the property not already being present, so it still fills in everything GORM did not.Limitations
Longids.Long.Commit 2 — bind a
Serializableaction parameterAn action parameter typed
Serializablewas treated as a command object type. Being an interface it could not be one, soControllerActionTransformerwarned "Interface types and abstract class types are not supported as command objects. This parameter will be ignored" and boundnull.Serializableis the type a domain class identifier is declared as when the action does not know the type itself, and it is whatGormEntity.get(Serializable)accepts. It is now bound the way aStringparameter is — the raw request value, which is aStringand so aSerializable. GORM converts it to the identity type on the way intoget(), returningnullfor a value that cannot be converted, so a non-numeric id against aLong-id domain still gives a 404 rather than an error.The dispatch compares the declared type exactly rather than testing assignability, so a command object that implements
Serializable— as many do — is still data bound as a command object. That case is covered by a test.Commit 3 — generate scaffolded controllers with a
SerializableidThe generated controllers declared
show,edit,updateanddeleteas taking aLong id. A MongoDB domain class declaringString id— the form the GORM for MongoDB guide recommends — boundnullinstead, and every one of those actions returnednotFound(). That is a pre-existing bug, independent of the setting above.They now declare
Serializable id, which works forLong,StringandObjectIddomain classes alike. The generated services already declaredget(Serializable id)anddelete(Serializable id), so this makes the controller agree with the service it calls. Applies to both thegrails-scaffoldingtemplates and therest-apiprofile.Documentation
grails-data-mongodb— new Native Identity Types section under Identity Generationgrails-doc— What's New in 8.0