Обсуждение: [BUGS]log can not be output when use DataSource

Hi,

In the following code,log can not be output as expected.

-------------------------------------
org.postgresql.Driver.setLogLevel(org.postgresql.Driver.DEBUG);
DataSource source = (DataSource)new InitialContext().lookup("DataSource");
Connection con = source.getConnection();
-------------------------------------

It's seems to be a problem,
I have made a small patch to fix it.
Hopefully this useful.

--
Best Regards,
Chen Huajun

 > It's early here but I can't see how this makes it work. It appears that the code that is being replaced is exactly
thesame except that there is an if statement in front of it ? 
 >

 >     org.postgresql.Driver.setLogLevel(org.postgresql.Driver.DEBUG);

here the JVM level's LogLevel is set to be DEBUG.


 >     DataSource source = (DataSource)new InitialContext().lookup("DataSource");
 >     Connection con = source.getConnection();

source.getConnection()
    -->DriverManager.getConnection(getUrl(), user, password)
       ->getUrl()
            sb.append("&loglevel=").append(logLevel);

here the Connection's  LogLevel is set to be the initial value 0(*) via URL,
although nobody set DataSource's LogLevel by calling BaseDataSource.setLogLevel().

I think it's better append "loglevel" to URL only when
BaseDataSource.setLogLevel() was called.
so a Connection created by DataSource.getConnection() can inherit JVM level's LogLevel setting
just like which one created by DriverManager.getConnection().


*)0 is the initial value and not a valid LogLevel,
valid values are INFO (1) and DEBUG (2).

--
Best Regards,
Chen Huajun
(2013/01/17 17:19), Dave Cramer wrote:
> It's early here but I can't see how this makes it work. It appears that the code that is being replaced is exactly
thesame except that there is an if statement in front of it ? 
>
>
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Wed, Jan 16, 2013 at 11:47 PM, Chen Huajun <chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>> wrote:
>
>     Hi,
>
>     In the following code,log can not be output as expected.
>
>     -------------------------------------
>     org.postgresql.Driver.setLogLevel(org.postgresql.Driver.DEBUG);
>     DataSource source = (DataSource)new InitialContext().lookup("DataSource");
>     Connection con = source.getConnection();
>     -------------------------------------
>
>     It's seems to be a problem,
>     I have made a small patch to fix it.
>     Hopefully this useful.
>
>     --
>     Best Regards,
>     Chen Huajun
>
>
>
>     --
>     Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org <mailto:pgsql-jdbc@postgresql.org>)
>     To make changes to your subscription:
>     http://www.postgresql.org/mailpref/pgsql-jdbc
>
>

Also is logLevel expected in the DriverManager.getConnection(getUrl(), user,
password)?

According to the API jdbc:subprotocol:subname is expected and
many of the other parameters seem to not be optionally applied like
loginTimeout, and socketTimeout like logLevel.

I'm also not inclinded to understand how this is a bug and fails
to not log, though may not report the parameter properly unless
the user does setLogLevel().

danap.


Dave Cramer wrote:
 > Yes, it might be better, but I don't see how it fails otherwise ?
 >
 > Dave Cramer
 >
 > dave.cramer(at)credativ(dot)ca
 > http://www.credativ.ca
 >
 >
 > On Thu, Jan 17, 2013 at 5:42 AM, Chen Huajun <chenhj@cn.fujitsu.com
 > <mailto:chenhj@cn.fujitsu.com>> wrote:
 >
 >
 >      > It's early here but I can't see how this makes it work. It
 >     appears that the code that is being replaced is exactly the same
 >     except that there is an if statement in front of it ?
 >      >
 >
 >      >
 >     org.postgresql.Driver.__setLogLevel(org.postgresql.__Driver.DEBUG);
 >
 >     here the JVM level's LogLevel is set to be DEBUG.
 >
 >
 >
 >      >     DataSource source = (DataSource)new
 >     InitialContext().lookup("__DataSource");
 >      >     Connection con = source.getConnection();
 >
 >     source.getConnection()
 >         -->DriverManager.__getConnection(getUrl(), user, password)
 >            ->getUrl()
 >                 sb.append("&loglevel=").__append(logLevel);
 >
 >     here the Connection's  LogLevel is set to be the initial value 0(*)
 >     via URL,
 >     although nobody set DataSource's LogLevel by calling
 >     BaseDataSource.setLogLevel().
 >
 >     I think it's better append "loglevel" to URL only when
 >     BaseDataSource.setLogLevel() was called.
 >     so a Connection created by DataSource.getConnection() can inherit
 >     JVM level's LogLevel setting
 >     just like which one created by DriverManager.getConnection().
 >
 >
 >     *)0 is the initial value and not a valid LogLevel,
 >     valid values are INFO (1) and DEBUG (2).
 >
 >
 >     --
 >     Best Regards,
 >     Chen Huajun
 >     (2013/01/17 17:19), Dave Cramer wrote:
 >
 >         It's early here but I can't see how this makes it work. It
 >         appears that the code that is being replaced is exactly the same
 >         except that there is an if statement in front of it ?
 >
 >
 >
 >         Dave Cramer
 >
 >         dave.cramer(at)credativ(dot)ca
 >         http://www.credativ.ca
 >
 >
 >         On Wed, Jan 16, 2013 at 11:47 PM, Chen Huajun
 >         <chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>
 >         <mailto:chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>>__>
 >         wrote:
 >
 >              Hi,
 >
 >              In the following code,log can not be output as expected.
 >
 >              ------------------------------__-------
 >
 >         org.postgresql.Driver.__setLogLevel(org.postgresql.__Driver.DEBUG);
 >              DataSource source = (DataSource)new
 >         InitialContext().lookup("__DataSource");
 >              Connection con = source.getConnection();
 >              ------------------------------__-------
 >
 >              It's seems to be a problem,
 >              I have made a small patch to fix it.
 >              Hopefully this useful.
 >
 >              --
 >              Best Regards,
 >              Chen Huajun
 >          --
 >              Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org
 >         <mailto:pgsql-jdbc@postgresql.org>
 >         <mailto:pgsql-jdbc@postgresql.__org
 >         <mailto:pgsql-jdbc@postgresql.org>>)
 >
 >              To make changes to your subscription:
 >         http://www.postgresql.org/__mailpref/pgsql-jdbc
 >         <http://www.postgresql.org/mailpref/pgsql-jdbc>

> Yes, it might be better, but I don't see how it fails otherwise ?

Let me describe it again
1)Driver has a global LogLevel setting by the following
   org.postgresql.Driver.setLogLevel()

2)each Connection has their own LogLevel,default is the same as Driver's,and can be rewrited by url,for example
   jdbc:postgresql://localhost/test?loglevel=1

     protected AbstractJdbc2Connection(HostSpec[] hostSpecs, String user, String database, Properties info, String url)
throwsSQLException 
     {
...
                int logLevel = Driver.getLogLevel();
         String connectionLogLevel = info.getProperty("loglevel");
         if (connectionLogLevel != null) {
             try {
                 logLevel = Integer.parseInt(connectionLogLevel);
             } catch (Exception l_e) {
                 // XXX revisit
                 // invalid value for loglevel; ignore it
             }
         }
...
     }

