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

DataTypeCodec relies on COMPAT fallback for 'BC' era parsing #1379

Closed
jerboaa opened this issue Nov 21, 2023 · 13 comments · Fixed by #1394
Closed

DataTypeCodec relies on COMPAT fallback for 'BC' era parsing #1379

jerboaa opened this issue Nov 21, 2023 · 13 comments · Fixed by #1394
Assignees
Labels
Milestone

Comments

@jerboaa
Copy link

jerboaa commented Nov 21, 2023

Version

4.4.6

Context

This quarkus issue made me arrive here and has the full context:
quarkusio/quarkus#37208

Do you have a reproducer?

Yes.

Steps to reproduce

Get a JDK 22 EA version. E.g. from the Adoptium API: wget -O jdk-22-ea.tar.gz https://api.adoptium.net/v3/binary/latest/22/ea/linux/x64/jdk/hotspot/normal/eclipse, and untar it. tar -xf jdk-22-ea.tar.gz

./jdk-22+24/bin/jshell -J-showversion
openjdk version "22-beta" 2024-03-19
OpenJDK Runtime Environment Temurin-22+24-202311162331 (build 22-beta+24-ea)
OpenJDK 64-Bit Server VM Temurin-22+24-202311162331 (build 22-beta+24-ea, mixed mode, sharing)
|  Welcome to JShell -- Version 22-beta
|  For an introduction type: /help intro

jshell> java.time.LocalDateTime.parse("4714-11-24 00:00:00 BC", java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss G", Locale.ROOT));
|  Exception java.time.format.DateTimeParseException: Text '4714-11-24 00:00:00 BC' could not be parsed at index 20
|        at DateTimeFormatter.parseResolved0 (DateTimeFormatter.java:2108)
|        at DateTimeFormatter.parse (DateTimeFormatter.java:2010)
|        at LocalDateTime.parse (LocalDateTime.java:494)
|        at (#1:1)

Extra

I've filed this bug in upstream OpenJDK and the change is intentional as old code fell back to the COMPAT provider when the ROOT locale was being used. The workaround is to use Locale.US.

jdk-22+24/bin/jshell -J-showversion
openjdk version "22-beta" 2024-03-19
OpenJDK Runtime Environment Temurin-22+24-202311162331 (build 22-beta+24-ea)
OpenJDK 64-Bit Server VM Temurin-22+24-202311162331 (build 22-beta+24-ea, mixed mode, sharing)
|  Welcome to JShell -- Version 22-beta
|  For an introduction type: /help intro

jshell> java.time.LocalDateTime.parse("4714-11-24 00:00:00 BC", java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss G", Locale.US));
$1 ==> -4713-11-24T00:00

Can this line be changed to that, please? It should be compatible with older JDKs as well (tested with JDK 17).

public static final LocalDateTime LDT_MINUS_INFINITY = LocalDateTime.parse("4714-11-24 00:00:00 BC",
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss G", Locale.ROOT));

@zakkak
Copy link

zakkak commented Nov 21, 2023

pinging @vietj since he was one of the last persons to touch that part

@vietj
Copy link
Member

vietj commented Nov 21, 2023

can you provide a reproducer (a SQL statement would work fine) for this @jerboaa ?

@jerboaa
Copy link
Author

jerboaa commented Nov 21, 2023

No, sorry. I have no idea. Perhaps somebody in quarkusio/quarkus#37208 can help?

@zakkak
Copy link

zakkak commented Nov 22, 2023

@vietj is the following reproducer OK?

  1. Grab an OpenJDK 22+24-EA build from https://github.com/adoptium/temurin22-binaries/releases/tag/jdk-22%2B24-ea-beta
git clone https://github.com/zakkak/vertx-pg-1379-reproducer.git
cd vertx-pg-1379-reproducer
mvn package
/path/to/jdk-22+24-ea/bin/java -jar target/vertx-pg-example-1.0-SNAPSHOT-jar-with-dependencies.jar

All it does is:

System.out.println( DataTypeCodec.LDT_MINUS_INFINITY );

Which runs fine with JDK 21 printing:

-4713-11-24T00:00

but fails with JDK 22+24-ea printing:

Exception in thread "main" java.lang.ExceptionInInitializerError
	at com.example.App.main(App.java:13)
Caused by: java.time.format.DateTimeParseException: Text '4714-11-24 00:00:00 BC' could not be parsed at index 20
	at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2108)
	at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:2010)
	at java.base/java.time.LocalDateTime.parse(LocalDateTime.java:494)
	at io.vertx.pgclient.impl.codec.DataTypeCodec.<clinit>(DataTypeCodec.java:1077)
	... 1 more

@vietj
Copy link
Member

vietj commented Nov 23, 2023

ah right, I thought an interaction with the database was required :-) @zakkak

@jerboaa
Copy link
Author

jerboaa commented Nov 28, 2023

@vietj Do you have an ETA when this will be fixed? We will need the updated vertx version then in quarkus too. Thanks!

@vietj
Copy link
Member

vietj commented Nov 28, 2023

@jerboaa not yet, but for sure the issue is on our radar

@jerboaa
Copy link
Author

jerboaa commented Nov 28, 2023

OK, thanks!

zakkak added a commit to zakkak/quarkus that referenced this issue Dec 12, 2023
Workaround for quarkusio#37208 till
eclipse-vertx/vertx-sql-client#1379 gets fixed
and released

Note that the substitution only happens if necessary (i.e. it won't
happen when using JDK < 22, and it won't happen once an upstream fix
becomes available.

Closes quarkusio#37208
@zakkak
Copy link

zakkak commented Dec 12, 2023

Hi @vietj, I applied the change in my fork and tested it on github actions. Unfortunately I can't do any more testing since I am not familiar with the code base. I also opened #1391 in case it helps.

@vietj
Copy link
Member

vietj commented Dec 13, 2023

@zakkak I see the bug you filed has been fixed, the JDK 22-EA in which the bug appear has never been released, so I am assuming the next EA will have the bug fixed and that shall be fine ?

@jerboaa
Copy link
Author

jerboaa commented Dec 13, 2023

@zakkak I see the bug you filed has been fixed, the JDK 22-EA in which the bug appear has never been released, so I am assuming the next EA will have the bug fixed and that shall be fine ?

Which bug? This one? https://bugs.openjdk.org/browse/JDK-8320431

That got closed as Not an issue, which essentially means a fix as suggested in #1391 is needed. It's intended behaviour in the JDK which was a side-effect of it working in earlier JDKs and got finally fixed.

@vietj
Copy link
Member

vietj commented Dec 13, 2023

ok so this is not a temporary workaround as far as you can tell

@jerboaa
Copy link
Author

jerboaa commented Dec 13, 2023

ok so this is not a temporary workaround as far as you can tell

Yes.

@vietj vietj linked a pull request Dec 13, 2023 that will close this issue
@vietj vietj closed this as completed Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants