sbglasius commented on code in PR #16411:
URL: https://github.com/apache/grails-core/pull/16411#discussion_r4119005139
##########
grails-converters/src/main/groovy/grails/converters/JSON.java:
##########
@@ -546,7 +547,8 @@ protected Object createNode(Object key, Map valueMap) {
writer.object();
for (Object o : valueMap.entrySet()) {
Map.Entry element = (Map.Entry) o;
- writer.key(String.valueOf(element.getKey()));
//.value(element.getValue());
+ Object elementKey = element.getKey();
+ writer.key(elementKey == null ? "null" :
JsonDateFormat.formatKey(elementKey));
Review Comment:
For a `null` key this writes the literal string `"null"`, but
`MapMarshaller.java` (touched in this same PR, also now calling
`JsonDateFormat.formatKey`) silently skips map entries whose key is `null`
instead. That means the same map with a `null` key serializes differently
depending on whether it goes through the JSON builder DSL (`json.build {
keyed(...) }`) or through `([...] as JSON)`/`MapMarshaller`. Not introduced by
this line specifically (the `null` handling predates this diff), but worth
reconciling now that both paths route through `JsonDateFormat`.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultGrailsJsonViewHelper.groovy:
##########
@@ -487,10 +488,10 @@ class DefaultGrailsJsonViewHelper extends
DefaultJsonViewHelper implements Grail
JsonView jsonView = (JsonView) view
MappingFactory mappingFactory = jsonView.mappingContext?.mappingFactory
if (mappingFactory != null) {
- return mappingFactory.isSimpleType(propertyType) || (value
instanceof Enum) || (value instanceof Map)
+ return mappingFactory.isSimpleType(propertyType) || (value
instanceof Enum) || (value instanceof Map) || hasDateTimeConverter(propertyType)
Review Comment:
This override of `isSimpleType(Class propertyType, value)` checks
`hasDateTimeConverter(propertyType)` (the *declared* type), whereas the base
`DefaultJsonViewHelper.isSimpleType` and the sibling check in
`processSimpleProperty` (`!hasDateTimeConverter(value.getClass())`) both use
the *runtime* value's class.
For a GORM property declared as a supertype/interface (e.g. `Object`) that
at runtime holds a `Month`/`Year`/`Duration` value, this would evaluate
`hasDateTimeConverter` against the non-date declared type and return `false`,
so the property would not be classified as simple and would be traversed as a
complex object instead of going through the registered date/time converter.
Should this use `value?.getClass() ?: propertyType` like the other call sites?
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/JsonViewGenerator.groovy:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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 grails.plugin.json.view
+
+import java.text.SimpleDateFormat
+import java.time.ZoneId
+import java.time.ZonedDateTime
+import java.time.temporal.TemporalAccessor
+import java.time.temporal.TemporalAmount
+
+import javax.xml.datatype.XMLGregorianCalendar
+
+import groovy.json.DefaultJsonGenerator
+import groovy.json.JsonGenerator
+import groovy.transform.CompileStatic
+import org.apache.groovy.json.internal.CharBuf
+
+import org.grails.web.json.JsonDateFormat
+
+/**
+ * The {@link JsonGenerator} of JSON views. It writes date and time values the
same way as
+ * Spring Boot's default Jackson rendering:
+ *
+ * <ul>
+ * <li>{@link Date} and {@link Calendar} values are written by {@link
JsonDateFormat} in the configured
+ * {@code grails.views.json.generator.timeZone} (a UTC instant such as
{@code 2024-06-15T14:30:45.123Z} in the
+ * default {@code GMT}), unless a {@code
grails.views.json.generator.dateFormat} pattern is configured, in which
+ * case they are written with that pattern, time zone and locale.</li>
+ * <li>{@link Date} and {@link Calendar} map keys are written the same way
as those values, and
+ * {@link ZonedDateTime} map keys as {@link
JsonDateFormat#formatKey(Object)} does, rather than with
+ * their {@code toString()}. Keys that format to the same text are all
written, as Jackson writes them.</li>
+ * </ul>
+ *
+ * @since 8.0
+ */
+@CompileStatic
+class JsonViewGenerator extends DefaultJsonGenerator {
+
+ private final boolean springBootDates
+
+ /**
+ * @param options the generator options
+ * @param springBootDates whether to write Date and Calendar values with
{@link JsonDateFormat}
+ * rather than the date format of the options
+ */
+ JsonViewGenerator(JsonGenerator.Options options, boolean springBootDates) {
+ super(options)
+ this.springBootDates = springBootDates
+ }
+
+ /**
+ * Whether values of the type are dates or times that one of the
converters of this generator writes, such as the
+ * date and time converters of JSON views, so that {@code g.render} writes
them as it writes other simple values.
+ * A converter that an application registers for any other type does not
change how {@code g.render} renders it.
+ *
+ * @param type a value type
+ * @return whether values of the type are dates or times that a converter
of this generator writes
+ */
+ boolean hasDateTimeConverter(Class<?> type) {
+ isDateTimeType(type) && findConverter(type) != null
+ }
+
+ private static boolean isDateTimeType(Class<?> type) {
+ TemporalAccessor.isAssignableFrom(type) ||
TemporalAmount.isAssignableFrom(type) ||
+ ZoneId.isAssignableFrom(type) ||
TimeZone.isAssignableFrom(type) || Date.isAssignableFrom(type) ||
Review Comment:
`isDateTimeType` checks `Date.isAssignableFrom(type)` but doesn't include
`Calendar`, even though `Calendar` is treated symmetrically with `Date`
everywhere else in this file (`formatMapKey`, `hasDateKey`/`isDateKey`,
`formatDate`). As written, `hasDateTimeConverter(Calendar)` always returns
`false`, even though `Calendar` values are in fact written through the
date/time formatting path. Was the omission intentional?
--
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]