3)BaseDataSource.getConnection() will append loglevel to url regardless BaseDataSource.setLogLevel() was not be called
So,the url should be as the following,and the Connection's log is off.
jdbc:postgresql://localhost/test?loglevel=0


--
Best Regards，
Chen Huajun
(2013/01/18 1:22), dmp wrote:
> Also is logLevel expected in the DriverManager.getConnection(getUrl(), user, password)?
>
> According to the API jdbc:subprotocol:subname is expected and
> many of the other parameters seem to not be optionally applied like loginTimeout, and socketTimeout like logLevel.
>
> I'm also not inclinded to understand how this is a bug and fails
> to not log, though may not report the parameter properly unless
> the user does setLogLevel().
>
> danap.
>
>
> Dave Cramer wrote:
>  > Yes, it might be better, but I don't see how it fails otherwise ?
>  >
>  > Dave Cramer
>  >
>  > dave.cramer(at)credativ(dot)ca
>  > http://www.credativ.ca
>  >
>  >
>  > On Thu, Jan 17, 2013 at 5:42 AM, Chen Huajun <chenhj@cn.fujitsu.com
>  > <mailto:chenhj@cn.fujitsu.com>> wrote:
>  >
>  >
>  > > It's early here but I can't see how this makes it work. It
>  > appears that the code that is being replaced is exactly the same
>  > except that there is an if statement in front of it ?
>  > >
>  >
>  > >
>  > org.postgresql.Driver.__setLogLevel(org.postgresql.__Driver.DEBUG);
>  >
>  > here the JVM level's LogLevel is set to be DEBUG.
>  >
>  >
>  >
>  > > DataSource source = (DataSource)new
>  > InitialContext().lookup("__DataSource");
>  > > Connection con = source.getConnection();
>  >
>  > source.getConnection()
>  > -->DriverManager.__getConnection(getUrl(), user, password)
>  > ->getUrl()
>  > sb.append("&loglevel=").__append(logLevel);
>  >
>  > here the Connection's LogLevel is set to be the initial value 0(*)
>  > via URL,
>  > although nobody set DataSource's LogLevel by calling
>  > BaseDataSource.setLogLevel().
>  >
>  > I think it's better append "loglevel" to URL only when
>  > BaseDataSource.setLogLevel() was called.
>  > so a Connection created by DataSource.getConnection() can inherit
>  > JVM level's LogLevel setting
>  > just like which one created by DriverManager.getConnection().
>  >
>  >
>  > *)0 is the initial value and not a valid LogLevel,
>  > valid values are INFO (1) and DEBUG (2).
>  >
>  >
>  > --
>  > Best Regards,
>  > Chen Huajun
>  > (2013/01/17 17:19), Dave Cramer wrote:
>  >
>  > It's early here but I can't see how this makes it work. It
>  > appears that the code that is being replaced is exactly the same
>  > except that there is an if statement in front of it ?
>  >
>  >
>  >
>  > Dave Cramer
>  >
>  > dave.cramer(at)credativ(dot)ca
>  > http://www.credativ.ca
>  >
>  >
>  > On Wed, Jan 16, 2013 at 11:47 PM, Chen Huajun
>  > <chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>
>  > <mailto:chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>>__>
>  > wrote:
>  >
>  > Hi,
>  >
>  > In the following code,log can not be output as expected.
>  >
>  > ------------------------------__-------
>  >
>  > org.postgresql.Driver.__setLogLevel(org.postgresql.__Driver.DEBUG);
>  > DataSource source = (DataSource)new
>  > InitialContext().lookup("__DataSource");
>  > Connection con = source.getConnection();
>  > ------------------------------__-------
>  >
>  > It's seems to be a problem,
>  > I have made a small patch to fix it.
>  > Hopefully this useful.
>  >
>  > --
>  > Best Regards,
>  > Chen Huajun
>  > --
>  > Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org
>  > <mailto:pgsql-jdbc@postgresql.org>
>  > <mailto:pgsql-jdbc@postgresql.__org
>  > <mailto:pgsql-jdbc@postgresql.org>>)
>  >
>  > To make changes to your subscription:
>  > http://www.postgresql.org/__mailpref/pgsql-jdbc
>  > <http://www.postgresql.org/mailpref/pgsql-jdbc>
>
>

I now have confirmed the issue, but Chen your patch does not I believe address
the underlining behavior that should be expected from the PgJDBC driver and
that is this I believe.

logLevel may be set via two methods.

***************************************
1. org.postgresql.Driver.setLogLevel()

2. User defined logLevel setting via the connection properties
    be it through the DriverManager or DataSource.
***************************************

I think most would agree and I have confirmed via the existing PgJDBC
behavior that (2), the user setting of the logLevel should override the
method (1), Driver.setLogLevel() at all times.

The patch Chen does not seem to produce this result and perhaps that is
why it is confusing. It is true as indicated that if the user does not
user DataSource.setLogLevel() then no matter what the Driver.setLogLevel()
is set for INFO, DEBUG, it will always be the default 0 when using the
DataSource connection creation.

Now I have wasted over an hour in determing the extent of this
reports validity and whether the patch is good.

I suggest that patches not be accepted unless sample code is supplied
that validates/demostrates the bug and if a patch is submitted it is
tested to address the failure via the sample code. Patches should I
believe as has been addressed before almost always accompanied by
test code to validate that will added to the existing code base.

Chen I have rebuilt the driver to include your patch which you can
use to see that the behavior I described as seems to be desired is
not achieved.

http://dandymadeproductions.com/temp/postgresql-loglevel.jdbc4.jar

danap.


Chen Huajun wrote:
>  > Yes, it might be better, but I don't see how it fails otherwise ?
>
> Let me describe it again
> 1)Driver has a global LogLevel setting by the following
> org.postgresql.Driver.setLogLevel()
>
> 2)each Connection has their own LogLevel,default is the same as
> Driver's,and can be rewrited by url,for example
> jdbc:postgresql://localhost/test?loglevel=1
>
> protected AbstractJdbc2Connection(HostSpec[] hostSpecs, String user,
> String database, Properties info, String url) throws SQLException
> {
> ...
> int logLevel = Driver.getLogLevel();
> String connectionLogLevel = info.getProperty("loglevel");
> if (connectionLogLevel != null) {
> try {
> logLevel = Integer.parseInt(connectionLogLevel);
> } catch (Exception l_e) {
> // XXX revisit
> // invalid value for loglevel; ignore it
> }
> }
> ...
> }
>
> 3)BaseDataSource.getConnection() will append loglevel to url regardless
> BaseDataSource.setLogLevel() was not be called
> So,the url should be as the following,and the Connection's log is off.
> jdbc:postgresql://localhost/test?loglevel=0

danap I am also a bit confused for what's the expected behavior of PgJDBC driver.
I believe the following is expected.Are you agree?

1)logLevel could be set via org.postgresql.Driver.setLogLevel()
2)logLevel also could be set via connection properties(through the DriverManager or DataSource.)
3)if both (1)and (2) are set, (2)override (1)

so according to (1),in the following sample ,the loglevel is expected to be DEBUG.
===================================================
    org.postgresql.Driver.setLogLevel(org.postgresql.Driver.DEBUG);
    PGPoolingDataSource source = new PGPoolingDataSource();
         ...(no DataSource.setLogLevel() called)
         Connection con = source.getConnection();
===================================================

But the actual loglevel is 0 in existing PgJDBC driver,
My patch is for that. After patched the loglevel is DEBUG.

 > Chen I have rebuilt the driver to include your patch which you can
 > use to see that the behavior I described as seems to be desired is
 > not achieved.

I don't know what is the behavior above and is not achieved ?


Chen Hujaun
(2013/01/20 3:52), dmp wrote:
> I now have confirmed the issue, but Chen your patch does not I believe address
> the underlining behavior that should be expected from the PgJDBC driver and
> that is this I believe.
>
> logLevel may be set via two methods.
>
> ***************************************
> 1. org.postgresql.Driver.setLogLevel()
>
> 2. User defined logLevel setting via the connection properties
> be it through the DriverManager or DataSource.
> ***************************************
>
> I think most would agree and I have confirmed via the existing PgJDBC
> behavior that (2), the user setting of the logLevel should override the
> method (1), Driver.setLogLevel() at all times.
>
> The patch Chen does not seem to produce this result and perhaps that is
> why it is confusing. It is true as indicated that if the user does not
> user DataSource.setLogLevel() then no matter what the Driver.setLogLevel()
> is set for INFO, DEBUG, it will always be the default 0 when using the
> DataSource connection creation.
>
> Now I have wasted over an hour in determing the extent of this
> reports validity and whether the patch is good.
>
> I suggest that patches not be accepted unless sample code is supplied
> that validates/demostrates the bug and if a patch is submitted it is
> tested to address the failure via the sample code. Patches should I
> believe as has been addressed before almost always accompanied by
> test code to validate that will added to the existing code base.
>
> Chen I have rebuilt the driver to include your patch which you can
> use to see that the behavior I described as seems to be desired is
> not achieved.
>
> http://dandymadeproductions.com/temp/postgresql-loglevel.jdbc4.jar
>
> danap.
>
>
> Chen Huajun wrote:
>> > Yes, it might be better, but I don't see how it fails otherwise ?
>>
>> Let me describe it again
>> 1)Driver has a global LogLevel setting by the following
>> org.postgresql.Driver.setLogLevel()
>>
>> 2)each Connection has their own LogLevel,default is the same as
>> Driver's,and can be rewrited by url,for example
>> jdbc:postgresql://localhost/test?loglevel=1
>>
>> protected AbstractJdbc2Connection(HostSpec[] hostSpecs, String user,
>> String database, Properties info, String url) throws SQLException
>> {
>> ...
>> int logLevel = Driver.getLogLevel();
>> String connectionLogLevel = info.getProperty("loglevel");
>> if (connectionLogLevel != null) {
>> try {
>> logLevel = Integer.parseInt(connectionLogLevel);
>> } catch (Exception l_e) {
>> // XXX revisit
>> // invalid value for loglevel; ignore it
>> }
>> }
>> ...
>> }
>>
>> 3)BaseDataSource.getConnection() will append loglevel to url regardless
>> BaseDataSource.setLogLevel() was not be called
>> So,the url should be as the following,and the Connection's log is off.
>> jdbc:postgresql://localhost/test?loglevel=0
>
>
>

Hello Chen,

Lets look at the current behavior without your patch of
both DriverManager and DataSource.

Note:

1. X Indicating Parameter not set
2. Boundary cases <0 and >2
3. DriverManager.getConnection() is the standard as
    Chen you have stated, which DataSource should duplicate.

DriverManager.getConnection(connectionString);

Driver.setLogLevel() | ConnectionString &logLevel | Logging

     X    <0    No Logging
     X     0    No Logging
     X     1    INFO Logging
     X     2    DEBUG Logging
     X    >2    DEBUG Logging
----------------------------------------
    <0    <0    No Logging
    <0     0    No Logging
    <0     1    INFO Logging
    <0     2    DEBUG Logging
    <0    >2    DEBUG Logging
----------------------------------------
     0      <0    No Logging
     0     0    No Logging
     0     1    INFO Logging
     0     2    DEBUG Logging
     0    >2    DEBUG Logging
----------------------------------------
     1    <0    No Logging
     1     0    No Logging

You can  continue down this path, but the behavior
will always be ConnectionString &logLevel overrides
Driver.setLogLevel(). Lets May sure we have the
last case though in the sequence which on review
seems to operate appropriately with logLevel now
following Driver.setLogLevel() when logLevel in
the connectionString is not set.

-----------------------------------------
    <0    X    No Logging
     0    X    No Logging
     1    X    INFO Logging
     2    X    DEBUG Logging
     >2    X    DEBUG Logging

DataSource.getConnection()

Driver.setLogLevel() | DataSource.setLogLevel() | Logging

     X    <0    No Logging
     X     0    No Logging
     X     1    INFO Logging
     X     2    DEBUG Logging
     X    >2    DEBUG Logging
----------------------------------------
    <0    <0    No Logging
    <0     0    No Logging
    <0     1    INFO Logging
    <0     2    DEBUG Logging
    <0    >2    DEBUG Logging
----------------------------------------
     0      <0    No Logging
     0     0    No Logging
     0     1    INFO Logging
     0     2    DEBUG Logging
     0    >2    DEBUG Logging
----------------------------------------
     1    <0    No Logging
     1     0    No Logging

Again you can continue down this path and everything
looks good with DataSource.setLogLevel() overriding
Driver.setLogLevel(). Lets not forget though the last
case though which is broken as you pointed out.

-----------------------------------------
    <0    X    No Logging
     0    X    No Logging
     1    X    No Logging
     2    X    No Logging
     >2    X    No Logging

Now lets look at the current behavior your patch has which
I will present just one case where it breaks the standard
set by the DriverManager.getConnection().

DataSource.getConnection()

Driver.setLogLevel() | DataSource.setLogLevel() | Logging

-----------------------------------------
       1      <0    No Logging
     1     0    INFO Logging <<<---- Breaks standard
     1     1    INFO Logging
     1     2    DEBUG Logging
     1    >2    DEBUG Logging

Chen,

If you would create sample code to run to validate both your
bug report and test your patch you may have seen the breakage.
I can tell you that in all cases when DataSource.SetLogLevel(0)
your patch breaks the standard of the user overrides the
Driver.setLogLevel() when it is stipulated.

I know that valid inputs maybe only be Driver.INFO & Driver.DEBUG,
but if you treat the PgJDBC has a black box which you stimulate
and watch the output then we can not ignore the boundard cases of
<0, 0, & >2.

danap.


Chen Huajun wrote:
>
> danap I am also a bit confused for what's the expected behavior of
> PgJDBC driver.

 > If you would create sample code to run to validate both your
 > bug report and test your patch you may have seen the breakage.
 > I can tell you that in all cases when DataSource.SetLogLevel(0)
 > your patch breaks the standard of the user overrides the
 > Driver.setLogLevel() when it is stipulated.
 >
 > I know that valid inputs maybe only be Driver.INFO & Driver.DEBUG,
 > but if you treat the PgJDBC has a black box which you stimulate
 > and watch the output then we can not ignore the boundard cases of
 > <0, 0, & >2.

