Bug 30760 - sql task handle pl/sql with ';' in pl/sql code. (patch)
Summary: sql task handle pl/sql with ';' in pl/sql code. (patch)
Status: NEW
Alias: None
Product: Ant
Classification: Unclassified
Component: Core tasks (show other bugs)
Version: 1.6.2
Hardware: PC All
: P3 normal with 2 votes (vote)
Target Milestone: ---
Assignee: Ant Notifications List
Depends on:
Reported: 2004-08-19 18:02 UTC by Eric Van
Modified: 2008-02-22 12:18 UTC (History)
0 users

SQLExec.java patch (3.42 KB, patch)
2004-08-19 18:03 UTC, Eric Van
Details | Diff
diff of original 1.6.2 code and new patch (4.82 KB, patch)
2004-09-10 18:31 UTC, Eric Van
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Van 2004-08-19 18:02:23 UTC
ant: 1.6.2
os: linux 2.4.26
java: 1.4.2_05
db: postgresql 7.4.3

- Current <sql> task behavior:

When supplying a sql file to <sql> task any line ending in a ';'
results in the end of the statement, and that statement is sent to the
rdbms.  Problems exists when a create function call contains pl/sql code
that has line ending ';' denoting statements within the procedure.
When <sql> task encounters the ';' in the procedure only the portion of
the procedure up to the first line ending ';' is sent to the rdbms.

Ex. Procedure statement:

    theTime timestamp;
    theTime := ''now'';
    UPDATE TheTable
      SET theTime = theTime
      WHERE tableId = NEW.tableId;
' LANGUAGE 'plpgsql';

Ex. Fragment sent to rdbms:

    theTime timestamp

Although there are workarounds for this, (delimetertype="row", etc),
they all result in reformatting the sql script.  This is very
cumbersome if the script is auto generated from any sort of relational tool.

- Patch:
  i've attached a patch that fixes this issue by checking if line ending
  ';' chars occur with a quoted expression or not.

  The patch works fine on postgresql, but i have yet to test it on
  oracle, sybase, sqlserver, etc.

I'm hoping people can check this patch for issues, bugs, etc and
hopefully this functionality can make it into future release of ant.
Comment 1 Eric Van 2004-08-19 18:03:36 UTC
Created attachment 12491 [details]
SQLExec.java patch
Comment 2 Jose Alberto Fernandez 2004-08-23 11:14:44 UTC
First, remember that <sql/> is not Oracle specific but generic and need to 
work with ALL JDBC drivers.

Second, I believe you can configure <sql/> to accept exactly what plsql would 
accept, except perhaps for the treatment of $. There are swithes to control 
whether formating should be kept, and what is the end of statement terminator 
and so on.

Take a look at ALL the options. If you do not want to set them all the time, 
take a look at <presetdef/>(sp?) on how to preset the option you use all the 
Comment 3 Eric Van 2004-08-23 14:23:18 UTC
well, as i stated in my initial comments, i DO realize that the <sql> task is
database independent and that although i've only tested my patch on postgres, i
DO intend to test it on oracle, sybase, and sqlserver since the product we are
developing will will be deployed on all four.

again, as i stated in my original comments, i DO realize that there are flags on
the <sql> task to change delimeters and what not.  but like i said, these
workarounds do require changes to the sql scripts to work and i find that
unacceptable given that many people use tools to automate the generation of the
sql scripts, and i for one do not want to reformat those scripts after each
generation just so i can run it in ant.  Especially when a simple solution such
as the one i provided, is so easily achieved.

If there is a setting where the following two statements can coexist in a sql
script (without reformatting the sql itself), then an example would be
appreciated.  But, while looking at the code for SQLExec.java, i do not see any
way for that to work (without the patch of course).

    tableId NUMERIC(16) NOT NULL,
    anotherColumn NUMERIC(4) NOT NULL,
    PRIMARY KEY (tableId)

    theTime timestamp;
    theTime := ''now'';
    UPDATE TheTable
      SET theTime = theTime
      WHERE tableId = NEW.tableId;
' LANGUAGE 'plpgsql';
Comment 4 Jose Alberto Fernandez 2004-08-23 14:59:42 UTC
Usually PL/SQL code uses the "//" at begining of line to separate
sql statements that must be processed in separate commits, but there
is no restriction on pl/sql to have more than one DDL statement sent at once
AFAIR (I may be wrong).

