persistence@glassfish.java.net

Code review for Issue 2799 - MySQL: invalid SQL is generated when unqualified attribute is used in UPDATE

From: Wonseok Kim <guruwons_at_gmail.com>
Date: Tue, 10 Apr 2007 18:55:52 +0900

Tom, Michael

Please review the below fix for issue 2799
https://glassfish.dev.java.net/issues/show_bug.cgi?id=2799

* Investigation:

ParseTree.initBaseExpression() has problem when setting base expression for
base variable. For Update query which has not qualfication variable,
lower-case(canonicalized) abstrat schema name is used for base variable like
"employee", but initBaseExpression() is using abstract schema name itself
like "Employee" without canonicalizing and put "Employee" ->
ExpressionBuilder to map as base expression. This caused generating a little
different(invalid) expression for unqualified where node.

The invalid base expression(of generated QueryKeyExpression) affects
generating invalid SQL for MySQL which always uses temporary table for bulk
update.

* Fix:
Extract the method of getting canonical base variable name in
ModifyQuery.validate() and reuse the method in ParseTree.initBaseExpression
().

* Diff:

Index:
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ModifyNode.java
===================================================================
RCS file:
/cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ModifyNode.java,v
retrieving revision 1.6
diff -c -w -r1.6 ModifyNode.java
***
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ModifyNode.java
 4 Jan 2007 14:31:31 -0000 1.6
---
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ModifyNode.java
 10 Apr 2007 09:09:02 -0000
***************
*** 49,57 ****
      public void validate(ParseTreeContext context) {
          // If defined use the abstractSchemaIdentifier as the base
variable,
          // otherwise use the abstractSchemaName
!         String baseVariable = abstractSchemaIdentifier != null ?
!             abstractSchemaIdentifier : abstractSchemaName;
!         baseVariable =
IdentificationVariableDeclNode.calculateCanonicalName(baseVariable);
          context.setBaseVariable(baseVariable);
          super.validate(context);
      }
--- 49,55 ----
      public void validate(ParseTreeContext context) {
          // If defined use the abstractSchemaIdentifier as the base
variable,
          // otherwise use the abstractSchemaName
!         String baseVariable = getCanonicalAbstractSchemaIdentifier();
          context.setBaseVariable(baseVariable);
          super.validate(context);
      }
***************
*** 92,97 ****
--- 90,107 ----
      }
      /**
+      * INTERNAL:
+      * Returns the canonical name of abstract schema identifier.
+      * If the identifier is not specified(unqualified attribute scenario),
+      * the canonical name of abstract schema is returned.
+      */
+     public String getCanonicalAbstractSchemaIdentifier() {
+         String variable = abstractSchemaIdentifier != null ?
+                 abstractSchemaIdentifier : abstractSchemaName;
+         return IdentificationVariableDeclNode.calculateCanonicalName
(variable);
+     }
+
+     /**
       * resolveClass: Answer the class which corresponds to my
variableName. This is the class for
       * an alias, where the variableName is registered to an alias.
       */
Index:
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ParseTree.java
===================================================================
RCS file:
/cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ParseTree.java,v
retrieving revision 1.25
diff -c -w -r1.25 ParseTree.java
***
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ParseTree.java
 4 Jan 2007 14:31:33 -0000    1.25
---
entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/ParseTree.java
 10 Apr 2007 09:09:02 -0000
***************
*** 139,151 ****
       */
      public void initBaseExpression(ModifyAllQuery theQuery,
GenerationContext generationContext) {
          ModifyNode queryNode = (ModifyNode)getQueryNode();
!         String variable = queryNode.getAbstractSchemaIdentifier();
!         if (variable == null) {
!             // UPDATE and DELETE may not define a variable =>
!             // use schema name as variable
!             variable = queryNode.getAbstractSchemaName();
!         }
!         ParseTreeContext context = generationContext.getParseTreeContext
();
          Class referenceClass = theQuery.getReferenceClass();
          // Create a new expression builder for the reference class
          ExpressionBuilder builder = new ExpressionBuilder(referenceClass);
--- 139,145 ----
       */
      public void initBaseExpression(ModifyAllQuery theQuery,
GenerationContext generationContext) {
          ModifyNode queryNode = (ModifyNode)getQueryNode();
!         String variable = queryNode.getCanonicalAbstractSchemaIdentifier
();
          Class referenceClass = theQuery.getReferenceClass();
          // Create a new expression builder for the reference class
          ExpressionBuilder builder = new ExpressionBuilder(referenceClass);
NOTE: I also removed the line "ParseTreeContext context =
generationContext.getParseTreeContext();" which is not referenced anywhere.
* Tests:
I ran entity-persistence-tests on MySQL and Derby.
Finally, all entity-persistence-tests has passed without any failure on
MySQL InnoDB engine!
Thanks,
-Wonseok