jdaugherty commented on code in PR #16411: URL: https://github.com/apache/grails-core/pull/16411#discussion_r4118024634
########## 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: This is the third shape for `Month` across two releases: the enum object in 7.x, `"SEPTEMBER"` per section 8 of the 8.0 guide, and now `9`. It is also the only enum that behaves differently from every other enum, and it needs `monthValueConverter` just to round-trip. I would keep `"SEPTEMBER"` for 8.0 and leave the numeric form for the wider parity discussion. `monthValueConverter` can stay either way, since it only claims numeric input and makes both forms bind. ########## 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: In 7.0.x a `java.sql.Time` went through `DateMarshaller` and rendered as a UTC instant (`"1970-01-01T01:48:46.000Z"`), and JSON views did the same through `writeDate`. This changes it to a local time-of-day in the JVM default zone, so both the shape and the zone semantics move for a type that rendered fine in 7. Jackson does write `toString()` here, so the parity claim is right, but this is not part of the #16406 regression. I would leave `Time` on the `Date` path for RC2 and decide the parity question separately. ########## 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: The closure passed to `convert(value, callable)` used to receive the `String` pattern and now receives a `DateTimeFormatter`. The subclass test was rewritten for that, but a 7.x subclass written as `convert(value) { String format -> X.parse(value, DateTimeFormatter.ofPattern(format)) }` now fails at runtime with a `MissingMethodException`. This is an `org.grails` internal, so it may be acceptable, but if we keep it the upgrade guide should say so. Alternatively keep passing the pattern string and build the formatter inside the closure as before. ########## 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: This is the part of the binding change I think needs discussion before it goes into an RC. With the default `dateFormats`, which still include `yyyy-MM-dd`, the whole-value rule turns these 7.x results into binding errors: | Input | 7.0.x | now | |---|---|---| | `2024-05-01T10:00` (HTML `datetime-local`) | midnight | error | | `2024-05-01 10:00:00` | midnight | error | | `2024-05-01T10:00:00.123` (no zone) | 10:00 local, millis dropped | error | The first two were silent data loss in 7, so an error is arguably better, but the third gave a usable value. A cheap way to keep 7.x compatibility while still fixing the offset and fraction cases: ISO first, then whole-value formats, then fall back to the 7.x prefix read (`formatter.parse(value)`) only when nothing else matched. ########## 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: The table here is accurate and I appreciate how complete it is. One suggestion once the split is settled: separate what 8.0.0 restores from 7 (the `.SSS` millisecond form and the `java.sql` types) from what it changes relative to 7, so a reader upgrading from 7.x can tell at a glance which rows require action. -- 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]
