jdaugherty commented on code in PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#discussion_r3724913097
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -421,16 +421,17 @@ class HalEmbeddedSpec extends Specification implements
JsonViewTest {
void 'test hal embedded with associations that have GORM embedded
properties'() {
given: 'A domain class with embedded associations'
- mappingContext.addPersistentEntities(Person, Parent)
- def p = new Person(name: 'Robert')
+ mappingContext.addPersistentEntities(HalPerson, Parent)
+ def p = new HalPerson(name: 'Robert')
p.homeAddress = new Address(postCode: '12345')
Review Comment:
The `Person` → `HalPerson` rename is right, but `Address` is still reaching
into another spec by exactly the mechanism this PR is closing: it's declared in
`EmbeddedAssociationsSpec` (line 190) and picked up here unqualified via
same-package resolution.
It matters for the same reason `Person` did. `Address` is the embedded type
of both `Person` and `HalPerson`, so `GormMappingConfigurationStrategy` calls
`context.createEmbeddedEntity(Address)` — see
`AbstractMappingContext#createEmbeddedEntity`, which builds a fresh
`EmbeddedPersistentEntity(type, this)` bound to the calling context — once for
this spec and once for `EmbeddedAssociationsSpec`. That's the identical `Class`
object wrapped by two independently-built mapping contexts, which is the
condition the rest of the PR eliminates.
Please give this spec its own `HalAddress` alongside `HalPerson`. It's a
two-line change in a file you're already editing.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy:
##########
@@ -339,3 +340,25 @@ class IterableRenderSpec extends Specification implements
JsonViewUnitTest {
ex.message == 'Error rendering view: JSON API pagination arguments
must contain resource and total'
}
}
+
+@Entity
+class IterableRenderTeam {
Review Comment:
Design question on the approach as a whole, anchored here because this file
shows the cost most clearly.
Prefixing the class names forces every expected-JSON string in the spec to
change, and that churn is most of the +363/−219. Per-spec sub-packages would
buy the same isolation for almost none of it: the JSON API `type` comes from
`PersistentEntity.decapitalizedName` (`DefaultJsonApiViewHelper:183`) and HAL
hrefs from `GrailsNameUtils.getPropertyName(clazz)` (`TestLinkGenerator:72`) —
both the *simple* name. So `grails.plugin.json.view.iterable.Player` renders
byte-identically to today's `Player`, distinct `Class` object and all, and
every assertion in the file stays untouched.
Two reasons I lean that way:
- Rewritten assertions lose their regression value. If name derivation
itself regressed, the old strings would catch it; the new ones were written to
match current output.
- It's self-enforcing. Nothing in this PR stops the next spec added to
`grails.plugin.json.view` from typing `new Player(...)` and silently binding to
`JsonViewHelperSpec` all over again. With per-spec packages that doesn't
compile.
There's no template fallout to worry about: the module's only `.gson`
fixtures live under `grails-app/views` and none of them are named for `player`
or `team`.
This is a rework of a rename you've already done twice, so I'll leave the
call to you. If you keep the prefixes, please add a line of comment above each
duplicated fixture block saying why it's duplicated — otherwise someone will
helpfully consolidate the seven copies back into one shared pair and
reintroduce the problem.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -330,14 +330,14 @@ class HalEmbeddedSpec extends Specification implements
JsonViewTest {
void 'test hal embedded method for one-to-many associations'() {
when: 'A GSON view that renders hal.embedded(..) is rendered'
- def player = new Player(id: 1L, name: 'Cantona')
+ def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
- def captain = new Player(name: 'Keane')
+ def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
Review Comment:
`==` rather than `=`, so this line does nothing — Spock only treats bare
conditions as assertions in `then:`/`expect:`, and this is a `when:` block.
Pre-existing, but you're changing the lines directly above and below it, so
it's free to fix here.
Worth noting the expected JSON further down asserts `"href":
"http://localhost:8080/halPlayer"` with no id, i.e. the captain genuinely has
no id and the feature is passing for the right reason. So the fix is to delete
this line rather than turn it into an assignment — unless you'd rather set the
id and update the expected href to `/halPlayer/1`.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/NullRenderingSpec.groovy:
##########
@@ -110,15 +111,37 @@ class NullRenderingSpec extends Specification implements
JsonViewTest {
model {
Object obj
}
-
+
json g.render(obj, [renderNulls: true])
'''
when:
- mappingContext.addPersistentEntity(Player)
+ mappingContext.addPersistentEntity(NullRenderingPlayer)
def renderResult = render(templateText, [obj: new Child2()])
then:
objectMapper.readTree(renderResult.jsonText) ==
objectMapper.readTree('{"name": null, "parent": null}')
}
+}
+
+@Entity
+class NullRenderingTeam {
+ String name
+ NullRenderingPlayer captain
+ List players
+ List<String> titles
+ @SuppressWarnings('unused')
+ static hasMany = [players: NullRenderingPlayer]
+}
+
+@Entity
+class NullRenderingPlayer {
+ Long version
+ String name
+ @SuppressWarnings('unused')
+ static belongsTo = [team: NullRenderingTeam]
+
+ static constraints = {
+ name nullable: false
+ }
}
Review Comment:
Nit: still missing the trailing newline at EOF, and this commit rewrites the
tail of the file anyway.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiHandleAssociationsSpec.groovy:
##########
@@ -72,3 +72,8 @@ class Publisher {
String name
}
+@Entity
+class HandleAssociationsAuthor {
+ String name
+}
+
Review Comment:
Nit: trailing blank line at EOF.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/NullRenderingSpec.groovy:
##########
@@ -92,12 +93,12 @@ class NullRenderingSpec extends Specification implements
JsonViewTest {
model {
Object obj
}
-
+
json g.render(obj)
'''
when:
- mappingContext.addPersistentEntity(Player)
+ mappingContext.addPersistentEntity(NullRenderingPlayer)
def renderResult = render(templateText, [obj: new Child2()])
Review Comment:
Same pattern as the `Team`/`Player` borrowing the rest of this PR fixes:
`Child2` is declared in `PogoDeepRenderingSpec` and reached here unqualified.
Lower stakes than the entity cases — `Child2` is a plain POGO, so nothing
registers it into a mapping context — but it's the same silent binding, in a
file you're already changing. A `NullRenderingChild` local to this spec closes
it.
--
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]