I had considered the above problem when made the patch,
and i thought 0 is a invalid input and ignored it.
Normally,a invalid input should lead to one of the following results.
1)a error
2)a undefined behavior
I think a undefined behavior can not give user any guarantee,
and it is not needed for modification to keep the undefined behavior same as past.

However,i alse begin to agree with you danap,May be it is better to keep the behavior
of DataSource.SetLogLevel(0) same as past and same as other invalid input(<0),
consider that implementation for the guarantee seems not so hard.

By the way, I suggest to add a new loglevel such as Driver.NONE(0) to
  explicitly set log off.So when Driver.setLogLevel() turn the log on,
DriverManager.getConnection() or DataSource.setLogLevel() can
turn the log off by a legal input Driver.NONE(0).

Chen Huajun
(2013/01/21 2:19), dmp wrote:
> Hello Chen,
>
> Lets look at the current behavior without your patch of
> both DriverManager and DataSource.
>
> Note:
>
> 1. X Indicating Parameter not set
> 2. Boundary cases <0 and >2
> 3. DriverManager.getConnection() is the standard as
> Chen you have stated, which DataSource should duplicate.
>
> DriverManager.getConnection(connectionString);
>
> Driver.setLogLevel() | ConnectionString &logLevel | Logging
>
> X <0 No Logging
> X 0 No Logging
> X 1 INFO Logging
> X 2 DEBUG Logging
> X >2 DEBUG Logging
> ----------------------------------------
> <0 <0 No Logging
> <0 0 No Logging
> <0 1 INFO Logging
> <0 2 DEBUG Logging
> <0 >2 DEBUG Logging
> ----------------------------------------
> 0 <0 No Logging
> 0 0 No Logging
> 0 1 INFO Logging
> 0 2 DEBUG Logging
> 0 >2 DEBUG Logging
> ----------------------------------------
> 1 <0 No Logging
> 1 0 No Logging
>
> You can continue down this path, but the behavior
> will always be ConnectionString &logLevel overrides
> Driver.setLogLevel(). Lets May sure we have the
> last case though in the sequence which on review
> seems to operate appropriately with logLevel now
> following Driver.setLogLevel() when logLevel in
> the connectionString is not set.
>
> -----------------------------------------
> <0 X No Logging
> 0 X No Logging
> 1 X INFO Logging
> 2 X DEBUG Logging
>  >2 X DEBUG Logging
>
> DataSource.getConnection()
>
> Driver.setLogLevel() | DataSource.setLogLevel() | Logging
>
> X <0 No Logging
> X 0 No Logging
> X 1 INFO Logging
> X 2 DEBUG Logging
> X >2 DEBUG Logging
> ----------------------------------------
> <0 <0 No Logging
> <0 0 No Logging
> <0 1 INFO Logging
> <0 2 DEBUG Logging
> <0 >2 DEBUG Logging
> ----------------------------------------
> 0 <0 No Logging
> 0 0 No Logging
> 0 1 INFO Logging
> 0 2 DEBUG Logging
> 0 >2 DEBUG Logging
> ----------------------------------------
> 1 <0 No Logging
> 1 0 No Logging
>
> Again you can continue down this path and everything
> looks good with DataSource.setLogLevel() overriding
> Driver.setLogLevel(). Lets not forget though the last
> case though which is broken as you pointed out.
>
> -----------------------------------------
> <0 X No Logging
> 0 X No Logging
> 1 X No Logging
> 2 X No Logging
>  >2 X No Logging
>
> Now lets look at the current behavior your patch has which
> I will present just one case where it breaks the standard
> set by the DriverManager.getConnection().
>
> DataSource.getConnection()
>
> Driver.setLogLevel() | DataSource.setLogLevel() | Logging
>
> -----------------------------------------
> 1 <0 No Logging
> 1 0 INFO Logging <<<---- Breaks standard
> 1 1 INFO Logging
> 1 2 DEBUG Logging
> 1 >2 DEBUG Logging
>
> Chen,
>
> If you would create sample code to run to validate both your
> bug report and test your patch you may have seen the breakage.
> I can tell you that in all cases when DataSource.SetLogLevel(0)
> your patch breaks the standard of the user overrides the
> Driver.setLogLevel() when it is stipulated.
>
> I know that valid inputs maybe only be Driver.INFO & Driver.DEBUG,
> but if you treat the PgJDBC has a black box which you stimulate
> and watch the output then we can not ignore the boundard cases of
> <0, 0, & >2.
>
> danap.
>
>
> Chen Huajun wrote:
>>
>> danap I am also a bit confused for what's the expected behavior of
>> PgJDBC driver.
>
>

Hi

>
>  > If you would create sample code to run to validate both your
>  > bug report and test your patch you may have seen the breakage.
>  > I can tell you that in all cases when DataSource.SetLogLevel(0)
>  > your patch breaks the standard of the user overrides the
>  > Driver.setLogLevel() when it is stipulated.
>  >
>  > I know that valid inputs maybe only be Driver.INFO & Driver.DEBUG,
>  > but if you treat the PgJDBC has a black box which you stimulate
>  > and watch the output then we can not ignore the boundard cases of
>  > <0, 0, & >2.
>

I made a new patch to avoid the above problem.


But i found a new another problem.
In the following code,source2.getConnection() fail to output log as expected.
----------------------------------------
PGPoolingDataSourcesource = new PGPoolingDataSource();
source.setLogLevel(2);
Connection con = source.getConnection();//OK, log does output

Properties props = new Properties();
props.put(InitialContext.INITIAL_CONTEXT_FACTORY,  "org.postgresql.test.util.MiniJndiContextFactory");
InitialContext initialContext = new InitialContext(props);
initialContext.rebind("DataSource", source);

BaseDataSource source2 = (BaseDataSource) initialContext.lookup("DataSource");
con = source2.getConnection();//NG, log does not output
----------------------------------------

The reason is that loadBaseDataSource(BaseDataSource ds, Reference ref)
fail to copy all properties from ref when source2 was created.
So the loglevel of source2 remains to be the defualt value 0.
Not only loglevel, some other properties such as "ssl" have the same problem.



Chen Huajun

> I would not expect it to copy the loglevel property. Why do you expect this behaviour, is there a specification that
youcan refer me to ? 

I have not that specification, i just think it seems to be reasonable since
PgJDBC manual(*1) said it will create a "a new instance with the same settings".

*1)http://jdbc.postgresql.org/documentation/head/jndi.html

In this case,loglevel may not be a good sample, in consideration of it does not be listed in manual(*2),but "ssl" does.

*2)http://jdbc.postgresql.org/documentation/head/ds-cpds.html

Sample for "ssl" looks like that.

-------------------------------------------------
PGPoolingDataSource source = new PGPoolingDataSource();
source.setSsl(true);
initialContext.rebind("DataSource", source);

