Re: Properties, defaults, PR 900, etc

On 17 September 2017 at 17:18, Vladimir Sitnikov <
sitnikov(dot)vladimir(at)gmail(dot)com> wrote:

> 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.
>
> Agreed,

I prefer the new Properties(defaultProps) but I don't have a strong reason
why

Dave Cramer

davec(at)postgresintl(dot)com
www.postgresintl.com