-
Notifications
You must be signed in to change notification settings - Fork 950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix @GrailsCompileStatic on domain objects with inner classes #13013
Fix @GrailsCompileStatic on domain objects with inner classes #13013
Conversation
Make sure that for every class a new scope is added to and removed from the stack. Previously a new scope was added for nearly every class but not removed. This had the effect that the wrong (empty) scope (from the inner class) was used for the entity class. This was then missing any of its `mapping`, `constraints` and `namedQueries` closures.
I would like to have some suggestions on the tests. They are covering the changes and would fail without them. But I think there should be better ways. Maybe they could even be integrated into the existing |
@Issue('https://github.com/grails/grails-core/issues/12461') | ||
void 'a domain class marked with @GrailsCompileStatic containing an inner class and a "namedQueries" block'() { | ||
setup: | ||
SomeClass.getNamedQuery('test') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required in contrast to the other tests as the namedQueries
closure seems only to be evaluated when the first named query is accessed and not when the class is compiled.
void 'a domain class marked with @GrailsCompileStatic containing an inner class and a "constraints" block'() { | ||
expect: 'the configuration from the "constraints" closure is available' | ||
SomeClass.constraints instanceof Closure | ||
SomeClass.constraintsClosureCalled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way could be to actually define a constraint, create an entity, call entity.validate()
and check for e.g. SomeClass.getConstrainedProperties().containsKey('some')
or even entity.errors['some']
or something else. But I think this would involve more components that could break in other ways.
|
||
expect: 'the configuration from the "namedQueries" closure is available' | ||
SomeClass.namedQueries instanceof Closure | ||
SomeClass.namedQueriesClosureCalled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way could be to actually define a named query, call SomeClass.getNamedQuery('some')
and verify that the result is not null
. But I think this would involve more components that could break in other ways.
void 'a domain class marked with @GrailsCompileStatic containing an inner class and a "mapping" block'() { | ||
expect: 'the configuration from the "mapping" closure is available' | ||
SomeClass.mapping instanceof Closure | ||
SomeClass.mappingClosureCalled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found a working alternative in a DomainUnitTest
to check any side effect on the entity instance or class based on something defined in a mapping
block . The idea could be to set e.g. table
, version
or comment
.
@puneetbehl Could you have a look and suggest improvements for the tests or mention someone who could? |
…#13013) Make sure that for every class a new scope is added to and removed from the stack. Previously a new scope was added for nearly every class but not removed. This had the effect that the wrong (empty) scope (from the inner class) was used for the entity class. This was then missing any of its `mapping`, `constraints` and `namedQueries` closures.
Backport: Fix @GrailsCompileStatic on domain objects with inner classes (#13013)
Fixes #12461.