BaseDataSource source2 = (BaseDataSource) initialContext.lookup("DataSource");
System.out.println(source2.getSsl());//the result is false
-------------------------------------------------


Chen Huajun
(2013/01/21 19:21), Dave Cramer wrote:
>
>
> On Mon, Jan 21, 2013 at 5:27 AM, Chen Huajun <chenhj@cn.fujitsu.com <mailto:chenhj@cn.fujitsu.com>> wrote:
>
>     Hi
>
>
>
>          > If you would create sample code to run to validate both your
>          > bug report and test your patch you may have seen the breakage.
>          > I can tell you that in all cases when DataSource.SetLogLevel(0)
>          > your patch breaks the standard of the user overrides the
>          > Driver.setLogLevel() when it is stipulated.
>          >
>          > I know that valid inputs maybe only be Driver.INFO & Driver.DEBUG,
>          > but if you treat the PgJDBC has a black box which you stimulate
>          > and watch the output then we can not ignore the boundard cases of
>          > <0, 0, & >2.
>
>
>     I made a new patch to avoid the above problem.
>
>
>     But i found a new another problem.
>     In the following code,source2.getConnection() fail to output log as expected.
>     ------------------------------__----------
>     PGPoolingDataSourcesource = new PGPoolingDataSource();
>     source.setLogLevel(2);
>     Connection con = source.getConnection();//OK, log does output
>
>     Properties props = new Properties();
>     props.put(InitialContext.__INITIAL_CONTEXT_FACTORY, "org.postgresql.test.util.__MiniJndiContextFactory");
>     InitialContext initialContext = new InitialContext(props);
>     initialContext.rebind("__DataSource", source);
>
>     BaseDataSource source2 = (BaseDataSource) initialContext.lookup("__DataSource");
>     con = source2.getConnection();//NG, log does not output
>     ------------------------------__----------
>
>     The reason is that loadBaseDataSource(__BaseDataSource ds, Reference ref)
>     fail to copy all properties from ref when source2 was created.
>     So the loglevel of source2 remains to be the defualt value 0.
>     Not only loglevel, some other properties such as "ssl" have the same problem.
>
>
>
> I would not expect it to copy the loglevel property. Why do you expect this behaviour, is there a specification that
youcan refer me to ? 
>
>
>
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca <http://www.credativ.ca/>
>

Chen,

Could you and possibly while considering the new possible bug you found with
copying DataSource do a more through review of the BaseDataSource class?

Then upon a review submit a complete patch to address not only the logLevel, but
any other issues you may find. In this way Dave may not have to deal with
this bug in a piecemeal fashion. You could also include with the complete
patch(s) the modification for Driver.NONE = 0 also, since I think you are
correct with this.

The reason I ask this is upon your discovery and my looking at that class
I suspect that some of the other parameters beside logLevel also have similar
problem. Also I think it best to deal with one issue at a time, so deal
with the original bug first, while considering the second then we can deal
with the copy/clone DataSource.

I think the patch you did send in today is the correct approach for logLevel.
Also a DataSource should in being cloned probably copy the current properties,
but that is only my first thought without review and full understanding.

danap.

danap,

 > Could you and possibly while considering the new possible bug you found with
 > copying DataSource do a more through review of the BaseDataSource class?

OK,I will review the BaseDataSource class first.

--
Best Regards,
Chen Huajun
(2013/01/22 0:45), dmp wrote:
> Chen,
>
> Could you and possibly while considering the new possible bug you found with
> copying DataSource do a more through review of the BaseDataSource class?
>
> Then upon a review submit a complete patch to address not only the logLevel, but any other issues you may find. In
thisway Dave may not have to deal with 
> this bug in a piecemeal fashion. You could also include with the complete
> patch(s) the modification for Driver.NONE = 0 also, since I think you are
> correct with this.
>
> The reason I ask this is upon your discovery and my looking at that class
> I suspect that some of the other parameters beside logLevel also have similar
> problem. Also I think it best to deal with one issue at a time, so deal
> with the original bug first, while considering the second then we can deal
> with the copy/clone DataSource.
>
> I think the patch you did send in today is the correct approach for logLevel.
> Also a DataSource should in being cloned probably copy the current properties, but that is only my first thought
withoutreview and full understanding. 
>
> danap.
>
>

Thank you sir for helping in identifying problems with the pgjdbc
code and contributing to maintaining this project.

danap.

Chen Huajun wrote:
> danap,
>
>  > Could you and possibly while considering the new possible bug you
> found with
>  > copying DataSource do a more through review of the BaseDataSource class?
>
> OK,I will review the BaseDataSource class first.
>

Hi

I reviewed the source related to properties.
My idea is that the set of supported properties should be the same
no matter the means of setting value(by url or by DataSource).
So I get all properties by searching keyword ".getProperty(" in pgjdbc source,
and then check whether BaseDataSource has correctly deal with all them.
As a result i found many inconsistencies as listed in the attachment.

I am wondering about if it's needed to add all lacked properties to BaseDataSource?


Chen Huajun
(2013/01/24 2:35), dmp wrote:
> Thank you sir for helping in identifying problems with the pgjdbc
> code and contributing to maintaining this project.
>
> danap.
>
> Chen Huajun wrote:
>> danap,
>>
>> > Could you and possibly while considering the new possible bug you
>> found with
>> > copying DataSource do a more through review of the BaseDataSource class?
>>
>> OK,I will review the BaseDataSource class first.
>>
>
>
>

Indeed Chen this is very good work. Thank you providing this information.

Dave though it should be the ultimate goal to have the one to one match
for all supported properties in the BaseDataSource this was not my initial
intend for asking for the review of BaseDataSource.

Chen you have defined the complete solution, but I was more interested
in checking the existing BaseDataSource Properties being properly implemented
because of the bug you found with logLevel. I suspect that other properties
that are now implemented have the same bug.

I would advocate that fixing the existing class is more important at this
time then implementing the complete properties set. Of course if this is
acceptable to you Chen for a course of action it is up to you.

Recommend:

1. Dave add tasks to get the Manual to the same state as existing properties
    that are implemented. So lacking documentation are:

    ApplicationName
    binaryTransfer
    binaryTransferDisable
    binaryTransferEnable
    receiveBufferSize
    sendBufferSize

2. Chen, No I do not think at this time that ALL properties be implemented
    in BaseDataSource at this time unless you desire to do so. As indicated
    my initial concern was properties that are implemented and have the same
    bug as you have already identified with logLevel.

3. The fourth column Chen in your spreadsheet is usefull in identifying two
    properties that are defined in the BaseDataSource but not implemented
    in getURL(). The properties user & password are included in getConnection()
    and therefore do not not need to be in getURL(). The two not implemented:

    binaryTransferDisable
    binaryTransferEnable

    These two should probably be added since that were already defined,
    but not implemented.

danap.


