This is an automated email from the ASF dual-hosted git repository. borinquenkid pushed a commit to branch fix/flaky-json-view-gson-shared-state in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit 8a4a6eba392c80f45dcac9ed39a644be25f0e5f8 Author: Walter Duque de Estrada <[email protected]> AuthorDate: Tue Jul 21 16:35:35 2026 -0500 Fix flaky grails-views-gson tests caused by shared static test state CI's flaky-test dashboard (apache/grails-core#16030) showed ~20 flaky test methods across JsonViewHelperSpec, ExpandSpec, JsonApiSpec and JsonViewTestSpec, all sharing a common root cause: static state that leaks between specs when several of them execute in the same test JVM/fork. Two concrete leaks were found: 1. ExpandSpec declared top-level `Team`/`Player` classes with no import, which silently resolved (same package, same simple names) to the *compiled `Team`/`Player` classes already defined by JsonViewHelperSpec*. Both specs then registered those identical Class objects into their own, independently-built KeyValueMappingContext instances, so any class-keyed GORM cache populated by one spec could be observed by the other. Fixed by giving ExpandSpec its own distinct `ExpandTeam`/`ExpandPlayer` domain classes (and updating the JSON/HAL assertions, whose type names and URLs are derived from the class name). 2. `org.grails.validation.ConstraintEvalUtils` memoizes the default GORM constraints map in a single JVM-wide static field keyed by `System.identityHashCode(config)`. JsonApiSpec already worked around this for its own SuperHero fixture with hand-rolled setup()/cleanup() logic (plus reflection into Validateable's internal static field), but JsonViewHelperSpec, ExpandSpec and JsonViewTestSpec had no equivalent reset, so a stale cache entry left by whichever spec ran first in a fork could be picked up by the next. Generalized the reset by adding a `cleanup()` fixture method directly to the `JsonViewTest` trait (grails-views-gson/src/main/.../test/JsonViewTest.groovy) that clears the ConstraintEvalUtils cache after every feature, so every spec implementing the trait gets it for free. A companion `cleanupSpec()` tears down any GrailsApplication cached by org.grails.testing.GrailsUnitTest, but only once per spec class (not per-feature): GrailsUnitTest intentionally builds and reuses its GrailsApplication across an entire spec's features, and some traits (e.g. DataTest) register beans into it once per spec, so tearing it down after every feature broke DataTest-based specs (MapRenderSpec) in testing. GrailsUnitTest itself is a test-only dependency that this main-sourceSet trait cannot reference directly, so the call is made dynamically only when the implementing spec actually has it. JsonApiSpec's own setup()/cleanup() was simplified accordingly: the reflection-based Validateable static field hack is replaced with the public `SuperHero.clearConstraintsMapCache()` API (available since 7.1), and the now-redundant ConstraintEvalUtils call is dropped since the trait handles it. Verified empirically (via isolated Groovy/Spock trait-composition probes) that a class overriding a trait-provided cleanup() fails to compile under Groovy 5/Spock 2.4, which is why JsonApiSpec no longer defines cleanup() itself. Also empirically confirmed that Groovy trait static fields are *not* shared across implementing classes (contrary to the initial triage hypothesis) - the actual leak vectors are the two described above. Full :grails-views-gson:test suite (178 tests) passes repeatedly, including reruns with --rerun-tasks and varied --tests subsets/orderings covering all previously-flagged specs plus the other GrailsUnitTest+JsonViewTest specs. codeStyle and aggregateStyleViolations report zero Checkstyle/CodeNarc violations. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../plugin/json/view/test/JsonViewTest.groovy | 39 ++++++++++ .../grails/plugin/json/view/ExpandSpec.groovy | 83 ++++++++++++++-------- .../grails/plugin/json/view/api/JsonApiSpec.groovy | 26 ++----- 3 files changed, 96 insertions(+), 52 deletions(-) diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy index 2069460c13..7daad95cec 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy @@ -21,6 +21,7 @@ package grails.plugin.json.view.test import groovy.json.JsonSlurper import groovy.text.Template +import groovy.transform.CompileDynamic import groovy.transform.CompileStatic import org.springframework.beans.factory.annotation.Autowired @@ -41,6 +42,7 @@ import grails.web.mapping.LinkGenerator import grails.web.mime.MimeUtility import org.grails.datastore.mapping.keyvalue.mapping.config.KeyValueMappingContext import org.grails.datastore.mapping.model.MappingContext +import org.grails.validation.ConstraintEvalUtils /** * A trait that test classes can implement to add support for easily testing JSON views @@ -104,6 +106,43 @@ trait JsonViewTest { return templateEngine }() + /** + * Resets the {@link ConstraintEvalUtils} default-constraints cache after every feature + * method so state from one test cannot leak into the next test that happens to run in the + * same JVM/fork. + * + * <p>{@link ConstraintEvalUtils} memoizes the default GORM constraints map in a single + * JVM-wide static field keyed by the identity of the last {@code Config} seen. Since each + * spec implementing this trait typically builds its own {@code GrailsApplication}/{@code + * Config}, a stale entry left behind by one spec can otherwise be picked up by another. + * This is cheap to recompute, so it is safe to clear after every feature.</p> + */ + void cleanup() { + ConstraintEvalUtils.clearDefaultConstraints() + } + + /** + * Tears down any {@code GrailsApplication} cached by {@code org.grails.testing.GrailsUnitTest} + * once the whole spec has finished, so a later spec running in the same JVM/fork always starts + * from a clean application context. + * + * <p>This is deliberately a {@code cleanupSpec()} (once per spec class), not a per-feature + * {@code cleanup()}: {@code GrailsUnitTest} intentionally builds its {@code GrailsApplication} + * lazily and reuses it across every feature of a spec (it is expensive to build and some + * traits, e.g. {@code DataTest}, register beans into it once per spec). Tearing it down after + * every feature would rebuild it before the next feature could see those spec-scoped beans.</p> + * + * <p>{@code GrailsUnitTest} lives in {@code grails-testing-support-core}, a test-only + * dependency this trait cannot reference directly since it ships as part of the main + * {@code grails-views-gson} artifact, so the call is made dynamically only when present.</p> + */ + @CompileDynamic + void cleanupSpec() { + if (metaClass.respondsTo(this, 'cleanupGrailsApplication')) { + cleanupGrailsApplication() + } + } + /** * Render a template for the given source * diff --git a/grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy b/grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy index 7dfe5befdf..132baf0f6a 100644 --- a/grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy +++ b/grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy @@ -19,6 +19,7 @@ package grails.plugin.json.view import tools.jackson.databind.json.JsonMapper +import grails.persistence.Entity import grails.plugin.json.view.test.JsonRenderResult import grails.plugin.json.view.test.JsonViewTest import org.grails.datastore.mapping.core.Session @@ -32,23 +33,23 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { JsonMapper objectMapper = JsonMapper.builder().build() void setup() { - mappingContext.addPersistentEntities(Team, Player) + mappingContext.addPersistentEntities(ExpandTeam, ExpandPlayer) } void 'Test expand parameter allows expansion of child associations'() { given: 'An entity with a proxy association' def mockSession = Mock(Session) mockSession.getMappingContext() >> mappingContext - mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United') - def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L) + mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United') + def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L) - def player = new Player(name: 'Cantona', team: teamProxy) + def player = new ExpandPlayer(name: 'Cantona', team: teamProxy) def templateText = ''' - import grails.plugin.json.view.* - + import grails.plugin.json.view.ExpandPlayer as Player + @Field Player player - + json g.render(player) ''' @@ -84,15 +85,13 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { given: 'An entity with a proxy association' def mockSession = Mock(Session) mockSession.getMappingContext() >> mappingContext - mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United') - def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L) + mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United') + def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L) - def player = new Player(name: 'Cantona', team: teamProxy) + def player = new ExpandPlayer(name: 'Cantona', team: teamProxy) def templateText = ''' - import grails.plugin.json.view.* - @Field Map map - + json g.render(map) ''' @@ -119,13 +118,13 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { given: 'An entity with a proxy association' def mockSession = Mock(Session) mockSession.getMappingContext() >> mappingContext - mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United') - def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L) + mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United') + def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L) - def player = new Player(name: 'Cantona', team: teamProxy) + def player = new ExpandPlayer(name: 'Cantona', team: teamProxy) def templateText = ''' - import grails.plugin.json.view.* + import grails.plugin.json.view.ExpandPlayer as Player model { Player player } @@ -140,7 +139,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { { "_links": { "self": { - "href": "http://localhost:8080/player";, + "href": "http://localhost:8080/expandPlayer";, "hreflang": "en", "type": "application/hal+json" } @@ -161,7 +160,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { "team": { "_links": { "self": { - "href": "http://localhost:8080/team/1";, + "href": "http://localhost:8080/expandTeam/1";, "hreflang": "en", "type": "application/hal+json" } @@ -171,7 +170,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { }, "_links": { "self": { - "href": "http://localhost:8080/player";, + "href": "http://localhost:8080/expandPlayer";, "hreflang": "en", "type": "application/hal+json" } @@ -185,18 +184,18 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { given: 'An entity with a proxy association' def mockSession = Mock(Session) mockSession.getMappingContext() >> mappingContext - mockSession.retrieve(Team, 9L) >> new Team(name: 'Manchester United') - def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 9L) - def player = new Player(name: 'Cantona', team: teamProxy) + mockSession.retrieve(ExpandTeam, 9L) >> new ExpandTeam(name: 'Manchester United') + def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 9L) + def player = new ExpandPlayer(name: 'Cantona', team: teamProxy) player.id = 3 when: 'The domain is rendered with expand parameters' JsonRenderResult result = render(''' - import grails.plugin.json.view.* + import grails.plugin.json.view.ExpandPlayer as Player model { Player player } - + json jsonapi.render(player, [expand: 'team']) ''', [player: player]) @@ -204,7 +203,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { objectMapper.readTree(result.jsonText) == objectMapper.readTree(''' { "data": { - "type": "player", + "type": "expandPlayer", "id": "3", "attributes": { "name": "Cantona" @@ -212,21 +211,21 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { "relationships": { "team": { "links": { - "self": "/team/9" + "self": "/expandTeam/9" }, "data": { - "type": "team", + "type": "expandTeam", "id": "9" } } } }, "links": { - "self": "/player/3" + "self": "/expandPlayer/3" }, "included": [ { - "type":"team", + "type":"expandTeam", "id": "9", "attributes": { "titles": null, @@ -241,7 +240,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { } }, "links": { - "self": "/team/9" + "self": "/expandTeam/9" } } ] @@ -249,3 +248,25 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest { ''') } } + +@Entity +class ExpandTeam { + String name + ExpandPlayer captain + List players + List<String> titles + @SuppressWarnings('unused') + static hasMany = [players: ExpandPlayer] +} + +@Entity +class ExpandPlayer { + Long version + String name + @SuppressWarnings('unused') + static belongsTo = [team: ExpandTeam] + + static constraints = { + name nullable: false + } +} diff --git a/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy b/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy index afbc012a01..d7d9177475 100644 --- a/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy +++ b/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy @@ -24,39 +24,23 @@ import grails.plugin.json.view.test.JsonRenderResult import grails.plugin.json.view.test.JsonViewTest import grails.validation.Validateable import org.grails.testing.GrailsUnitTest -import org.grails.validation.ConstraintEvalUtils import spock.lang.Shared import spock.lang.Specification class JsonApiSpec extends Specification implements JsonViewTest, GrailsUnitTest { - // Cache the static field helper interface for performance - private static final Class<?> STATIC_FIELD_HELPER = Class.forName('grails.validation.Validateable$Trait$StaticFieldHelper') - @Shared JsonMapper objectMapper = JsonMapper.builder().build() void setup() { - ConstraintEvalUtils.clearDefaultConstraints() - clearConstraintsMapCache(SuperHero) + // Resets SuperHero's own cached constraints map (see Validateable#clearConstraintsMapCache). + // JsonViewTest#cleanup() already resets the shared ConstraintEvalUtils cache after every + // feature, but SuperHero's cache is specific to this spec's own Validateable command + // object, so it is reset here too. + SuperHero.clearConstraintsMapCache() mappingContext.addPersistentEntities(Widget, Author, Book, ResearchPaper) } - void cleanup() { - ConstraintEvalUtils.clearDefaultConstraints() - clearConstraintsMapCache(SuperHero) - } - - /** - * Clears the private static constraintsMapInternal field in the Validateable trait. - */ - private static void clearConstraintsMapCache(Class<?> clazz) { - if (STATIC_FIELD_HELPER.isAssignableFrom(clazz)) { - def setterMethod = clazz.getMethod('grails_validation_Validateable__constraintsMapInternal$set', Map) - setterMethod.invoke(null, (Map) null) - } - } - void 'test simple case'() { given: def theWidget = new Widget(name: 'One', width: 4, height: 7)
