|
Changing fixin to be 10.1.2.0 and 10.2.0.0 since I have a fix ready and will be posting it once I have written corresponding tests. The fix should be ready for the 10.1.2 bug fix release, and of course it should go into the trunk, as well, hence my choice of fixin values.
Attaching a patch for this issue. The patch does the following:
1) Updates client to look for statements that begin with comments and to ignore the comments when deciding the kind of statement that is being executed. 2) Adds test cases for statements that begin with comments to the jdbcapi/nullSQLText.java test 3) Creates two new masters for nullSQLText: one for DerbyNet, one for DerbyNetClient, to reflect the additional test cases. 4) Updates lang/syscat.sql master for DerbyNetClient to match the new behavior. I couldn't find an existing test that really matched the condition, so I chose "nullSQLText" since it involves testing edge cases for statements that are passed to Derby via the JDBC "execute" call, which is similar to what the new tests do. If, however, anyone thinks I should create a new test for this particular issue (for the sake of clarity), I can certainly go ahead and do so. I ran derbyall with Sun JDK 1.4 on Windows 2000 and saw no failures. Many thanks to Kathey for the helpful comment regarding where the problem was--she was absolutely right. I know there is code in ij that strips of the comments for client. It would be great if it could be changed to not strip off the comments for a 10.2 client. Then you will see that there are actually lots of test cases checked in in many of the sql files. I think ij should remain the same and strip off the comments for 10.1 clients even when this is ported to 10.1, so as not to cause trouble with old clients.
I wonder about comments before Stored procedures that return result sets. Do those work ok? Attaching an second version of the patch that has the following changes (w.r.t the first version):
1) Logic in client/am/Statement.java has been re-written to account for some edge cases that weren't covered in the initial patch. 2) Per Kathey Marsden's suggestion, the ij code has been changed so that it no longer strips leading comments when passing statements to the Derby Client. NOTE: to be safe, this particular part of the patch probably should _not_ be ported to 10.1. 3) In working with this patch, I noticed that statements which _end_ in comments do not work when passed through JDBC--rather, the result will be a syntax error in all modes (embedded, Derby Client, and JCC); see below for more on that issue. The new version of the patch fixes that problem in addition to the other documented problems in this Jira issue. 4) Updated additional master files to reflect the new changes. ------ NOTES: ------ Regarding #3 above: it turns out that if one tries to execute a SQL statement that _ends_ with a comment via JDBC, the result will be a syntax error. This occurs in embedded mode as well as client/server mode. Ex: ResultSet rs = st.execute("values 2, 3, 4\n-- trailing comment"); will result in the following error: ERROR 42X01: Syntax error: Encountered "-" at line 2, column 1. Note, though, that if you add an end-of-line character to the exact same statement, it will work: ResultSet rs = st.execute("values 2, 3, 4\n-- trailing comment\n"); The reason is because the grammar in sqlgrammar.jj defines a comment as always ending with either "\n", "\r", or "\r\n". Thus, in the first example above, since the comment doesn't end with any of these three characters, the result is a syntax error. To fix this, I just made the final "\n", "\r", or "\r\n" optional--then in situations where the comment is the last part of the statement and there's no trailing newline, it will still be recognized as a comment and processed accordingly (i.e. ignored). I ran derbyall after making this simple change and there were no failures, so I _think_ it's safe. However, I'm hoping to get a second opinion on this, just to be safe. Note that this change WILL have one side effect: in the edge case where a user tries to execute a statement that consists of ALL comments, the error message will now be: ERROR 42X01: Syntax error: Encountered "<EOF>" at line 1, column 3. instead of: ERROR 42X01: Syntax error: Encountered "-" at line 1, column 3. To me, this seems like an acceptable change in behavior since the new behavior matches what currently happens if the user passes in an empty string--and, if the entire statement is just comments, that is in effect the what we're doing. However, if anyone thinks this is unacceptable, please let me know and I'll just file a separate JIRA entry for that issue, to be addressed later. Finally, regarding Kathey's question of whether or not this fix will work if a comment precedes a stored procedure call that returns result sets, the answer is "Yes." I ran something like: ResultSet rs = st.executeQuery("-- leading comment\ncall proc1()"); and was able to process the results successfully. Did you have a particular concern with this Kathey? I added this test case to the set of cases for I ran derbyall on Windows 2000 with Sun JDK 1.4.2 with this patch and saw no failures. Many thanks to anyone who has additional comments/feedback on this patch. I reviewed this patch and my only comment is that I think that the ij change should strip comments for JCC or for Derby client drivers whose version is less than 10.2. That way the behaviour is dependent upon which client the ij tool is working with and a 10.2 ij will continue to work with 10.1 client.
Thanks for the review, Kathey. I misunderstood what you meant when you initially talked about not trimming for the 10.1 or earlier clients, hence my last patch didn't deal with that (sorry). I'll work on another version of the patch and will post it soon. Thanks again!
Updated patch so that ij will continue to remove comments from a statement against the Derby Client driver _if_ the client version is earlier than 10.2. This makes it so that a 10.2 ij will still work a 10.1 Derby Client. Thanks for catching this, Kathey.
Committed this to the trunk: Please submit the merge command for merge to 10.1 when ready.
Date: Mon Oct 17 15:41:03 2005 New Revision: 325993 URL: http://svn.apache.org/viewcvs?rev=325993&view=rev Log: DERBY-522 1) Updates client to look for statements that begin with comments and to ignore the comments when deciding the kind of statement that is being executed. 2) Changes ij to only strip comments for client versions less than 10.2 3) Adds test cases for statements that begin with comments to the jdbcapi/nullSQLText.java test 4) Updates masters Command to merge to 10.1 is:
svn merge -r 325992:325993 https://svn.apache.org/repos/asf/db/derby/code/trunk Attaching a master update for the _10.1_ codeline that fixes a diff for lang/forUpdate.sql.
The new logic in ij only strips comments if the client is 10.1 or earlier, and since the original patch was against 10.2, the master file that was submitted reflects the fact that the comments were NOT stripped. For the 10.1 branch, the comments are still stripped, hence the diff. The attached "d10_1Master.patch" file fixes that diff for 10.1. Could a committer please commit? Patch for this has been committed:
svn 325993 -- 10.2 svn 326222, 326308 -- 10.1 I've verified the repro and the new test cases in both the 10.2 and 10.1 branches and all looks to be correct. So I'm marking this as resolved. Kathey, I'll leave it to you to close this issue if everything looks okay. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
ERROR X0Y79 raised when adding comments using "--" before sql queries with Network Client
EXCSQLIMM cannot be used for a statement that returns result sets.
Client needs to treat the statement as a select and handle it properly.
Note:
Currently IJ currently has code to strip out the comments for Network Client so the issue will not reproduce with IJ.