Dave Cramer wrote:
> Chen,
>
> This is excellent work.
>
> I don't see any reason why all of the documented url parameters should
> not be available manually, and in both the data source and the connection.
>
> Dave
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Mon, Jan 28, 2013 at 6:28 AM, Chen Huajun <chenhj@cn.fujitsu.com
> <mailto:chenhj@cn.fujitsu.com>> wrote:
>
>     Hi
>
>     I reviewed the source related to properties.
>     My idea is that the set of supported properties should be the same
>     no matter the means of setting value(by url or by DataSource).
>     So I get all properties by searching keyword ".getProperty(" in
>     pgjdbc source,
>     and then check whether BaseDataSource has correctly deal with all them.
>     As a result i found many inconsistencies as listed in the attachment.
>
>     I am wondering about if it's needed to add all lacked properties to
>     BaseDataSource?
>
>
>     Chen Huajun
>
>     (2013/01/24 2:35), dmp wrote:
>
>         Thank you sir for helping in identifying problems with the pgjdbc
>         code and contributing to maintaining this project.
>
>         danap.
>
>         Chen Huajun wrote:
>
>             danap,
>
>              > Could you and possibly while considering the new possible
>             bug you
>             found with
>              > copying DataSource do a more through review of the
>             BaseDataSource class?
>
>             OK,I will review the BaseDataSource class first.

 > 1. Dave add tasks to get the Manual to the same state as existing properties
 > that are implemented. So lacking documentation are:
 >
 > ApplicationName
 > binaryTransfer
 > binaryTransferDisable
 > binaryTransferEnable
 > receiveBufferSize
 > sendBufferSize

My work was not based the newest source,
so i checked again ,and found the "readOnly" was alse lacked.

 > 2. Chen, No I do not think at this time that ALL properties be implemented
 > in BaseDataSource at this time unless you desire to do so. As indicated
 > my initial concern was properties that are implemented and have the same
 > bug as you have already identified with logLevel.

I agree with you danap.

 > 3. The fourth column Chen in your spreadsheet is usefull in identifying two
 > properties that are defined in the BaseDataSource but not implemented
 > in getURL(). The properties user & password are included in getConnection()
 > and therefore do not not need to be in getURL(). The two not implemented:

I advocate user & password also need to be in getURL().
Because getURL() is the key way to affect a connection created by the DataSource.
If a property not need be included in getURL(),it also seems not needed to appear in BaseDataSource.
And just like DriverManager,by which can set user & password via getConnection's arguments or url,
DataSource should same to DriverManager.

Connection db = DriverManager.getConnection(url, username, password);

or

String url = "jdbc:postgresql://localhost/test?user=fred&password=secret&ssl=true";
Connection conn = DriverManager.getConnection(url);

Chen Huajun
(2013/01/29 1:54), dmp wrote:
> Indeed Chen this is very good work. Thank you providing this information.
>
> Dave though it should be the ultimate goal to have the one to one match
> for all supported properties in the BaseDataSource this was not my initial
> intend for asking for the review of BaseDataSource.
>
> Chen you have defined the complete solution, but I was more interested
> in checking the existing BaseDataSource Properties being properly implemented
> because of the bug you found with logLevel. I suspect that other properties
> that are now implemented have the same bug.
>
> I would advocate that fixing the existing class is more important at this
> time then implementing the complete properties set. Of course if this is
> acceptable to you Chen for a course of action it is up to you.
>
> Recommend:
>
> 1. Dave add tasks to get the Manual to the same state as existing properties
> that are implemented. So lacking documentation are:
>
> ApplicationName
> binaryTransfer
> binaryTransferDisable
> binaryTransferEnable
> receiveBufferSize
> sendBufferSize
>
> 2. Chen, No I do not think at this time that ALL properties be implemented
> in BaseDataSource at this time unless you desire to do so. As indicated
> my initial concern was properties that are implemented and have the same
> bug as you have already identified with logLevel.
>
> 3. The fourth column Chen in your spreadsheet is usefull in identifying two
> properties that are defined in the BaseDataSource but not implemented
> in getURL(). The properties user & password are included in getConnection()
> and therefore do not not need to be in getURL(). The two not implemented:
>
> binaryTransferDisable
> binaryTransferEnable
>
> These two should probably be added since that were already defined,
> but not implemented.
>
> danap.
>
>
> Dave Cramer wrote:
>> Chen,
>>
>> This is excellent work.
>>
>> I don't see any reason why all of the documented url parameters should
>> not be available manually, and in both the data source and the connection.
>>
>> Dave
>>
>> Dave Cramer
>>
>> dave.cramer(at)credativ(dot)ca
>> http://www.credativ.ca
>>
>>
>> On Mon, Jan 28, 2013 at 6:28 AM, Chen Huajun <chenhj@cn.fujitsu.com
>> <mailto:chenhj@cn.fujitsu.com>> wrote:
>>
>> Hi
>>
>> I reviewed the source related to properties.
>> My idea is that the set of supported properties should be the same
>> no matter the means of setting value(by url or by DataSource).
>> So I get all properties by searching keyword ".getProperty(" in
>> pgjdbc source,
>> and then check whether BaseDataSource has correctly deal with all them.
>> As a result i found many inconsistencies as listed in the attachment.
>>
>> I am wondering about if it's needed to add all lacked properties to
>> BaseDataSource?
>>
>>
>> Chen Huajun
>>
>> (2013/01/24 2:35), dmp wrote:
>>
>> Thank you sir for helping in identifying problems with the pgjdbc
>> code and contributing to maintaining this project.
>>
>> danap.
>>
>> Chen Huajun wrote:
>>
>> danap,
>>
>> > Could you and possibly while considering the new possible
>> bug you
>> found with
>> > copying DataSource do a more through review of the
>> BaseDataSource class?
>>
>> OK,I will review the BaseDataSource class first.
>
>
>

 >> dmp wote:
>> 3. The fourth column Chen in your spreadsheet is usefull in identifying two
>> properties that are defined in the BaseDataSource but not implemented
>> in getURL(). The properties user & password are included in getConnection()
>> and therefore do not not need to be in getURL(). The two not implemented:
>

Chen Huajun wrote:
> I advocate user & password also need to be in getURL().
> Because getURL() is the key way to affect a connection created by the DataSource.
> If a property not need be included in getURL(),it also seems not needed to appear in BaseDataSource.
> And just like DriverManager,by which can set user & password via getConnection's arguments or url,
> DataSource should same to DriverManager.
>
> Connection db = DriverManager.getConnection(url, username, password);
>
> or
>
> String url = "jdbc:postgresql://localhost/test?user=fred&password=secret&ssl=true";
> Connection conn = DriverManager.getConnection(url);

If this is a route you would like to take then that can be included with the
patch for review. The Java API for the DataSource though only indicates the
two methods:

Connection getConnection()
Connection getConnection(String username, String password)

Therefore they must stay. The two additional methods you advocate only make
more work, but has you say allows more flexibility.

Chen, this project is really short of contributors so any major changes to the
code must be accompanied by testing on your part. Maintenance seems to be me
to be the main priority until that changes. That is why since you were familiar
with the BaseDataSource logLevel bug I asked if you could review, to see if
there were other similar bugs in that class for the properties.

danap.

Hi

I have made a new patch for BaseDataSource ,please check it.
Some modifications in getReference() is just for keeping
the same style to other String parameters .

Chen Huajun
(2013/01/30 1:23), dmp wrote:
>
>  >> dmp wote:
>>> 3. The fourth column Chen in your spreadsheet is usefull in identifying two
>>> properties that are defined in the BaseDataSource but not implemented
>>> in getURL(). The properties user & password are included in getConnection()
>>> and therefore do not not need to be in getURL(). The two not implemented:
>>
>
> Chen Huajun wrote:
>> I advocate user & password also need to be in getURL().
>> Because getURL() is the key way to affect a connection created by the DataSource.
>> If a property not need be included in getURL(),it also seems not needed to appear in BaseDataSource.
>> And just like DriverManager,by which can set user & password via getConnection's arguments or url,
>> DataSource should same to DriverManager.
>>
>> Connection db = DriverManager.getConnection(url, username, password);
>>
>> or
>>
>> String url = "jdbc:postgresql://localhost/test?user=fred&password=secret&ssl=true";
>> Connection conn = DriverManager.getConnection(url);
>
> If this is a route you would like to take then that can be included with the
> patch for review. The Java API for the DataSource though only indicates the
> two methods:
>
> Connection getConnection()
> Connection getConnection(String username, String password)
>
> Therefore they must stay. The two additional methods you advocate only make
> more work, but has you say allows more flexibility.
>
> Chen, this project is really short of contributors so any major changes to the
> code must be accompanied by testing on your part. Maintenance seems to be me
> to be the main priority until that changes. That is why since you were familiar
> with the BaseDataSource logLevel bug I asked if you could review, to see if
> there were other similar bugs in that class for the properties.
>
> danap.
>
>
>
>

--
Best Regards
--------------------------------------------------
   富士通南大軟件技術有限公司（FNST)
   第二ソフトウェア事業部第三開発部
   陳華軍（ﾁﾝ ｶｸﾞﾝ）
   Addr: 南京富士通南大軟件技術有限公司（FNST)
         中国南京市雨花台区文竹路6号（210012）
   Mail: chenhj@cn.fujitsu.com
   Tel : +86+25-86630566-8406  内線: 7998-8406
   Fax : +86+25-83317685
--------------------------------------------------

Hello,

I can review this tomorrow and get back by Monday, or sooner.

danap.

Chen Huajun wrote:
> Hi
>
> I have made a new patch for BaseDataSource ,please check it.
> Some modifications in getReference() is just for keeping
> the same style to other String parameters .
>
> Chen Huajun

danap,

Please use the new patch.
I add a test case,and fixed a mistake in the old one.

Chen Huajun
(2013/02/03 10:32), dmp wrote:
> Hello,
>
> I can review this tomorrow and get back by Monday, or sooner.
>
> danap.
>
> Chen Huajun wrote:
>> Hi
>>
>> I have made a new patch for BaseDataSource ,please check it.
>> Some modifications in getReference() is just for keeping
>> the same style to other String parameters .
>>
>> Chen Huajun
>
>

Review for patch:

org/postgresql/ds/common/BaseDataSource:

1. databaseName - Was null possibly so getURL(), getReference(), &
    writeBaseObject() checked,
    OK

2. binaryTransferEnable/Disable - Same as databaseName for writeBaseObject(),
    OK

3. logLevelSet - Needed to Address Bug this patch applies to directly,
    OK

4. receiveBufferSize & sendBufferSize - Addresses lack of getter Methods,
    OK

5. getURL() - Changed from private to public, Why?, DriverManager Contains
    no such public method nor Does the Java API define for interface
    DataSource,
    OK???

6. user & password - Even if 5. approved public why would these be open to
    a public interface for returning these values with URL. In a basic
    instinct I feel this is security risk.
    NOT OK

7. sslFactory - Proper check for null in getReference(),
    OK

8. applicationName - Proper check for null in getReference(),
    OK

org/postgresql/test/jdbc2/optional/BaseDataSourceTest:

1. I'm not proficient with JUnit, but appears to add testing for getURL()
    in 5. above. If that is not approved then test suite addition is
    not needed.
    OK?

danap

Chen Huajun wrote:
> danap,
>
> Please use the new patch.
> I add a test case,and fixed a mistake in the old one.
>
> Chen Huajun
> (2013/02/03 10:32), dmp wrote:
>> Hello,
>>
>> I can review this tomorrow and get back by Monday, or sooner.
>>
>> danap.
>>
>> Chen Huajun wrote:
>>> Hi
>>>
>>> I have made a new patch for BaseDataSource ,please check it.
>>> Some modifications in getReference() is just for keeping
>>> the same style to other String parameters .
>>>
>>> Chen Huajun

danap,

 > 5. getURL() - Changed from private to public, Why?, DriverManager Contains
 > no such public method nor Does the Java API define for interface
 > DataSource,
 > OK???

The changing is just for testing.
It is also useful to compare two DataSource(for instance one is a copy of another )
But if just for testing,now i have an another idea,
implementing toString() method which just call getURL().
What about that?

 > 6. user & password - Even if 5. approved public why would these be open to
 > a public interface for returning these values with URL. In a basic
 > instinct I feel this is security risk.
 > NOT OK

oh,they are not needed just as you said.
Sorry,i failed to understood your reply.
 > in getURL(). The properties user & password are included in getConnection()
 > and therefore do not not need to be in getURL().

Chen Huajun
(2013/02/04 4:44), dmp wrote:
> Review for patch:
>
> org/postgresql/ds/common/BaseDataSource:
>
> 1. databaseName - Was null possibly so getURL(), getReference(), &
> writeBaseObject() checked,
> OK
>
> 2. binaryTransferEnable/Disable - Same as databaseName for writeBaseObject(),
> OK
>
> 3. logLevelSet - Needed to Address Bug this patch applies to directly,
> OK
>
> 4. receiveBufferSize & sendBufferSize - Addresses lack of getter Methods,
> OK
>
> 5. getURL() - Changed from private to public, Why?, DriverManager Contains
> no such public method nor Does the Java API define for interface
> DataSource,
> OK???
>
> 6. user & password - Even if 5. approved public why would these be open to
> a public interface for returning these values with URL. In a basic
> instinct I feel this is security risk.
> NOT OK
>
> 7. sslFactory - Proper check for null in getReference(),
> OK
>
> 8. applicationName - Proper check for null in getReference(),
> OK
>
> org/postgresql/test/jdbc2/optional/BaseDataSourceTest:
>
> 1. I'm not proficient with JUnit, but appears to add testing for getURL()
> in 5. above. If that is not approved then test suite addition is
> not needed.
> OK?
>
> danap
>
> Chen Huajun wrote:
>> danap,
>>
>> Please use the new patch.
>> I add a test case,and fixed a mistake in the old one.
>>
>> Chen Huajun
>> (2013/02/03 10:32), dmp wrote:
>>> Hello,
>>>
>>> I can review this tomorrow and get back by Monday, or sooner.
>>>
>>> danap.
>>>
>>> Chen Huajun wrote:
>>>> Hi
>>>>
>>>> I have made a new patch for BaseDataSource ,please check it.
>>>> Some modifications in getReference() is just for keeping
>>>> the same style to other String parameters .
>>>>
>>>> Chen Huajun
>
>

Hi

