quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.77k stars 2.68k forks source link

Hibernate Validator Failure When TZ Set to Non-UTC #32831

Closed this-user closed 1 month ago

this-user commented 1 year ago

Describe the bug

Annotation-based validation via the Hibernate Validator module can fail in native mode if the time zone is set to anything but UTC. This only applies to running a native build inside a container. The container's OS time zone is set to local time, but in order to make the Quarkus application run in local time, too, setting the TZ environment variable is necessary. However, if this variable is set to anything but UTC, validation on time-based types can fail.

Specifically, I am using @PastOrPresent on a LocalDateTime field that is part of an entity class. The LocalDateTime is set to LocalDateTime.now(), so this should not fail when validation is being run. However, under the aforementioned circumstances of having set TZ to anything that has a positive offset to UTC, an exception is thrown, stating that the time must be in the past or present.

Seemingly, the explanation for this is that validation is internally still using UTC while the rest of the Quarkus application is running with the TZ that has been set. I have tried setting quarkus.hibernate-orm.jdbc.timezone to the same TZ, but this has no effect.

The issue only occurs for native builds, and does not occur for JVM-based images.

I am using the quarkus-micro-image:2.0 as the base.

The issue occurs when building for Java 17 with the Mandel builder image (v22.3) as well as when using Java 19 with GraalVM CE (v22.3).

Expected behavior

Validation should pass, because the field is valid.

Actual behavior

Validation throws an exception.

How to Reproduce?

https://github.com/this-user/quarkus-hibernate-validation-issue-tz

Steps to reproduces:

  1. Comment out quarkus.test.arg-line that sets the TZ in application.properties
  2. Run ./gradlew testNative --info
  3. Integration tests pass
  4. Remove comment from 1. and run tests again, now with non-UTC TZ
  5. Tests will fail because validation of TestEntity fails

Output of uname -a or ver

Linux 6.2.11-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 13 Apr 2023 16:59:24 +0000 x86_64 GNU/Linux

Output of java -version

17.0.6

GraalVM version (if different from Java)

22.3.1.0

Quarkus version or git rev

2.16.6 Final

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 8.0.2

Additional information

No response

quarkus-bot[bot] commented 1 year ago

/cc @gsmet (hibernate-validator), @yrodiere (hibernate-validator)

this-user commented 1 year ago

Is anyone actually looking at this?

gsmet commented 2 months ago

@marko-bekhta could you check if this is still an issue when you have some time?

We probably need to update the reproducer with quarkus update.

marko-bekhta commented 2 months ago

Hey @gsmet πŸ˜ƒ πŸ‘‹πŸ»

Yeah, it seems that this is still an issue ... I'll try to debug it a bit more, but after a quick check, what I've noticed was that: if you access a clock provider from an injected ValidatorFactory we get an expected

clock tz: Europe/Brussels
LocalDateTime.now( from HV clock ): 2024-08-25T10:47:07.764884
LocalDateTime.now(): 2024-08-25T10:47:07.764796

but what happens inside the injected Validator instance is a bit different:

reference clock: SystemClock[GMT]
reference value: 2024-08-25T08:47:07.764978
value (LocalDateTime.now()): 2024-08-25T10:47:07.764621
marko-bekhta commented 2 months ago

ok, so I think that I've found where the problem comes from. In here:

https://github.com/hibernate/hibernate-validator/blob/d2694eced589af7a279e366984e797fc026491e9/engine/src/main/java/org/hibernate/validator/internal/engine/constraintvalidation/ConstraintTree.java#L55-L60

We initialize the constraints, and in the case of date-time ones, we get a clock instance stored. At build time, it would be a GMT clock, so the clock is set to a build zone, and at runtime, it may be different... maybe we should use some custom clock like:

private static class CustomClock extends Clock {

    private ZoneId zoneId;

    @Override
    public ZoneId getZone() {
        if (zoneId == null) {
            synchronized (this) {
                if (zoneId == null) {
                    zoneId = ZoneId.systemDefault();
                }
            }
        }
        return zoneId;
    }

    @Override
    public Clock withZone(ZoneId zone) {
        throw new UnsupportedOperationException();
    }

    @Override
    public Instant instant() {
        return Instant.now();
    }
}

to let it get initialized on the first validation call?

gsmet commented 2 months ago

Hum. That's actually a great find. Having a Clock in the native image is definitely a problem.

Not sure about how best we can fix it but... we need to be careful as to not be too slow (and your code probably needs a volatile and then to optimize the volatile reads).

marko-bekhta commented 2 months ago

Another idea that I thought of... can we rely on a startup event to initialize the time zone? In other words, what if we add clock provider bean like:

public class StartupAwareClockProvider implements ClockProvider {
    private final StartupAwareClock clock = new StartupAwareClock();
    private ZoneId zoneId;

    void onStart(@Observes StartupEvent event) {
        // init the zone to match the one that is set at runtime:
        zoneId = ZoneId.systemDefault();
    }

    @Override
    public Clock getClock() {
        return clock;
    }

    private class StartupAwareClock extends Clock {

        @Override
        public ZoneId getZone() {
            return zoneId;
        }

        @Override
        public Clock withZone(ZoneId zone) {
            if (zoneId.equals(zone)) {
                return this;
            }
            return Clock.system(zone);
        }

        @Override
        public Instant instant() {
            return Instant.now();
        }
    }
}

Or, there may be something triggering validation before the event is fired?

gsmet commented 2 months ago

This problem will only occur if we use the default Clock right?

I think you're onto something but it seems a bit brittle and I wonder if we should just reinitialize your Clock at runtime and have the zone in a static field that would be reinitialized.

See RuntimeReinitializedClassBuildItem.

That way we would rely on proven GraalVM mechanism and the value with be switched as soon as we are in runtime mode.

Does it make sense?

marko-bekhta commented 2 months ago

This problem will only occur if we use the default Clock right?

I guess if a user decides to supply a custom one, but implements it as something like:

public class UserClockProvider implements ClockProvider {
    @Override
    public Clock getClock() {
        return Clock.systemDefaultZone();
    }
}

then the app will get into the same state, I guess...

but it seems a bit brittle

yeah πŸ˜ƒ I was just thinking if there was a way to do it safely, and I thought you'd know of one if there is πŸ˜ƒ

Does it make sense?

yes, and I can give that a try. Maybe for the user-provided ones, we can add a warning to the documentation that users should be aware that this may happen and that they should address it on their side... πŸ™ˆ

gsmet commented 2 months ago

For the user provided ones, I would expect them to use a non-default clock and in this case, it wouldn't be a problem.

I think we can start by fixing the default case and then when someone complains with a use case, we can see how we can iterate?

But sure you can add a warning if you can write one that is clear enough for people to understand how to fix it :).