With this in mind, have you tried just giving your file with delimeter="//" 
delimetertype="row" (check attribute names) and see what the Oracle JDBC driver
will do?

On the other hand, as every generation tool may do their particular quirks on 
the generated code, I see no reason why you could not have a step in your 
building process where the generatedfiles are preprocessed to produce 
something more friendly to a generic <sql/> task. 

I do not see why the <sql/> task needs to be maintained (and BC verified) 
against the whims of every generation tool that there is around.

Alternatively you could subclass. Sql.java and have your own version 
specialized for your tools and maintained by you (as oppose to us).
This is my main problem, who is goin to maintain all this special behaviors.
Comment 5 Eric Van 2004-08-23 15:31:11 UTC
well, i'm not trying to make your life difficult, and i'm deffinetly not trying
to turn the SQLExec.java into some monster that handles "the whims of every
generation tool that there is around".  

I do however have issues with the fact that the task is not recognizing valid
sql.  The stored procedure example i gave is valid sql, and i can take that sql
(unmodified) to any other database independent tool and run it with no problems.
 But i take that very same sql and give it to the <sql> task and it fails.  To
me that is a flaw in the task, not the tool that generated the statement nor the
statement itself.

Now, i don't mind doing the leg work and testing this against the 4 databases i
mentioned, but won't be able to do so until later this year.  I just assumed
that the developers maintaining the task had unit tests for ensuring the
functionality and compatability of their code and that it wouldn't be too
difficult to run those tests with my patch or a similar patch created with the
same goal in mind.

If you still think this is a one off (even though valid sql statements fail),
then i will stop harrassing you and just spawn off my own task.  I am surprised
that others are not complaining about this, with the exception of a couple
mailing list posts i found. 

I REALLY DO appreciate you taking the time to consider this and giving me good
responses on you opinions why you don't see a need for this.
Comment 6 Jose Alberto Fernandez 2004-08-23 18:12:54 UTC
Are stored procedures part of a SQL9x standard? They are not in SQL 92 and as 
I remember Oracle PL/SQL procedures are not exactly what the newer SQL specs 
defined. I already think we have way too many stuff which is server specific 
hardcoded on this beast like "REM" (for MSSQL) for example. 

I look at your code and I am not sure whether it will play well with every 
other JDBC out there which may or may not support X,Y or Z. What is some allow 
for multiline strings? 

I wish we could define some new sql task with pluggability in mind so that 
each manufacturer could provide an ANT module (as part of the JDBC driver jar) 
that would do the syntactic checks required for that particular driver. 

So that the task just needs to identify the driver and delegate all this 
parsing issues. Then we could modularly and separately test all this stuff and
encourage manufactures to provide certified versions of the plug-in.

It would make this big monster into something manageable and neat.

Comment 7 Eric Van 2004-08-23 19:16:01 UTC
Well, i think the only goal of the <sql> task needs to be how to delimit
statements.  The parsing of the statement is ultimatly done by the driver and
the database.  The <sql> task just needs to be smart enough to determine where
one statement ends and a new one begins.  Unfortunatly the task just fails to do
so in my example.

With that in mind the task only needs to be responsible for knowing what the
standard is on delimiting statements.  As far as i know that standard is ';'.
Allowing the user to override that is obviously a necessity and no surprise that
you guys included it.

Now, my issues is, does that ';' (or other delimiter) count as a delimiter when
it's in an unterminated string?  I don't believe any database would consider a
statement containing an unterminated string valid, which is why all the other
database independent tools (which are java based just like ant) that i've tried
will not delimit a statement on a line ending ';' when it's enclosed in an
unterminated string.  They will continue to parse the remainder of the statement
until that string is closed and then a ';' is encountered.

It's my opinion that the <sql> task should do the same.

Also, just to clarify, i'm doing all my tesing on Postgresql, not Oracle.  If
this was just an Oracle specific problem, then i would be behind you 100%. 
Oracle has earned it's reputation as the Microsoft of databases, and i wouldn't
expect you to add 1 to 101 different cases to handle quirks they tend to throw
at people.  Although, they do own a substantial market share, and supporting
their caveats is a necessity these days even in open source software.

Comment 8 Jose Alberto Fernandez 2004-08-24 11:40:15 UTC
OK, took a look at what actually your example here says and I am even more 
horrified than before 8-O

Aparently in POSTGRESSQL an stored procedure is of the form:
  "CREATE" ... "AS" <multi-line-string> ";"

