jdaugherty commented on code in PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#discussion_r3816555407
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -33,25 +33,25 @@ class HalEmbeddedSpec extends Specification implements
JsonViewTest {
JsonMapper objectMapper = JsonMapper.builder().build()
void setup() {
- mappingContext.addPersistentEntities(Team, Player)
+ mappingContext.addPersistentEntities(HalTeam, HalPlayer)
Review Comment:
This is the clearest place to raise it: I don't think the mapping-context
rationale holds, and I checked rather than reasoned about it.
`JsonViewTest.mappingContext` is a plain trait property, not `@Shared`:
```groovy
//
grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:67
@Autowired(required = false)
MappingContext mappingContext = {
def ctx = new KeyValueMappingContext('test')
ctx.setCanInitializeEntities(true)
return ctx
}()
```
Groovy initialises trait properties from `$init$` in the implementing
class's constructor, and Spock instantiates the spec once per feature method.
So every feature gets its own `KeyValueMappingContext`, and this `setup()`
re-registers the same two `Class` objects into a fresh one each time.
I confirmed it with a throwaway spec implementing `JsonViewTest` — three
features, `setup()` calling `addPersistentEntities`, recording
`System.identityHashCode` of both the context and the resulting
`PersistentEntity`:
```
PROBE distinct mappingContexts=3 distinct PersistentEntities=3
```
Three features, three independently-built contexts, three distinct
`PersistentEntity` instances for one `Class`.
So "no class is registered into two independently-built mapping contexts"
isn't an invariant this codebase has, and it isn't one the renames can
establish. Post-rename, `HalEmbeddedSpec` registers `HalTeam`/`HalPlayer` into
nine independently-built contexts on its own (nine features, all through this
`setup()`); `IterableRenderSpec`, `NullRenderingSpec` and `MapRenderSpec` do
the same explicitly, per feature, in this very diff. Whether a *second spec*
also registers the class isn't a categorical change — it's N versus 2N of
something that already happens by design.
That moves the stated root cause from "unconfirmed", where the description
currently leaves it, to "contradicted by the PR's own code". I'd rather the
description drop the flakiness framing than leave a hypothesis in the permanent
history that the diff itself disproves.
What still stands on its own merits is narrower: the template strings
previously did `import grails.plugin.json.view.*` into a package holding 20+
specs' fixtures, and the specs referenced `Player`/`Team` unqualified. Both are
genuinely fragile and worth closing. But that's a readability and robustness
argument — which is exactly what the per-spec sub-package suggestion on
`IterableRenderSpec` buys, at a fraction of the churn.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/IncludeAssociationsSpec.groovy:
##########
@@ -44,7 +45,29 @@ json jsonapi.render(players, [associations: false])
}
then: "The result is an array"
- renderResult.jsonText ==
'{"data":[{"type":"player","id":"1","attributes":{"name":"Cantona"}}],"links":{"self":"/foo"}}'
+ renderResult.jsonText ==
'{"data":[{"type":"includeAssociationsPlayer","id":"1","attributes":{"name":"Cantona"}}],"links":{"self":"/foo"}}'
}
}
+
+@Entity
+class IncludeAssociationsTeam {
Review Comment:
Two more borrows the sweep hasn't reached, beyond the `Address` and `Child2`
cases I flagged last time:
- `TemplateInheritanceSpec` constructs `new Player(...)` in six places and
`new Circular(...)` in `"test circular rendering is handled"`, both binding
unqualified to `JsonViewHelperSpec`'s `@Entity` classes
(`JsonViewHelperSpec:672` and `:690`). Those are the same `Team`/`Player`
fixtures the rest of this PR is decoupling, so it's the last remaining
`@Entity` case in the package.
- `PaginationSpec` (`grails.plugin.json.view.api`) imports
`grails.plugin.json.view.Book`, which is `JsonViewTemplateEngineSpec`'s
`@Linkable` POGO — while `JsonApiSpec` declares its own `Book` in
`PaginationSpec`'s own package. Only the explicit import disambiguates the two;
remove it and the reference silently flips to a different class. Lower stakes
than the entity cases, but the same shape.
I'm not asking for six more renames on top of these — the opposite.
Enumerating them is the argument for the sub-package approach, which would
cover every case in the package mechanically and without touching a single
expected-JSON string.
--
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]
