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

fix: avoid reflective access to TimeZone.defaultTimeZone in Java 9+ #1002

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Oct 31, 2017

closes #986

@vlsi vlsi added this to the 42.1.5 milestone Oct 31, 2017
Copy link
Member

@jorsol jorsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not better to use the preprocessor to do the version check?

tzField = null;
// Avoid reflective access in Java 9+
if (javaVersion.startsWith("1.") && javaVersion.length() == 3
&& javaVersion.charAt(2) >= '4' && javaVersion.charAt(2) <= 8) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be chatAt(2) >= '6' and you forgot the quotes for '8'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, makes sense

Object tzFromField = tzField.get(null);
if (defaultTz == null || !defaultTz.equals(tzFromField)) {
tzField = null;
String javaVersion = System.getProperty("java.version");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Java version check must be in an utility method.

@vlsi
Copy link
Member Author

vlsi commented Oct 31, 2017

Is not better to use the preprocessor to do the version check?

preprocessor is compile-time thing, while the point is to check JRE at runtime.
We have no specific jar for Java 9, so the codebase for 8 and 9 is the same.

@codecov-io
Copy link

codecov-io commented Oct 31, 2017

Codecov Report

Merging #1002 into master will increase coverage by 0.15%.
The diff coverage is 20%.

@@             Coverage Diff              @@
##             master    #1002      +/-   ##
============================================
+ Coverage     65.71%   65.87%   +0.15%     
+ Complexity     3604     3579      -25     
============================================
  Files           168      167       -1     
  Lines         15491    15320     -171     
  Branches       2494     2482      -12     
============================================
- Hits          10180    10092      -88     
+ Misses         4134     4050      -84     
- Partials       1177     1178       +1

@vlsi vlsi force-pushed the tz_reflection branch 2 times, most recently from 1910c8a to 8bb2477 Compare November 27, 2017 21:54
@vlsi vlsi merged commit fd0eeee into pgjdbc:master Nov 28, 2017
@vlsi vlsi modified the milestones: 42.1.5, 42.2.0 Jan 8, 2018
@jugimaster
Copy link

No idea if this was the latest solution, but I'm getting the same warning on Java 11:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.postgresql.jdbc.TimestampUtils
user=> (System/getProperty "java.version")
"11.0.3"

What does the committed code do with a Java version String of "11.0.3"?

I found a commit with this:

      tzField = null;
      // Avoid reflective access in Java 9+
      if (JavaVersion.getRuntimeVersion().compareTo(JavaVersion.v1_8) <= 0) {
        tzField = TimeZone.class.getDeclaredField("defaultTimeZone");
        tzField.setAccessible(true);

How does this work? Does the tzField variable not need a value on Java 9+ (or is it set elsewhere)?

@davecramer
Copy link
Member

Good question.

@davecramer
Copy link
Member

@jugimaster what version of the driver are you using?

@jugimaster
Copy link

jugimaster commented May 1, 2020

org.postgresql/postgresql "9.4.1212"

(It's a Clojure project.)

@davecramer
Copy link
Member

upgrade the driver to the latest.

@jugimaster
Copy link

Oh, alright. I just assumed it wouldn't be older than something from 2017

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

Successfully merging this pull request may close these issues.

Illegal reflective access operation warning with Java 9
5 participants