db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel John Debrunner (JIRA)" <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-819) Provide JDBC4 SQLException subclasses support in Embedded driver
Date Thu, 16 Feb 2006 22:07:10 GMT
    [ http://issues.apache.org/jira/browse/DERBY-819?page=comments#action_12366695 ] 

Daniel John Debrunner commented on DERBY-819:

review  derby819_2.diff

-1 veto still in place: If these BLOCKER issues are addressed, the veto is lifted
      - SystemProcedure changes need to be removed (see below)
      - NPE potential at statup needs to be addressed. (see below)
       - Test code just ignores the very exceptions it is meant to be testing. (see below)

- Why is a SQLExceptionFactory and SQLExceptionFactory 40 classes needed? With the way this
is implemented, the new method
   it defines could just  included in InternalDriver and the 40 specific one in Driver40.
Seems a lot of overhead for little value.

BLOCKER - We still have a window where before the driver is loaded any InternalDriver.getSQLExceptionFactory()
could return null leading to a NullPointerException. Maybe just simply closing this by having
InternalDriver initialize the static field to a factory that returns the plain SQLException
would be sufficient.

- Copyright dates for the new files need to be 2005,2006 or just 2006 depending on when they
were created

- no class javadoc for SQLExceptionFactory40

- SQLExceptionFactory40: Don't understand why there is a method getJDBC40Exception, why not
just have the
   code in getSQLException

 -  SQLExceptionFactory40.getJDBC40Exception - No need to have the 'if (ex == null) ' just
make ex  = new SQLException
     part of the else in the above block. Then no need to initialize ex = null at the beginning
of the method. This is where Java
     is great, the compiler will tell you if ex is never initialized. WIth the code as-is
I have to look to see under what conditions
     ex can be null for the 'if (ex == null) '  if I want to understand it. Thus I waste my
time looking for a condition that can never exist.

- Removal of all the shortcut methods seems like the wrong approach to me, that's been discussed
on the list.
   It's about 90% of this patch and could have been avoided, making the real changes easier
to spot.

- Seems to be some unrelated changes included, addition of an extra parameter here.

-            throw Util.generateCsSQLException(SQLState.INVALID_API_PARAMETER,map,"map",
+            throw InternalDriver.getExceptionFactory()
+            .generateCsSQLException(SQLState.INVALID_API_PARAMETER,map,"map",

BLOCKER - Very strange changes in SystemProcedures.java, look like some global edit script
gone wrong. Makes me *very* nervous about the rest of the patch, not having the shortcut methods
removed would make the patch easier to manage and easier for the reviewers to spot issues
like this.

BLOCKER - Tests have code like this:

+        } catch (SQLIntegrityConstraintViolationException e) {
+        }

How does this test that we are setting the SQLState and message correctly in these new exceptions?
Maybe the coder accidently swapped the SQLState and message parameters.

> Provide JDBC4 SQLException subclasses support in Embedded driver
> ----------------------------------------------------------------
>          Key: DERBY-819
>          URL: http://issues.apache.org/jira/browse/DERBY-819
>      Project: Derby
>         Type: Sub-task
>   Components: JDBC
>  Environment: all
>     Reporter: Anurag Shekhar
>     Assignee: Anurag Shekhar
>     Priority: Minor
>  Attachments: .derby-819_2.stat, derby-819-onlyforreview.diff, derby-819.diff, derby-819_2.diff,

This message is automatically generated by JIRA.
If you think it was sent incorrectly contact one of the administrators:
For more information on JIRA, see:

View raw message