codeconsole commented on code in PR #16694:
URL: https://github.com/apache/grails-core/pull/16694#discussion_r4332508958
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -1592,6 +1592,32 @@ Subscript assignment, `attrs['gspTagSyntaxCall'] =
value`, now stores an attribu
NOTE: When adding a class that extends `grails.util.TypeConvertingMap`, do not
declare a no-argument `getX()` or `isX()` method on it, or the entry `x`
becomes unreachable for the same reason. Statically compiled callers bind
directly to the accessor, so the collision cannot be corrected at runtime.
+===== 28.4 Map entries named `properties` or `metaClass`
+
+The accessor-first resolution described above applies to every `Map`, not only
to a class that declares an accessor of its own, because every Groovy object
has the `properties` and `metaClass` properties.
+In Groovy 4 `map.properties` and `map['properties']` read the map entry.
+In Groovy 5 both return the Groovy meta-properties of the map object, in
dynamic and in statically compiled code alike, and `map['properties'] = value`
throws `groovy.lang.ReadOnlyPropertyException`.
+`metaClass` behaves the same way, and assigning to it replaces the metaclass
of the map.
Review Comment:
Assigning `metaClass` through a subscript was never a map put, so that part
didn't change in Groovy 5. On 4.0.33, `map['metaClass'] = 'x'` already throws
`GroovyCastException`, in dynamic code, in statically compiled code
(`Map<String, Object>` with a `String` key) and with an `Object`-typed key.
With a `MetaClass` value it replaces the metaclass, as on 5.1.3. What Groovy 5
did change for `metaClass` is the read: `map['metaClass']` and `map.metaClass`
return the entry on 4.0.33 and the metaclass on 5.1.3. The table in the PR
description has the same slip: its Groovy 4 column says `map['metaClass'] = v`
is a map put.
So on 7.x, the same `map[key] = value` sites this PR rewrites already throw
for a `metaClass` key. Only the `properties` half of #16675 is new in Groovy 5.
Not a blocker. Your call whether the `metaClass` part is worth a 7.x backport.
Suggested wording:
```suggestion
`metaClass` reads the same way. Assigning to it through a subscript was
never a map put: in Groovy 4 as in Groovy 5 it sets the metaclass of the map,
and a value that is not a `MetaClass` throws `groovy.lang.GroovyCastException`.
```
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -1592,6 +1592,32 @@ Subscript assignment, `attrs['gspTagSyntaxCall'] =
value`, now stores an attribu
NOTE: When adding a class that extends `grails.util.TypeConvertingMap`, do not
declare a no-argument `getX()` or `isX()` method on it, or the entry `x`
becomes unreachable for the same reason. Statically compiled callers bind
directly to the accessor, so the collision cannot be corrected at runtime.
+===== 28.4 Map entries named `properties` or `metaClass`
+
+The accessor-first resolution described above applies to every `Map`, not only
to a class that declares an accessor of its own, because every Groovy object
has the `properties` and `metaClass` properties.
+In Groovy 4 `map.properties` and `map['properties']` read the map entry.
+In Groovy 5 both return the Groovy meta-properties of the map object, in
dynamic and in statically compiled code alike, and `map['properties'] = value`
throws `groovy.lang.ReadOnlyPropertyException`.
+`metaClass` behaves the same way, and assigning to it replaces the metaclass
of the map.
+`class` and `empty` are exempt and still address the map entry.
+
+Such keys are common in data an application does not control; the `properties`
member of a GeoJSON `Feature` is one.
+Grails 8 stores and loads them through the `Map` API, so they round-trip
through a `Map` property or an embedded map of a GORM for MongoDB entity, and
through data binding of a `Map` property, whether the value arrives as a nested
map or as an indexed key such as `attributes[properties]`.
+
+Application code that reads one of these keys from a `Map` must call `get`:
+
+[source,groovy]
+----
+Map feature = place.geoJson // a Map property holding a GeoJSON Feature
+
+feature.get('properties') // the entry
+feature.properties // the meta-properties of the map object
+feature['properties'] // the same, on Groovy 5
+----
+
+The map a loaded GORM `Map` property comes back as is a dirty-checking wrapper
with accessors of its own, named `target`, `parent`, `property`, `originalSize`
and `assigned`.
+The wrapper resolves those five names to the map entry, in dotted and
subscript form, so an entry such as `parent` reads and writes through the
wrapper as it does through the plain map, and as it did in Grails 7.
+The one form that still reaches the accessor is dotted access from statically
compiled code, which binds at compile time as it always has; read such an entry
with `get` there.
Review Comment:
This paragraph is only true for GORM for MongoDB. `DirtyCheckingMap` comes
from the Mongo BSON decoders, and from `DirtyCheckingSupport.rewrap` once a
value is already wrapped. With GORM for Hibernate, the default, a loaded `Map`
property is Hibernate's `PersistentMap`. Its own accessors hide entries of the
same names on Groovy 5, and nothing redirects them.
I checked it with a throwaway spec in `grails-data-hibernate7-core`. It
saved a `Map<String, String> attrs` with `[key: 'K', value: 'V', owner: 'O',
role: 'R', size: 'S', name: 'N']`, cleared the session and reloaded it (the
class is `org.hibernate.collection.spi.PersistentMap`):
| `k` | `attrs.get(k)` | `attrs."$k"` and `attrs[k]` |
|---|---|---|
| `key` | `K` | `1` (the owner's id) |
| `value` | `V` | the map itself |
| `owner` | `O` | the owning entity |
| `role` | `R` | `'grails.gorm.tests.ZzProbeHolder.attrs'` |
| `size` | `S` | `6` |
| `name` | `N` | `N` |
On Groovy 4.0.33 every one of them reads the entry. Running `Introspector`
on `PersistentMap` (hibernate-core 7.4) also lists `session`, `dirty`,
`storedSnapshot`, `directlyAccessible`, `unreferenced`, `rowUpdatePossible`,
`elementRemoved`, `initializing`, `newlyInstantiated` and `temporalIdentifier`.
`key`, `value` and `size` are everyday keys in user data, so Hibernate
applications are more likely to hit this than Mongo ones.
A wrapper isn't an option for Hibernate, so I'm not asking for a code fix in
this PR. This change does already fix binding `attrs[key]` into an existing
`PersistentMap`, through the `put` calls in the binders. But the note shouldn't
tell a Hibernate user that a loaded `Map` property is the dirty-checking
wrapper, and they need the `get` advice for their own names:
```suggestion
With GORM for MongoDB, a loaded `Map` property comes back as a
dirty-checking wrapper with accessors of its own, named `target`, `parent`,
`property`, `originalSize` and `assigned`.
The wrapper resolves those five names to the map entry, in dotted and
subscript form, so an entry such as `parent` reads and writes through the
wrapper as it does through the plain map, and as it did in Grails 7.
The one form that still reaches the accessor is dotted access from
statically compiled code, which binds at compile time as it always has; read
such an entry with `get` there.
With GORM for Hibernate, a loaded `Map` property is Hibernate's
`PersistentMap`, and its accessors are not redirected.
In Groovy 5, `attrs.key`, `attrs['value']`, `attrs.owner`, `attrs.role`,
`attrs.size`, `attrs.session` and `attrs.dirty`, among others, return the state
of the collection rather than the entry; read such an entry with `get`.
```
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/MapKeysNamedLikeBeanPropertiesSpec.groovy:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.mongo
+
+import grails.persistence.Entity
+import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.bson.Document
+import org.bson.types.ObjectId
+import spock.lang.Issue
+
+/**
+ * A Map property whose entries are named like bean properties of the map, as
the {@code properties}
+ * member of a GeoJSON Feature is, is stored and loaded like any other entry.
Groovy 5 resolves a
+ * String subscript on a Map as property access, so the codecs go through the
Map API instead, and
+ * the dirty-checking wrapper a loaded map comes back as resolves its own
accessor names
+ * ({@code target}, {@code parent}, {@code property}, {@code originalSize},
{@code assigned}) to
+ * the entries.
+ */
+@Issue('https://github.com/apache/grails-core/issues/16675')
+class MapKeysNamedLikeBeanPropertiesSpec extends
GrailsDataTckSpec<GrailsDataMongoTckManager> {
+
+ static final Map FEATURE = [
+ type : 'Feature',
+ geometry : [type: 'Point', coordinates: [100.0d, 0.0d]],
+ properties : [name: 'Test', metaClass: 'nested'],
+ metaClass : 'top',
+ firstKey : 'first',
+ target : 't',
+ parent : 'p',
+ property : 'q',
+ originalSize: 42,
+ assigned : 'yes'
+ ]
+
+ void setupSpec() {
+ manager.registerDomainClasses(GeoFeature)
+ }
+
+ void "a Map property saved with a key named properties is read back with
the entry intact"() {
+ when: "an entity whose map holds keys named like bean properties is
saved and reloaded"
+ new GeoFeature(name: 'saved', geoJson: new
LinkedHashMap(FEATURE)).save(flush: true, failOnError: true)
+ manager.session.clear()
+ GeoFeature loaded = GeoFeature.findByName('saved')
+
+ then: "every entry survives the round trip"
+ loaded.geoJson.get('properties') == [name: 'Test', metaClass: 'nested']
+ loaded.geoJson.get('metaClass') == 'top'
+ loaded.geoJson.get('firstKey') == 'first'
+ loaded.geoJson == FEATURE
+
+ and: "entries named like the accessors of the loaded map's wrapper
read as entries"
+ loaded.geoJson.target == 't'
Review Comment:
These five assertions only exercise the new `getProperty`/`setProperty`
override if `loaded.geoJson` is a `DirtyCheckingMap`. A plain `LinkedHashMap`
has no `target`/`parent`/... accessors and passes them as well. It is the
wrapper today, because `BasicCollectionTypeDecoder` wraps a raw `Map` property,
but nothing here pins that. If decoding ever stopped wrapping, this block would
still pass and no longer test anything. Suggest asserting the premise (plus
`import org.grails.datastore.mapping.dirty.checking.DirtyCheckingMap`):
```suggestion
and: "entries named like the accessors of the loaded map's wrapper
read as entries"
loaded.geoJson instanceof DirtyCheckingMap
loaded.geoJson.target == 't'
```
--
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]
