Re: [JDBC] Properties, defaults, PR 900, etc

Hi,

I'm reviewing PR#900 fix: use loginTimeout in java.sql.DriverManager by default (see https://github.com/pgjdbc/pgjdbc/pull/900 ), and I think it requires to be discussed in more details.

The property dance has already been performed multiple times (https://github.com/pgjdbc/pgjdbc/issues/259, https://github.com/pgjdbc/pgjdbc/pull/260, https://github.com/pgjdbc/pgjdbc/pull/438 )

As far as I see, 259 and 438 did teach a isPresent method to *ignore* "default storage" (e.g. System.properties) while checking for property presence.

Frankly speaking, I think if a default storage has some value for property X, then it should be indistinguishable from the case when the property was provided via connection URL.

Jeremy Whiting, Alexis Meneses, I would appreciate if you agree or disagree with "there is a case for isPresent to ignore default properties storage".

Here's the change in PR#900 that makes me shiver:

public static Properties parseURL(String url, Properties defaults) {

+ Properties urlProps = new Properties();

+ if (defaults != null) {

+ urlProps.putAll(defaults);

There are two gems in this change:

g1) It mixes existing "new Properties(defaults)" with "copy everything into final properties" approaches. It would make the code harder to maintain

g2) urlProps.putAll(defaults) would *ignore* the "parent properties" of a "defaults" object. It happens so that "Properties" does not override "entrySet"/"putAll", so this putAll would not put all the values, but it would ignore the ones defined in the "parent" of "defaults".

Just in case, I see the following property sources

"production case":

1) "user" comes from System.properties

2) "org/postgresql/driverconfig.properties" files on the "pgjdbc.classloader"

3) Properties provided in the "DriverManager.getConnection(...)" call

4) URL parameters

5) fallback values

Note: System.properties is not considered by pgjdbc except "user"

"test mode":

-2) build.properties file

-1) System.properties

... the rest is as in production

Frankly speaking, I would love to make property handling aligned, so we either always do "new Properties(defaultProps)" kind of thing (do not copy values, and use proper Properties.getString API to access defaults) or we do "copy all the values to one final big properties object).

I don't like the mix of the two approaches.