SQuirreL SQL Client

I am reviewing it now.   So far I like what I am seeing.  At the moment I
have a few questions:

1. Can we change the name of NopWhereClausePart to EmptyWhereClausePart ?
 That seems a bit more readable to me.

2. In DataSetUpdateableTableModelImpl.updateTableComponent, in your new
code, there is now this:

          // into the first (and only) variable position in the prepared
stmt
          CellComponentFactory.setPreparedStatementValue(
                 colDefs[col], pstmt, newValue, 1);
         WhereClausePartUtil.setParameters(pstmt, whereClauseParts, 2);

Would it be possible to use an additional ParameterWhereClausePart to make
the call to CellComponentFactory.setPreparedStatementValue unnecessary ?

3. Would it be too much trouble to give WhereClausePartUtil an interface
(say IWhereClausePartUtil) and have it be injectable instead of static ? (so
- a WhereClausePartUtil instance variable with a public setter that
accepts IWhereClausePartUtil in each class where it is used; This helps with
testability)

Rob


On Sat, Feb 19, 2011 at 2:45 PM, Stefan Willinger <stefan.home@...:

>  Hi Rob,
>
> after some time (unfortunate several months) I have finished the
> implementation for the patch.
>
> *First: Like TDD - let me explain, how to test the changes:*
> According to your suggestion,I have prepared 2 SQL-Scripts, which prepare
> an DB-Table (called basicTypes) for an MySQL and Oracle Database.
>
>    -
>    /squirrel-sql/src/test/java/net/sourceforge/squirrel_sql/fw/datasetviewer/cellcomponent/whereClause/mysql.sql
>    -
>    /squirrel-sql/src/test/java/net/sourceforge/squirrel_sql/fw/datasetviewer/cellcomponent/whereClause/oracle.sql
>
> The content of this table contains some characters, which must be escaped
> in an MySQL Database (according to
> http://dev.mysql.com/doc/refman/5.1/en/string-syntax.html). There existing
> 3 Test-Classes which tests the behavior of the JDBC-Driver for the escaping
> of the special characters:
>
>    -
>    /squirrel-sql/src/test/java/net/sourceforge/squirrel_sql/fw/datasetviewer/cellcomponent/whereClause/MySQLAdhocTests.java
>    -
>    /squirrel-sql/src/test/java/net/sourceforge/squirrel_sql/fw/datasetviewer/cellcomponent/whereClause/MySQLnoBackslashEscapesAdhocTests.java
>    -
>    /squirrel-sql/src/test/java/net/sourceforge/squirrel_sql/fw/datasetviewer/cellcomponent/whereClause/OracleAdhocTests.java
>
> By extending their super class (AbstractAdhocTests) it should be very
> simple to use the test for another db-driver.
>
> These provided ad-hoc tests uses CellComponentFactory.getWhereClauseValue()
> to create the where clause and count the affected rows (each row is unique)
> when we use each column (the same as Squirrel did, when updating
> table-data). So, this should be a representing test-case.
>
> For the new code, e.g. *IsNullWhereClausePart* exists new test cases e.g.
> *IsNullWhereClausePartTest.
>
> Second: The Idea behind the implementation
> *Normally the jdbc-driver is responsible to escape the special characters
> of an database, when sql-parameters are used. Until now, Squirrel build the
> where-clause for updating data or counting the affected rows without
> parameters. In the future, Squirrel uses an PreparedStatement for this. So
> the responsibility is moved to the jdbc-driver.
>
> *Third: The implementation*
> The most new code exists in the module "fw". There is a new package called
> *net.sourceforge.squirrel_sql.fw.datasetviewer.cellcomponent.whereClause*.
> The interface *IWhereClausePart* replaces the simple String return type
> of  *IDataTypeComponent.getWhereClauseValue()*.
> There are some implementations for this new interface:
>
>    - *IsNullWhereClausePart*
>    - *NoParameterWhereClausePart*
>    - *NopWhereClausePart*
>    - *ParameterWhereClausePart*
>
> The new *WhereClausePartUtil *is responsible for creating the String for
> the where-clause and setting the parameter into the PreparedStatement.
> Each implementation of the *IWhereClausePart *knows the *
> IDataTypeComponent* which created the instance because it uses the
> existing functionality for setting the parameter value.
>
>
> Now, please review the changes. And if you think this is an suitable
> solution, I would be happy if Squirrel contains this piece of code.
> If there are some things to change - let me know - I will do it.
>
>
> Stefan
>
>
>
>
>
>
> Am 2010-11-06 19:54, schrieb Stefan Willinger:
>
> Hi Rob,
>
> your comment sounds reasonable.
>
> I will finish the implementation and create a script a you recommended.
>
> At first, I will change the DataTypes by a conservative way, so that the
> actual behaviour will not be changed. In a future step it seems, that
> some DataTypes which doesn't support the where clause at the moment,
> could change their behaviour.
>
> But first I will ensure the actual functionality with PreparedStatements.
>
> OK - so far...
>
> Stefan
>
>
>
> Am 2010-11-04 01:40, schrieb Robert Manning:
>
>  Setfan,
>
> Thinking about this more, I agree that this change would be beneficial
> in the ways you have described.  However, it is a bit like major
> surgery and presents a bit of risk to include in the 3.2 release which
> we are currently preparing.  If Gerd agrees, I would like to see this
> as a patch soon after we release 3.2.  One other thing; it would be
> nice to demonstrate how effective this implementation is on various
> databases.  I am thinking perhaps 1) simple SQL script to create a
> table and populate it with data that needs to be quoted and 2) a test
> driver that connects and verifies that the PreparedStatement approach
> works as expected.  If such a test existed I have a number of
> databases setup to verify it against.  I know that this sounds like
> more of a test for JDBC drivers, but as SQuirreL depends on the
> driver, it is affected by the bugs they have.  Does that sound
> reasonable ?
>
> Rob
>
>
>

