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

refactor: fix getDriverVersion and getDriverName and getJDBCMajor/MinorVersion methods #668

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
6 participants
@jorsol
Contributor

jorsol commented Oct 19, 2016

The driver name is "PostgreSQL JDBC Driver"
The output of the driver version should not include "PostgreSQL"

@vlsi

This comment has been minimized.

Member

vlsi commented Oct 19, 2016

The output of the driver version should not include "PostgreSQL"

Any specific reason for doing so?
I'm afraid it might trigger unexpected backward-compatibility issues

@codecov-io

This comment has been minimized.

codecov-io commented Oct 19, 2016

Current coverage is 64.37% (diff: 41.66%)

Merging #668 into master will increase coverage by 0.14%

@@             master       #668   diff @@
==========================================
  Files           163        163          
  Lines         15142      15140     -2   
  Methods           0          0          
  Messages          0          0          
  Branches       2987       2987          
==========================================
+ Hits           9727       9747    +20   
+ Misses         4182       4156    -26   
- Partials       1233       1237     +4   

Powered by Codecov. Last update e1a2578...b564f42

@jorsol

This comment has been minimized.

Contributor

jorsol commented Oct 19, 2016

@vlsi, prior to maven conversion the version was returned in this format:

42c2e3b#diff-a82520f846ea7ba590d13a466aeafffeL498

The output of postgresql-9.3-1103.jdbc3.jar is "PostgreSQL 9.3 JDBC3g (build 1103)"
and the output of postgresql-9.4-1206-jdbc42.jar is "PostgreSQL 9.4 JDBC4.2 (build 1206)"
so there was already a "potential" backward-compatibility issue since now returns "PostgreSQL 9.4.1211".

I doubt anyone is using that to "parse" the version of the driver, for that there is getDriverMajorVersion() and getDriverMinorVersion() methods. It's a matter of correctness since getDriverVersion() means version.

@davecramer

This comment has been minimized.

Member

davecramer commented Oct 19, 2016

given that nobody looks at it, I guess this is fine, on the other hand nobody looks at it ;)

/**
* Retrieves the name of this JDBC driver.
*
* @return "PostgreSQL JDBC Driver"

This comment has been minimized.

@panchenko

panchenko Oct 20, 2016

getDriverName() incompatibility has potentially higher impact than getDriverVersion().

This comment has been minimized.

@davecramer

davecramer Oct 20, 2016

Member

Good catch. Please revert this change

This comment has been minimized.

@jorsol

jorsol Dec 30, 2016

Contributor

@panchenko have you find a project that use getDriverName() to implement logic and cause a potentially higher impact? I don't have found any.

@jorsol

This comment has been minimized.

Contributor

jorsol commented Oct 20, 2016

IMO general applications use getDriverVersion() and getDriverName() for printing information not for implement logic.

I did a quick find (nothing deep) on "potential" projects that "probably" use this information which are mostly ORMs, on JOOQ, Sprint-JDBC, Hibernate and found.... nothing.

Since we have entered in a highly speculative area I will close this PR.

@jorsol jorsol closed this Oct 20, 2016

@jorsol

This comment has been minimized.

Contributor

jorsol commented Nov 29, 2016

Now that we have moving forward with versioning 42 this can be reconsidered.

@jorsol jorsol reopened this Nov 29, 2016

@jorsol jorsol force-pushed the jorsol:change_version_output branch from fb6ea20 to 6a3d427 Dec 6, 2016

@jorsol jorsol force-pushed the jorsol:change_version_output branch 2 times, most recently from 0cbbb4c to b7e7fec Dec 14, 2016

/**
* Retrieves the major JDBC version number for this driver.
*
* @return JDBC version major number

This comment has been minimized.

@vlsi

vlsi Dec 30, 2016

Member

Any clue

@vlsi

In general looks good, except two things:

  1. I would like to refrain from copy-pasting inherited javadocs. The build process handles that just fine (I mean it is smart enough to copy javadocs from java.sql.* and alike).
  2. string split in getXXXversion look odd.
*
* @return JDBC version major number
*/
@Override