This is extremely POSTGRESS specific, it seems like. Certaintly it does not 
match SQL99 spec which is the one standarizing PSMs (i.e., stored procedures).
The truth is that as with any other programming language, there is no unique 
separator in SQL99, you need to understand the grammar of the language to know 
when things end due to the presence of nested statements.

What the current <sql/> does is a complete hack. I may be willing to accept 
that <sql/> should be able ignore the ending char when inside a SQL string,
which is the fundamental of what you provide. But we need to think not only 
on '....' SQL strings, but maybe also on "....." SQL table names, for example.

Notice that this does not mean that this will solve any of the issues you were 
pretending to solve (i.e., a CREATE statement in SQL99 can have multiple ";"s 
outside strings). But it may just happen to be enough for POSGRESS.
Comment 9 Eric Van 2004-08-24 20:14:10 UTC
Ok, that is a completely acceptable response.  It' been awhile since i've done
pl/sql on any other platform, and i didn't realize that i was accounting for a
postgres specific issue.

that still leaves the <sql> task incomplete in my opinion, but i now agree that
my patch is not a catch all solution, and a better approach needs to be formulated.

with that being the case, i think your pluggable approach seems like the real
solution to the problem.  I wouldn't necessarily rely on the dabase vendors to
provide their own implementations though.  I'd at least provide working tasks
for the major players, and the most commonly used databases (oracle, sybase,
sqlserver, postgres, mysql, hsql, etc).
Comment 10 Jose Alberto Fernandez 2004-08-25 09:31:40 UTC
Agree 100%. Any takers on proposing a design?
Comment 11 Eric Van 2004-09-10 18:31:31 UTC
Created attachment 12700 [details]
diff of original 1.6.2 code and new patch
Comment 12 Eric Van 2004-09-10 18:32:08 UTC
Ok, now i have moved beyond Postgresql and i'm now working with Postgresql,
Sybase, and SQL Server.

With Postgres i was able to remove my patch and use a workaround to get the
<sql> task to feed the entire .sql file as a single statement to the database
(by setting the delimiter to a string that never occurs in the sql script) and
letting Postgres determine where one statement began and a new one ended.

The same senario works.

The work around fails.  This is because SQL Server complains that any CREATE
VIEW statements should be sent one at a time.

SO... I revisted patching the <sql> task to support delimiting the statements
once again.  Now that i have 3 databases to use as examples, i can now make sure
that i'm not creating a one off for one database vendor (which my original
quoted expression patch was).

My new patch works similar to the first one i submitted, except now instead of
checking for quoted expression (postgres specific), i check for BLOCK
statements.  A block statement in this case is defined as one that starts with
BEGIN and ends with END.  It also checks for nesting of other BEGIN, END pairs
to prevent sending a statement fragment to the datbase.  The only requirement is
that BEGIN and END occur on a line of there own.

SO, now the following work for Postgresql, Sybase, AND Sql Server (Oracle to be
tested here next week).

POSTGRES: (only change to my original example is that the final END no longer
has a trailing ';'
      theTime timestamp;
      theTime := ''now'';
      UPDATE TheTable
        SET theTime = theTime
        WHERE tableId = NEW.tableId;
  ' LANGUAGE 'plpgsql';


    UPDATE TheTable
      SET theTime = (SELECT GETDATE())
      WHERE tableId = newRow.tableId;


  ON TestTable
    DECLARE @tableId NUMERIC(16);
    SELECT @tableId = (SELECT tableId FROM Inserted);
    Update TheTable set theTime = (SELECT GETDATE()) WHERE tableId = @tableId;
Comment 13 Jose Alberto Fernandez 2004-09-15 15:01:09 UTC
I would suggest taking a look at the syntax of SQL99 to see if you cover it.

Looking at the syntax of PL/SQL it is much more complicated as, for example, 
for a stored function there will be ";" as part of specifying the RESULT TYPE
but before the BEGIN ... END block.

As I said is not a simple problem.
Comment 14 Eric Van 2004-09-15 17:23:18 UTC
so far it has been a very simple problem to solve.  adding the code took a
wopping 10 minutes to do, and adding additional cases as i come across them,
takes only a few minutes.  if at some point the code for determining when a
delimiter occurs within a code block gets too big, then i'll break it out by
database and add an attribute to <sql> for the target database.

since no one else is taking any initiative to solve this, i will just maintain
it for my own project and stop pressing the issue here.

"work is the path of least resistance"