Hi Rob,

1. Can we change the name of NopWhereClausePart to EmptyWhereClausePart 
?  That seems a bit more readable to me.

    Of course. The first programming language I learned was Step 5 (the
    programming language for Simatic S5 an Programable Logic Controller
    within automation systems. Within the most build in functions you
    needed the NOP operator for the NoOperation  - Seems that the old
    times are very present in my head :-)
    I will update the code and JavaDoc.

2. Would it be possible to use an additional ParameterWhereClausePart to 
make the call to CellComponentFactory.setPreparedStatementValue unnecessary?

    Hm, with the call of CellComponentFactory.setPreparedStatementValue
    we set the data of the changed data field. In the strict sense, this
    is not an part of the where clause. If we add an additional
    ParameterWhereClausePart for the changed data - that would be like
    cheating.
    If we want do this, we could:

        * use an placeholder that is on the right (first) position which
          is responsible only for setting the parameter and not for
          creating the set=? part.
        * or enhance the functionality, that constructing the update
          statement is part of if (and maybe - insert/delete/select)
        * or another suggestion?

3. Would it be too much trouble to give WhereClausePartUtil an interface.

    Of course not. I think I didn't see an Dependency Injection
    Container within Squirrel - So would it be appropriate to create an
    instance variable initialized with an default, that could be
    overwritten by the setter at each class where is is used?


I will prepare the first and the third part of your suggestions.

But what should we do with the second one?

Stefan