This comment has been minimized.

@vlsi

vlsi Dec 30, 2016

Member

Any clue why do you copy javadocs for @Override methods?

AFAIK, javadoc build does handle that just fine.
See http://static.javadoc.io/org.postgresql/postgresql/9.4.1212.jre7/org/postgresql/Driver.html#getParentLogger() as an example.

So I would remove all the inherited copy-pasted javadocs unless they have some postgresql specifics.
For instance, connect method does have some PG specific, so it is LGTM. On the other hand, things like Retrieves the major JDBC version number for this driver add little value and can be inherited.

However, in this particular method adding some pgjdbc-specific example would help a lot. For instance "The method returns the driver major version, not the versions of the backend. This method would return something like 42 ad pgjdbc versions are like 42.x.y."

This comment has been minimized.

@jorsol

jorsol Dec 30, 2016

Contributor

Good point, I will look at it.

This comment has been minimized.

@vlsi

vlsi Dec 30, 2016

Member

Well, technically speaking, there's non-trivial build step that unzips jdk sources (e.g. https://github.com/pgjdbc/pgjdbc-parent-poms/blob/master/pgjdbc-core-parent/pom.xml#L270-L278) and adds them to javadoc tool, however it does "just work".

*/
@Override
public int getJDBCMajorVersion() {
return Integer.parseInt(JDBCVERSION.split("\\.")[0]);

This comment has been minimized.

@vlsi

vlsi Dec 30, 2016

Member

Is mvn.project.property.parsedversion.majorversion better here?

This comment has been minimized.

@jorsol

jorsol Dec 30, 2016

Contributor

No, the mvn.project.property.parsedversion.majorversion is for the driver version, not for the JDBC specification version, so this should return 4 as the major specification version, this is get from mvn.project.property.jdbc.specification.version and parsed the first number.

*/
@Override
public int getJDBCMinorVersion() {
return Integer.parseInt(JDBCVERSION.split("\\.")[1]);

This comment has been minimized.

@vlsi

vlsi Dec 30, 2016

Member

Is mvn.project.property.parsedversion.minorversion better here

This comment has been minimized.

@jorsol

jorsol Dec 30, 2016

Contributor

No, the mvn.project.property.parsedversion.minorversion is for the driver version, not for the JDBC specification version, so this should return 0 for Java 6, 1 for Java 7 and 2 for Java 8 as the major specification version, this is get from mvn.project.property.jdbc.specification.version and parsed the second number.

@vlsi vlsi added this to the 42.0.0 milestone Dec 30, 2016

@jorsol jorsol force-pushed the jorsol:change_version_output branch 6 times, most recently from 0e8f448 to 415b3c3 Dec 30, 2016

@jorsol jorsol changed the title from style: fix getDriverVersion() and getDriverName() methods to refactor: fix getDriverVersion and getDriverName and getJDBCMajor/MinorVersion methods Jan 1, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 1, 2017

@vlsi, requested changes done, removed javadoc that can be inherited, I could not reproduce the behavior using mvn javadoc:javadoc but since the release artifact use a special "unzip jdk" I guess it's ok.

For the second point, the other form to split the version is with a StringTokenizer, but the JavaDocs say:

StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead.

Happy New Year!

@jorsol jorsol force-pushed the jorsol:change_version_output branch 2 times, most recently from 6f92d48 to 18bedad Jan 2, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 5, 2017

Another comment about this?

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 12, 2017

@vlsi , is there any other request on this?

*
* @exception SQLException if a database access error occurs
* @return "PostgreSQL"
* @throws SQLException if a database access error occurs

This comment has been minimized.

@panchenko

panchenko Jan 12, 2017

I would rather remove "throws" from code, rather then documenting it.

@@ -281,7 +282,7 @@
/**
* The application name (require server version >= 9.0)
*/
APPLICATION_NAME("ApplicationName", null, "name of the application (backend >= 9.0)"),
APPLICATION_NAME("ApplicationName", DriverInfo.DRIVER_NAME, "Name of the Application (backend >= 9.0)"),

This comment has been minimized.

@panchenko

panchenko Jan 12, 2017

Is this default absolutely necessary?

This comment has been minimized.

@jorsol

jorsol Jan 12, 2017

Contributor

And why not?

This comment has been minimized.

@panchenko

panchenko Jan 12, 2017

Theoretically, one might want a connection without this property. I don't have a real world example for that, just being cautious about unnecessary changes :-)


public static final String JDBC_VERSION = "/*$mvn.project.property.jdbc.specification.version$*/";
public static final int JDBC_MAJOR_VERSION = Integer.parseInt(JDBC_VERSION.split("\\.")[0]);
public static final int JDBC_MINOR_VERSION = Integer.parseInt(JDBC_VERSION.split("\\.")[1]);

This comment has been minimized.

@panchenko

panchenko Jan 12, 2017

in poms there is also jdbc.specification.version.nodot - might be use it and int math operations instead?

This comment has been minimized.

@jorsol

jorsol Jan 12, 2017

Contributor

Maybe, but the problem with the split is?

This comment has been minimized.

@panchenko

panchenko Jan 12, 2017

Less operations during runtime, it will be just a compile time constant.

This comment has been minimized.

@vlsi

vlsi Jan 12, 2017

Member
  1. static final fields are initialized once, so there's no point in discussing "runtime overhead"
  2. My main concern on .split was it is hard to get right ("off by one" errors and so on).

This comment has been minimized.

@jorsol

jorsol Jan 12, 2017

Contributor

The property is set by default on the pom, so it should be hard to "break" or get the "off by one" errors, I can use the int math if you wish, at least it will allow to catch bad values at compile time.

To be completely safe there must be the properties in the pom.xml and use those values.
jdbc.specification.majorversion
jdbc.specification.minorversion

@jorsol jorsol force-pushed the jorsol:change_version_output branch 3 times, most recently from e5aeb90 to 7501977 Jan 12, 2017

@jorsol jorsol force-pushed the jorsol:change_version_output branch from 7501977 to b564f42 Jan 12, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 12, 2017

Now is using the int math, what do you think?

@vlsi

vlsi approved these changes Jan 20, 2017

@vlsi vlsi merged commit aa97434 into pgjdbc:master Jan 20, 2017

2 checks passed

codecov/project 64.37% (+0.14%) compared to e1a2578
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorsol jorsol deleted the jorsol:change_version_output branch Jan 27, 2017

vlsi added a commit to vlsi/pgjdbc that referenced this pull request Aug 4, 2017

fix: reintroduce Driver.getVersion for backward compatibility reasons
The method was removed in pgjdbc#668, however
certain applications might still depend on it.

Mark the method as deprecated.

vlsi added a commit that referenced this pull request Aug 4, 2017

davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017

@hmatt1

This comment has been minimized.

hmatt1 commented Jan 4, 2018

It turns out ActiveMQ was relying on the original name, so this change broke compatibility. See this issue: https://issues.apache.org/jira/browse/AMQ-6780

@davecramer

This comment has been minimized.

Member

davecramer commented Jan 4, 2018

@hmatt1 not sure how that is our problem ?

@hmatt1

This comment has been minimized.

hmatt1 commented Jan 4, 2018

@davecramer it's not, no action needed on your part. I figured I share because jorsol commented

IMO general applications use getDriverVersion() and getDriverName() for printing information not for implement logic.

and it turns out ActiveMQ is relying on getDriverName()

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 4, 2018

Yes, and it's ActiveMQ fault to relying on getDriverName(), for instance if I want to use pgjdbc-ng or a custom fork of PgJDBC, it makes impossible to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment