codeconsole commented on code in PR #16411: URL: https://github.com/apache/grails-core/pull/16411#discussion_r4123869251
########## grails-converters/src/main/groovy/org/grails/web/converters/marshaller/json/MonthMarshaller.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.web.converters.marshaller.json; + +import java.time.Month; + +import grails.converters.JSON; +import org.grails.web.converters.exceptions.ConverterException; +import org.grails.web.converters.marshaller.ObjectMarshaller; +import org.grails.web.json.JSONException; + +/** + * JSON ObjectMarshaller which converts a Month to its number, 1 for January through 12 for December, + * the same as Spring Boot's default Jackson rendering. It is registered ahead of + * {@link SimpleEnumMarshaller}, which would otherwise render the enum name. + * + * @since 8.0 + */ +public class MonthMarshaller implements ObjectMarshaller<JSON> { + + public boolean supports(Object object) { + return object instanceof Month; + } + + public void marshalObject(Object object, JSON converter) throws ConverterException { + try { + converter.getWriter().value(((Month) object).getValue()); Review Comment: Kept as a number. - On 9.0, #16414 renders `Month` through Boot's `JsonMapper` as `9` anyway, so `"SEPTEMBER"` in 8.0 would mean a second change in 9 for `grails.converters.JSON` users. - JSON views users see a single change, since 7 already wrote `"SEPTEMBER"`. - `monthValueConverter` binds both forms, and §76.4 shows the opt-out. - The registration is in `ConvertersConfigurationInitializer` now (f54c009bc3). ########## grails-converters/src/main/groovy/org/grails/web/converters/marshaller/json/SqlTimeMarshaller.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.web.converters.marshaller.json; + +import java.sql.Time; + +import grails.converters.JSON; +import org.grails.web.converters.exceptions.ConverterException; +import org.grails.web.converters.marshaller.ObjectMarshaller; +import org.grails.web.json.JSONException; + +/** + * JSON ObjectMarshaller which converts a {@link java.sql.Time} to its wall-clock time in the + * JVM default time zone ({@code HH:mm:ss}, from {@link Time#toString()}), the same as Spring Boot's + * default Jackson rendering. It is registered ahead of {@link DateMarshaller}, which would + * otherwise render the {@code Time} as a full date and time. + * + * @since 8.0 + */ +public class SqlTimeMarshaller implements ObjectMarshaller<JSON> { + + public boolean supports(Object object) { + return object instanceof Time; + } + + public void marshalObject(Object object, JSON converter) throws ConverterException { + try { + converter.getWriter().value(object.toString()); Review Comment: Kept. - The 7.x form doesn't bind back: `"1970-01-01T08:48:46.000Z"` is a `typeMismatch` for a `java.sql.Time` property. - `"01:48:46"` binds back to the same `Time` (bd89825ee6). - §76.4 shows a marshaller that restores the 7.x string, and a test covers it. ########## grails-databinding-core/src/main/groovy/org/grails/databinding/converters/DateConversionHelper.groovy: ########## @@ -44,19 +48,26 @@ class DateConversionHelper implements ValueConverter { */ boolean dateParsingLenient = false + /** + * Converts a date and time with an offset, such as {@code 2024-05-01T10:00:00Z} or + * {@code 2024-05-01T10:00:00+02:00}, as ISO 8601 writes it, and as Grails renders a date in + * JSON, to the instant it names, whatever the zone of the server. Any other value is + * converted by the first of the {@link #formatStrings} that reads all of it. + */ Object convert(value) { Date dateValue if (value instanceof String) { if (!value) { return null } + dateValue = offsetDateTime((String) value) Exception firstException formatStrings.each { String format -> if (dateValue == null) { DateFormat formatter = new SimpleDateFormat(format) try { formatter.lenient = dateParsingLenient - dateValue = formatter.parse((String) value) + dateValue = parseAll(formatter, (String) value) Review Comment: Done as you suggested, in 778d07203a: ISO, then a format that reads the whole value, then the 7.x read. - Rows 4–7 bind as in 7.x again. - The fallback also covers `…T10:00:00.123+0200`, a custom format matching only the start, and `Timestamp#toString()` with nanos, which the whole-value rule had also broken. ########## grails-databinding/src/main/groovy/org/grails/databinding/converters/Jsr310ConvertersConfiguration.groovy: ########## @@ -410,17 +441,40 @@ class Jsr310ConvertersConfiguration { value instanceof String } + /** + * Converts a value with the first of the configured date formats that reads all of it. + * + * @param callable parses the value with the formatter it is given + */ T convert(Object value, Closure callable) { + convert(value, null, callable) + } + + /** + * Converts a value in the ISO 8601 form of the type, which is how Grails renders it in JSON, or else with + * the first of the configured date formats that reads all of it. + * + * @param iso the ISO 8601 formatter of the type, or {@code null} to use only the configured formats + * @param callable parses the value with the formatter it is given + */ + T convert(Object value, DateTimeFormatter iso, Closure callable) { T dateValue if (value instanceof String) { if (!value) { return null } + if (iso != null) { + try { + return (T) callable.call(iso) + } catch (DateTimeParseException ignored) { + // Not the ISO 8601 form, so one of the configured formats. + } + } def firstException formatStrings.each { String format -> if (dateValue == null) { try { - dateValue = (T) callable.call(format) + dateValue = (T) callable.call(DateTimeFormatter.ofPattern(format)) Review Comment: Fixed in 77fa0e0a4e. - `convert(value, callable)` passes the `String` pattern again, so a 7.x subclass works unchanged. - `FormatsOnlyLocalTimeConverter` in the spec is written the 7.x way now (`{ String format -> … }`). ########## grails-doc/src/en/guide/upgrading/upgrading80x.adoc: ########## @@ -4161,3 +4161,148 @@ existing header alone. render(file: new File(absolutePath), fileName: 'report.pdf') render(file: new File(absolutePath), inline: true) ---- + +==== 76. JSON Rendering of Dates and Times Matches Spring Boot Review Comment: Done in ecee9bf3f3: §76.1 is what 8.0.0 restores from 7, §76.2 what renders differently than in 7, §76.3 what 7 didn't render as a value, and §76.4 how to keep the 7 rendering. ########## 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: That line is in `isSimpleValue(Object value)`, where `propertyType` is `value.getClass()`. So it checks the runtime type, as the other call sites do. ########## 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: Intentional. - No converter handles `Calendar`, so `hasDateTimeConverter(Calendar)` would be false even if `isDateTimeType` included it. - `Calendar` is already a `MappingFactory` simple type, and the generator writes it with the date format. -- 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]