Am 2011-02-26 15:52, schrieb Robert Manning:
> I am reviewing it now.   So far I like what I am seeing.  At the 
> moment I have a few questions:
>
> 1. Can we change the name of NopWhereClausePart to Empty 
> WhereClausePart ?  That seems a bit more readable to me.
>
> 2. In DataSetUpdateableTableModelImpl.updateTableComponent, in your 
> new code, there is now this:
>
>           // into the first (and only) variable position in the 
> prepared stmt
>           CellComponentFactory.setPreparedStatementValue(
>                  colDefs[col], pstmt, newValue, 1);
>          WhereClausePartUtil.setParameters(pstmt, whereClauseParts, 2);
>
> Would it be possible to use an additional ParameterWhereClausePart to 
> make the call to CellComponentFactory.setPreparedStatementValue 
> unnecessary ?
>
> 3. Would it be too much trouble to give WhereClausePartUtil an 
> interface (say IWhereClausePartUtil) and have it be injectable instead 
> of static ? (so - a WhereClausePartUtil instance variable with a 
> public setter that accepts IWhereClausePartUtil in each class where it 
> is used; This helps with testability)
>
> Rob

For #2 - nevermind, you are absolutely correct; I mistook
setPreparedStatementValue
for setting a bind var in the where clause.

As of yet there is no IoC container being used in the main application.  For
the update application, since it must run outside of SQuirreL, and because
it was brand new I took liberty to use Spring there.  For now, an instance
variable initialized in the declaration is fine.  Should we decide to move
to IoC in the future, any statics we can avoid will help.

Rob

On Sat, Feb 26, 2011 at 2:05 PM, Stefan Willinger <stefan.home@...:

>  Hi Rob,
>
>
> 1. Can we change the name of NopWhereClausePart to EmptyWhereClausePart ?
>  That seems a bit more readable to me.
>
> Of course. The first programming language I learned was Step 5 (the
> programming language for Simatic S5 an Programable Logic Controller within
> automation systems. Within the most build in functions you needed the NOP
> operator for the NoOperation  - Seems that the old times are very present in
> my head :-)
> I will update the code and JavaDoc.
>
> 2. Would it be possible to use an additional ParameterWhereClausePart to
> make the call to CellComponentFactory.setPreparedStatementValue unnecessary?
>
> Hm, with the call of CellComponentFactory.setPreparedStatementValue we set
> the data of the changed data field. In the strict sense, this is not an part
> of the where clause. If we add an additional ParameterWhereClausePart for
> the changed data - that would be like cheating.
> If we want do this, we could:
>
>    - use an placeholder that is on the right (first) position which is
>    responsible only for setting the parameter and not for creating the set=?
>    part.
>    - or enhance the functionality, that constructing the update statement
>    is part of if (and maybe - insert/delete/select)
>    - or another suggestion?
>
>  3. Would it be too much trouble to give WhereClausePartUtil an interface.
>
> Of course not. I think I didn't see an Dependency Injection Container
> within Squirrel - So would it be appropriate to create an instance variable
> initialized with an default, that could be overwritten by the setter at each
> class where is is used?
>
>
> I will prepare the first and the third part of your suggestions.
>
> But what should we do with the second one?
>
> Stefan
>
>
> Am 2011-02-26 15:52, schrieb Robert Manning:
>
> I am reviewing it now.   So far I like what I am seeing.  At the moment I
> have a few questions:
>
>  1. Can we change the name of NopWhereClausePart to Empty WhereClausePart
> ?  That seems a bit more readable to me.
>
> 2. In DataSetUpdateableTableModelImpl.updateTableComponent, in your new
> code, there is now this:
>
>            // into the first (and only) variable position in the prepared
> stmt
>           CellComponentFactory.setPreparedStatementValue(
>                  colDefs[col], pstmt, newValue, 1);
>          WhereClausePartUtil.setParameters(pstmt, whereClauseParts, 2);
>
>  Would it be possible to use an additional ParameterWhereClausePart to
> make the call to CellComponentFactory.setPreparedStatementValue unnecessary
> ?
>
>  3. Would it be too much trouble to give WhereClausePartUtil an interface
> (say IWhereClausePartUtil) and have it be injectable instead of static ? (so
> - a WhereClausePartUtil instance variable with a public setter that
> accepts IWhereClausePartUtil in each class where it is used; This helps with
> testability)
>
>  Rob
>
>