According the review result,and i modified the patch.
Deleted user & password from getUrl() and resumed getUrl() to private.

I think the following should be considered in the future, even if it 's really needed.
 > The changing is just for testing.
 > It is also useful to compare two DataSource(for instance one is a copy of another )
 > But if just for testing,now i have an another idea,
 > implementing toString() method which just call getURL()

Chen Huajun
(2013/02/04 10:28), Chen Huajun wrote:
> danap,
>
>  > 5. getURL() - Changed from private to public, Why?, DriverManager Contains
>  > no such public method nor Does the Java API define for interface
>  > DataSource,
>  > OK???
>
> The changing is just for testing.
> It is also useful to compare two DataSource(for instance one is a copy of another )
> But if just for testing,now i have an another idea,
> implementing toString() method which just call getURL().
> What about that?
>
>  > 6. user & password - Even if 5. approved public why would these be open to
>  > a public interface for returning these values with URL. In a basic
>  > instinct I feel this is security risk.
>  > NOT OK
>
> oh,they are not needed just as you said.
> Sorry,i failed to understood your reply.
>  > in getURL(). The properties user & password are included in getConnection()
>  > and therefore do not not need to be in getURL().
>
> Chen Huajun
> (2013/02/04 4:44), dmp wrote:
>> Review for patch:
>>
>> org/postgresql/ds/common/BaseDataSource:
>>
>> 1. databaseName - Was null possibly so getURL(), getReference(), &
>> writeBaseObject() checked,
>> OK
>>
>> 2. binaryTransferEnable/Disable - Same as databaseName for writeBaseObject(),
>> OK
>>
>> 3. logLevelSet - Needed to Address Bug this patch applies to directly,
>> OK
>>
>> 4. receiveBufferSize & sendBufferSize - Addresses lack of getter Methods,
>> OK
>>
>> 5. getURL() - Changed from private to public, Why?, DriverManager Contains
>> no such public method nor Does the Java API define for interface
>> DataSource,
>> OK???
>>
>> 6. user & password - Even if 5. approved public why would these be open to
>> a public interface for returning these values with URL. In a basic
>> instinct I feel this is security risk.
>> NOT OK
>>
>> 7. sslFactory - Proper check for null in getReference(),
>> OK
>>
>> 8. applicationName - Proper check for null in getReference(),
>> OK
>>
>> org/postgresql/test/jdbc2/optional/BaseDataSourceTest:
>>
>> 1. I'm not proficient with JUnit, but appears to add testing for getURL()
>> in 5. above. If that is not approved then test suite addition is
>> not needed.
>> OK?
>>
>> danap
>>
>> Chen Huajun wrote:
>>> danap,
>>>
>>> Please use the new patch.
>>> I add a test case,and fixed a mistake in the old one.
>>>
>>> Chen Huajun
>>> (2013/02/03 10:32), dmp wrote:
>>>> Hello,
>>>>
>>>> I can review this tomorrow and get back by Monday, or sooner.
>>>>
>>>> danap.
>>>>
>>>> Chen Huajun wrote:
>>>>> Hi
>>>>>
>>>>> I have made a new patch for BaseDataSource ,please check it.
>>>>> Some modifications in getReference() is just for keeping
>>>>> the same style to other String parameters .
>>>>>
>>>>> Chen Huajun
>>
>>
>
>
>
>

Dave,

This new patch from Chen addresses 5. & 6. in my reivew. Those aspects
removed. The patch seems to address properly the BUG identified in the
logLevel original posting & other minor fixes as identified by review.

danap.

Chen Huajun wrote:
> Hi
>
> According the review result,and i modified the patch.
> Deleted user & password from getUrl() and resumed getUrl() to private.
>
> I think the following should be considered in the future, even if it 's
> really needed.
>  > The changing is just for testing.
>  > It is also useful to compare two DataSource(for instance one is a
> copy of another )
>  > But if just for testing,now i have an another idea,
>  > implementing toString() method which just call getURL()
>
> Chen Huajun
> (2013/02/04 10:28), Chen Huajun wrote:
>> danap,
>>
>> > 5. getURL() - Changed from private to public, Why?, DriverManager
>> Contains
>> > no such public method nor Does the Java API define for interface
>> > DataSource,
>> > OK???
>>
>> The changing is just for testing.
>> It is also useful to compare two DataSource(for instance one is a copy
>> of another )
>> But if just for testing,now i have an another idea,
>> implementing toString() method which just call getURL().
>> What about that?
>>
>> > 6. user & password - Even if 5. approved public why would these be
>> open to
>> > a public interface for returning these values with URL. In a basic
>> > instinct I feel this is security risk.
>> > NOT OK
>>
>> oh,they are not needed just as you said.
>> Sorry,i failed to understood your reply.
>> > in getURL(). The properties user & password are included in
>> getConnection()
>> > and therefore do not not need to be in getURL().
>>
>> Chen Huajun
>> (2013/02/04 4:44), dmp wrote:
>>> Review for patch:
>>>
>>> org/postgresql/ds/common/BaseDataSource:
>>>
>>> 1. databaseName - Was null possibly so getURL(), getReference(), &
>>> writeBaseObject() checked,
>>> OK
>>>
>>> 2. binaryTransferEnable/Disable - Same as databaseName for
>>> writeBaseObject(),
>>> OK
>>>
>>> 3. logLevelSet - Needed to Address Bug this patch applies to directly,
>>> OK
>>>
>>> 4. receiveBufferSize & sendBufferSize - Addresses lack of getter
>>> Methods,
>>> OK
>>>
>>> 5. getURL() - Changed from private to public, Why?, DriverManager
>>> Contains
>>> no such public method nor Does the Java API define for interface
>>> DataSource,
>>> OK???
>>>
>>> 6. user & password - Even if 5. approved public why would these be
>>> open to
>>> a public interface for returning these values with URL. In a basic
>>> instinct I feel this is security risk.
>>> NOT OK
>>>
>>> 7. sslFactory - Proper check for null in getReference(),
>>> OK
>>>
>>> 8. applicationName - Proper check for null in getReference(),
>>> OK
>>>
>>> org/postgresql/test/jdbc2/optional/BaseDataSourceTest:
>>>
>>> 1. I'm not proficient with JUnit, but appears to add testing for
>>> getURL()
>>> in 5. above. If that is not approved then test suite addition is
>>> not needed.
>>> OK?
>>>
>>> danap
>>>
>>> Chen Huajun wrote:
>>>> danap,
>>>>
>>>> Please use the new patch.
>>>> I add a test case,and fixed a mistake in the old one.
>>>>
>>>> Chen Huajun
>>>> (2013/02/03 10:32), dmp wrote:
>>>>> Hello,
>>>>>
>>>>> I can review this tomorrow and get back by Monday, or sooner.
>>>>>
>>>>> danap.
>>>>>
>>>>> Chen Huajun wrote:
>>>>>> Hi
>>>>>>
>>>>>> I have made a new patch for BaseDataSource ,please check it.
>>>>>> Some modifications in getReference() is just for keeping
>>>>>> the same style to other String parameters .
>>>>>>
>>>>>> Chen Huajun