Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard DateConverter against null #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pingpingy1
Copy link
Contributor

When null is passed to the DateConverter constructor, NullPointerException will eventually be thrown.
However, since the constructor is lazily evaluated, this is not thrown until much later, leading to difficult debugging.
This commit implements a guard against null input, throwing an IllegalArgumentException.

@jungm
Copy link
Member

jungm commented Jan 25, 2024

Generally looks good to me, but fails checkstyle execution in mvn checkstyle:check:

[ERROR] /home/markus/tmp/johnzon-pingpingy1/johnzon-mapper/src/test/java/org/apache/johnzon/mapper/converter/DateConverterTest.java:3:1: Redundant import from the same package - org.apache.johnzon.mapper.converter.DateConverter. [RedundantImport]
[ERROR] /home/markus/tmp/johnzon-pingpingy1/johnzon-mapper/src/test/java/org/apache/johnzon/mapper/converter/DateConverterTest.java:9:1: File contains tab characters (this is the first instance). [FileTabCharacter]

@rmannibucau
Copy link
Contributor

now we moved the minimum java version we can probably move to DateTimeFormatter and drop the threadlocal (just need to maintain some pattern compatibility but it is almost the same format IIRC) so then we can just leverage the already existing validations and get rid of this dirty threadlocal no? wdyt?

When `null` is passed to the `DateConverter` constructor, NullPointerException will eventually be thrown.
However, since the constructor is lazily evaluated, this is not thrown until much later, leading to difficult debugging.
This commit implements a guard against `null` input, immediately throwing an IllegalArgumentException.
@pingpingy1
Copy link
Contributor Author

Fixed the checkstyle issue for the moment, in case it might be of help.
I don't consider myself wholly qualified for conducting the transition you mentioned, but it sounds good IMHO.

@rmannibucau
Copy link
Contributor

In the spirit (date parsing can be enhanced) - this is what we do for JSON-B already for ex:

public class DateConverter implements Converter<Date> {
    private static final ZoneId UTC = ZoneId.of("UTC");

    private final DateTimeFormatter formatter;

    public DateConverter(final String pattern) {
        this.formatter = ofPattern(pattern, ROOT);
    }

    @Override
    public String toString(final Date instance) {
        return formatter.format(ZonedDateTime.ofInstant(instance.toInstant(), UTC));
    }

    @Override
    public Date fromString(final String text) {
        try {
            return Date.from(parseZonedDateTime(text, formatter).toInstant());
        } catch (final DateTimeParseException dpe) {
            return Date.from(LocalDateTime.parse(text).toInstant(ZoneOffset.UTC));
        }
    }

    private static ZonedDateTime parseZonedDateTime(final String text, final DateTimeFormatter formatter) {
        final TemporalAccessor parse = formatter.parse(text);
        ZoneId zone = parse.query(TemporalQueries.zone());
        if (Objects.isNull(zone)) {
            zone = UTC;
        }
        final int year = parse.isSupported(YEAR) ? parse.get(YEAR) : 0;
        final int month = parse.isSupported(MONTH_OF_YEAR) ? parse.get(MONTH_OF_YEAR) : 0;
        final int day = parse.isSupported(DAY_OF_MONTH) ? parse.get(DAY_OF_MONTH) : 0;
        final int hour = parse.isSupported(HOUR_OF_DAY) ? parse.get(HOUR_OF_DAY) : 0;
        final int minute = parse.isSupported(MINUTE_OF_HOUR) ? parse.get(MINUTE_OF_HOUR) : 0;
        final int second = parse.isSupported(SECOND_OF_MINUTE) ? parse.get(SECOND_OF_MINUTE) : 0;
        final int millisecond = parse.isSupported(MILLI_OF_SECOND) ? parse.get(MILLI_OF_SECOND) : 0;
        return ZonedDateTime.of(year, month, day, hour, minute, second, millisecond, zone);
    }
}

Checked the pattern table and formatter seems to be a superset of simple dateformat so we shouldn't be too bad.
Once applied there are a few failures but mainly around the error messages so test can be "fixed".

Do you want to try that path?

@jungm
Copy link
Member

jungm commented Mar 21, 2024

@pingpingy1, want to take care of this PR as well? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants