matrei commented on PR #16468:
URL: https://github.com/apache/grails-core/pull/16468#issuecomment-5954730300
Post-merge review of f15d4688dd (base `8.0.x` at d13aa33282, merged as
7102307bec). No later commit on `8.0.x` touches `MappingFactory`.
What I ran at the PR head:
- `:grails-datastore-core:test` (310 tests), `:grails-data-simple:test`
(15), `MongoMappingContextSpec` (7, no server needed),
`SnowflakeIdGeneratorSpec` (Neo4j, 2): all pass.
- With only the `MappingFactory` change reverted,
`IdentityGeneratorMappingSpec` fails 3 of 6 and `SimpleMapIdentifierSpec` fails
7 of 7 with `No enum constant
ValueGenerator.EXAMPLE.CUSTOMIDENTIFIERGENERATOR`. So the new specs do pin the
fix.
- A throwaway `DataTest` spec in `grails-test-suite-persistence` mocking a
domain with `id generator: 'com.example.CustomIdGenerator'`: without the fix it
fails in setup (`BeanCreationException: Error creating bean with name
'grailsDatastore'`). With the fix, `save()`/`get()` work.
The fix is correct and small. Moving the fallback into the base class and
dropping the Neo4j copy is the right call. None of the points below needs a
revert; they are follow-ups.
## Follow-ups
### 1. No test goes through `DataTest`, the API the bug report is about
The PR describes a failure in `DataTest`, `DomainUnitTest` and controller
unit tests. But the regression specs drive `SimpleMapDatastore` and the mapping
contexts directly. If the `DataTest` setup path ever builds its mapping
differently (its own factory, a different mapping context), these specs would
stay green while users break again. I've confirmed above that a plain
`DataTest` spec reproduces the original failure and passes with the fix.
Something like this in `grails-test-suite-persistence` (or the `demo33`
functional app the guide already includes from) would cover the user-facing
case:
```groovy
class CustomIdGeneratorDataTestSpec extends Specification implements
DataTest {
Class[] getDomainClassesToMock() { [CustomIdGeneratorBook] as Class[] }
void 'a domain class mapped with a generator class name can be mocked
and saved'() {
when:
def book = new CustomIdGeneratorBook(title: 'Grails').save(flush:
true, failOnError: true)
then:
book.id != null
CustomIdGeneratorBook.get(book.id).title == 'Grails'
}
}
@Entity
class CustomIdGeneratorBook {
String id
String title
static mapping = {
id generator: 'com.example.CustomIdGenerator'
}
}
```
### 2. A misspelled generator name now passes unit tests but fails at
startup on Hibernate
`MappingFactory.resolveGenerator` maps every unknown name to `CUSTOM`. The
Hibernate 5 (`HibernateMappingContext.java:255`) and Hibernate 7
(`HibernateMappingFactory.groovy:204`) factories only do that when the name is
a loadable class (or `table` on H7), and otherwise throw
`DatastoreConfigurationException("Invalid id generation strategy ...")`. So `id
generator: 'identiy'` mocks fine in a `DataTest` and then fails when the
application boots. Grails 7 behaved the same way (it never read the generator
on that path), so this isn't a regression, and the doc note already points
people to an integration test. It might be worth saying so explicitly in the
note, e.g. "...and never runs or validates that generator".
Related, optional: the valueOf/upper-case/catch block now exists three times
(base, H5, H7). Making `resolveGenerator` `protected` with an overridable hook
for the unknown-name case would let the Hibernate factories add only their
class-presence check.
### 3. Doc note placement and wording
`unitTestingDomainClasses.adoc:48`: the `NOTE` sits between "Alternatively,
the `DataTest` trait may be used..." and "Another way to express which domain
classes should be mocked...". Those two paragraphs are one sequence of ways to
declare the mocked classes, and the note interrupts it. It would read better at
the end of the section, after the `ControllerUnitTest` example.
"The mocked datastore assigns its own identifiers" also isn't quite true for
`generator: 'assigned'`, where the supplied id is kept (the PR's own
`IdAssigned` test checks this). "generates identifiers itself instead of
running that generator" keeps the point without that ambiguity.
### 4. Nit
`IdentityGeneratorMappingSpec`: `@Unroll` is the default in Spock 2.x and
can be dropped.
## Verified as correct
- `Locale` was already imported in `MappingFactory`, so the switch from
`java.util.Locale.ENGLISH` is fine.
- The removed Neo4j `createDefaultIdentityMapping` override did exactly what
the new base does: same identifier names, case-insensitive lookup, `CUSTOM`
fallback, `AUTO` for no name. `GraphPersistentEntity` keeps reading the raw
name from the mapped form, so `snowflake` and generator classes still resolve,
as `SnowflakeIdGeneratorSpec` confirms.
- `AbstractGormMappingFactory.getIdentityMappedForm` and
`GormKeyValueMappingFactory` both route to the changed method, so key-value
(simple map) and document (Mongo) contexts both get the fallback. No other
subclass overrides it.
- The only reader of `IdentityMapping.getGenerator()` outside Hibernate is
`AbstractRxDatastoreClient`, which checks only `NATIVE` and `ASSIGNED`.
`CUSTOM` falls through to normal id generation there, the same as `AUTO`.
`EntityPersister.isAssignedId` and Mongo's assigned check still use the raw
mapped-form string, so they are unaffected.
- Hibernate 5/7 override `createIdentityMapping` themselves and never reach
this code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
