Derby
  1. Derby
  2. DERBY-3610

Confusing error message when granting execute privilege

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: SQL
    • Labels:
      None

      Description

      When the wrong keyword is used in a grant execute privilege statement, the error message
      leads one the believe the identifier is neither a procedure or a function, when in deed it is one of the two.

      > create function bc(i int) returns int language java parameter style java external name 'java.lang.Integer.bitCount' no sql;
      > grant execute on procedure bc to foo;
      ERROR 42Y03: 'BC' is not recognized as a function or procedure.

      > grant execute on function bc to foo;
      (works)

      The reason is that the error message is generic. It would be better to have an error message for each case.

      1. DERBY-3610-update2.patch
        5 kB
        Hiranya Jayathilaka
      2. GrantRevokeTest.log
        9 kB
        Knut Anders Hatlen
      3. DERBY-3610-update1.patch
        5 kB
        Hiranya Jayathilaka
      4. DERBY-3610.patch
        5 kB
        Hiranya Jayathilaka

        Issue Links

          Activity

          Hide
          Hiranya Jayathilaka added a comment -

          I ran some debug sessions and found the cause of the issue. The problem is we only have one error message to handle both scenarios (42Y03). We can easily fix this by adding two new messages, one for each scenario and making some code changes in the PrivilegeNode class. I've already done these changes in my local copy and it seems to be working fine. Before I submit a patch I want to know the rationale behind the coding scheme used to classify error messages. If I'm to add two new messages what are the codes I should use to identify them (for the moment I'm using 62Y03 and 72Y03).

          Can somebody please shed some light on the subject?

          Show
          Hiranya Jayathilaka added a comment - I ran some debug sessions and found the cause of the issue. The problem is we only have one error message to handle both scenarios (42Y03). We can easily fix this by adding two new messages, one for each scenario and making some code changes in the PrivilegeNode class. I've already done these changes in my local copy and it seems to be working fine. Before I submit a patch I want to know the rationale behind the coding scheme used to classify error messages. If I'm to add two new messages what are the codes I should use to identify them (for the moment I'm using 62Y03 and 72Y03). Can somebody please shed some light on the subject?
          Hide
          Hiranya Jayathilaka added a comment -

          In the proposed solution we don't remove the message 42Y03. This message is being referred to by many classes and it's probably not a good idea to remove it. That will cause many small code changes all over the code base. So I suggest simply adding two new messages and making some changes local to the PrivilegeNode class to tackle the reported issue. I think we also have to update the corresponding test cases in the GrantRevokeTest class and GrantRevokeDDLTest class. Thoughts?

          Show
          Hiranya Jayathilaka added a comment - In the proposed solution we don't remove the message 42Y03. This message is being referred to by many classes and it's probably not a good idea to remove it. That will cause many small code changes all over the code base. So I suggest simply adding two new messages and making some changes local to the PrivilegeNode class to tackle the reported issue. I think we also have to update the corresponding test cases in the GrantRevokeTest class and GrantRevokeDDLTest class. Thoughts?
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this issue, Hiranya.

          You could make one new error message which can take an argument, "procedure" or "function", and use that
          in the PrivilegeNode as the case may be. Note that arguments to error messages need to be localized (i18n), but in this case the variability would be SQL keywords, so it's ok to use them as is. As for code assignment,
          it is a bit messy already, but we should try to look at:

          a) SQL standard codes if prescribed in the standard, cf. Foundation(part 2), section 24.1
          (drafts can be found here: http://www.wiscorp.com/SQLStandards.html)
          b) Derby established practice, cf. the comment headers in reference/SQLState.java

          Is probably wise to avoid codes that have been used in the past (i.e. are commented out).
          In this case I think it would be good to put it close to the existing one, e.g. 42Y21 is unused.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this issue, Hiranya. You could make one new error message which can take an argument, "procedure" or "function", and use that in the PrivilegeNode as the case may be. Note that arguments to error messages need to be localized (i18n), but in this case the variability would be SQL keywords, so it's ok to use them as is. As for code assignment, it is a bit messy already, but we should try to look at: a) SQL standard codes if prescribed in the standard, cf. Foundation(part 2), section 24.1 (drafts can be found here: http://www.wiscorp.com/SQLStandards.html ) b) Derby established practice, cf. the comment headers in reference/SQLState.java Is probably wise to avoid codes that have been used in the past (i.e. are commented out). In this case I think it would be good to put it close to the existing one, e.g. 42Y21 is unused.
          Hide
          Dag H. Wanvik added a comment -

          Yes, you would also have to update the test that would be affected by this change. GrantRevokeTest and GrantRevokeDDLTest are likely candidates, yes, and maybe the role tests.

          Show
          Dag H. Wanvik added a comment - Yes, you would also have to update the test that would be affected by this change. GrantRevokeTest and GrantRevokeDDLTest are likely candidates, yes, and maybe the role tests.
          Hide
          Hiranya Jayathilaka added a comment -

          Thanks for the tips Dag. I'm on this issue. Expect a patch soon.

          Show
          Hiranya Jayathilaka added a comment - Thanks for the tips Dag. I'm on this issue. Expect a patch soon.
          Hide
          Hiranya Jayathilaka added a comment -

          I'm uploading a patch for a review. This patch solves the reported problem. I also verified the derbyall testsuite with this patch applied and didn't notice any issues.

          Your feedback is most appreciated. I'm not 100% sure about the way I'm passing the SQL keywords (procedure/function) to the error message. Is there a better way of doing that?

          Show
          Hiranya Jayathilaka added a comment - I'm uploading a patch for a review. This patch solves the reported problem. I also verified the derbyall testsuite with this patch applied and didn't notice any issues. Your feedback is most appreciated. I'm not 100% sure about the way I'm passing the SQL keywords (procedure/function) to the error message. Is there a better way of doing that?
          Hide
          Knut Anders Hatlen added a comment -

          I think it may be better to have two different error messages, since passing the words "procedure" and "function" as arguments to the error message doesn't work that well for other languages than English.

          It is possible to have many messages with the same SQLState, by adding a suffix that is only used by Derby internally. For example

          42Y21.S.0 -

          {0} is not recognized as a procedure
          42Y21.S.1 - {0}

          is not recognized as a function

          See here for a description of how Derby interprets the states: http://db.apache.org/derby/javadoc/engine/org/apache/derby/shared/common/reference/SQLState.html

          Perhaps we should reuse the SQLState 42Y03 instead of adding 42Y21, so that the change doesn't affect existing applications that check the SQLState. If the state is changed, we'd need a release note to give a heads up to the users.

          Show
          Knut Anders Hatlen added a comment - I think it may be better to have two different error messages, since passing the words "procedure" and "function" as arguments to the error message doesn't work that well for other languages than English. It is possible to have many messages with the same SQLState, by adding a suffix that is only used by Derby internally. For example 42Y21.S.0 - {0} is not recognized as a procedure 42Y21.S.1 - {0} is not recognized as a function See here for a description of how Derby interprets the states: http://db.apache.org/derby/javadoc/engine/org/apache/derby/shared/common/reference/SQLState.html Perhaps we should reuse the SQLState 42Y03 instead of adding 42Y21, so that the change doesn't affect existing applications that check the SQLState. If the state is changed, we'd need a release note to give a heads up to the users.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Hiranya!
          Knut, I don't think "procedure" and "function" is a problem, both are SQL keywords, we had similar cases in the past. But I like the idea of avoiding having to introduce a new error message so doing as you suggest is still good: +1.

          Patch looks good, but some nits: One of the lines was longer than 80 characters and had trailing whitespace. We try to avoid both.

          As a matter of style (feel free to disagree); I prefer this

          String keyword;

          if (rd.isFunction)

          { keyword = "function"; }

          else

          { keyword = "procedure"; }

          over:

          String keyword = "procedure";
          if (rd.isFunction)

          { keyword = "function"; }

          since in the function case there is one redundant assignment. I also find it easier to read.

          Some would perhaps prefer:

          String keyword = rd.isFunction ? "function" : "procedure";

          but I find this harder to read. But in any case, using Knut's idea, this code would go away anyway...

          Show
          Dag H. Wanvik added a comment - Thanks, Hiranya! Knut, I don't think "procedure" and "function" is a problem, both are SQL keywords, we had similar cases in the past. But I like the idea of avoiding having to introduce a new error message so doing as you suggest is still good: +1. Patch looks good, but some nits: One of the lines was longer than 80 characters and had trailing whitespace. We try to avoid both. As a matter of style (feel free to disagree); I prefer this String keyword; if (rd.isFunction) { keyword = "function"; } else { keyword = "procedure"; } over: String keyword = "procedure"; if (rd.isFunction) { keyword = "function"; } since in the function case there is one redundant assignment. I also find it easier to read. Some would perhaps prefer: String keyword = rd.isFunction ? "function" : "procedure"; but I find this harder to read. But in any case, using Knut's idea, this code would go away anyway...
          Hide
          Knut Anders Hatlen added a comment -

          > Knut, I don't think "procedure" and "function" is a problem, both
          > are SQL keywords, we had similar cases in the past. But I like the
          > idea of avoiding having to introduce a new error message so doing as
          > you suggest is still good: +1.

          Hm... I thought I proposed the opposite of what you said, that is
          adding two new error messages, not avoiding the introduction of new
          ones... But I'm glad you support the idea!

          As to "procedure" and "function" being keywords, that's true, but we
          normally use upper case letters when they are used as such. Looking at
          the translations of the 42Y03 message, it seems like the translators
          did not regard those words as SQL keywords in that message, but rather
          as names of concepts that needed to be translated.

          Show
          Knut Anders Hatlen added a comment - > Knut, I don't think "procedure" and "function" is a problem, both > are SQL keywords, we had similar cases in the past. But I like the > idea of avoiding having to introduce a new error message so doing as > you suggest is still good: +1. Hm... I thought I proposed the opposite of what you said, that is adding two new error messages, not avoiding the introduction of new ones... But I'm glad you support the idea! As to "procedure" and "function" being keywords, that's true, but we normally use upper case letters when they are used as such. Looking at the translations of the 42Y03 message, it seems like the translators did not regard those words as SQL keywords in that message, but rather as names of concepts that needed to be translated.
          Hide
          Dag H. Wanvik added a comment -

          To be clear, I think it is good to reuse the SQL state (42Y03), but
          introduce two new messages as you suggest (i.e. no new state).

          As for keywords in messages, all caps is probably advisable. Still, in this
          case, "function" and "procedure" are actually used as concepts more
          than referring to the actual keyword (although that distinction is
          often blurry it is fairly clear here), so providing a version of the
          error messages that would allow these terms to be translated is good for
          localization.

          So, +1 to two new messages reusing the old SQL state.

          Show
          Dag H. Wanvik added a comment - To be clear, I think it is good to reuse the SQL state (42Y03), but introduce two new messages as you suggest (i.e. no new state). As for keywords in messages, all caps is probably advisable. Still, in this case, "function" and "procedure" are actually used as concepts more than referring to the actual keyword (although that distinction is often blurry it is fairly clear here), so providing a version of the error messages that would allow these terms to be translated is good for localization. So, +1 to two new messages reusing the old SQL state.
          Hide
          Dag H. Wanvik added a comment -

          Hiranya, you may want to assign yourself to this issue.

          Show
          Dag H. Wanvik added a comment - Hiranya, you may want to assign yourself to this issue.
          Hide
          Hiranya Jayathilaka added a comment -

          Thanks for all the comments folks. I just need a small clarification. Do you suggest something like this?

          42Y03.S.0 -

          {0} is not recognized as a procedure
          42Y03.S.1 - {0}

          is not recognized as a function

          If there are other classes which uses the state 42Y03, won't they get affected by this addition?

          Show
          Hiranya Jayathilaka added a comment - Thanks for all the comments folks. I just need a small clarification. Do you suggest something like this? 42Y03.S.0 - {0} is not recognized as a procedure 42Y03.S.1 - {0} is not recognized as a function If there are other classes which uses the state 42Y03, won't they get affected by this addition?
          Hide
          Knut Anders Hatlen added a comment -

          Adding 42Y03.S.0 and 42Y03.S.1 won't affect those who already use 42Y03, as the three message ids are different. For consistency though, it may make sense to rename the original 42Y03 message to 42Y03.S.X so that we have

          42Y03.S.0 -

          {0} is not recognized as a function or a procedure
          42Y03.S.1 - {0}

          is not recognized as a procedure
          42Y03.S.2 -

          {0}

          is not recognized as a function

          The constant SQLState.LANG_NO_SUCH_METHOD_ALIAS and the <name> tag for that message in messages.xml will have to be updated.

          Show
          Knut Anders Hatlen added a comment - Adding 42Y03.S.0 and 42Y03.S.1 won't affect those who already use 42Y03, as the three message ids are different. For consistency though, it may make sense to rename the original 42Y03 message to 42Y03.S.X so that we have 42Y03.S.0 - {0} is not recognized as a function or a procedure 42Y03.S.1 - {0} is not recognized as a procedure 42Y03.S.2 - {0} is not recognized as a function The constant SQLState.LANG_NO_SUCH_METHOD_ALIAS and the <name> tag for that message in messages.xml will have to be updated.
          Hide
          Dag H. Wanvik added a comment -

          Note that the tests can't just test for 42Y03 when we overload the SQL state. To verify that the correct error message is used, you will have to assert on the message text.

          Show
          Dag H. Wanvik added a comment - Note that the tests can't just test for 42Y03 when we overload the SQL state. To verify that the correct error message is used, you will have to assert on the message text.
          Hide
          Hiranya Jayathilaka added a comment -

          Attaching the updated patch. Please review and provide feedback. Thanks Knut and Dag for all the support so far.

          Show
          Hiranya Jayathilaka added a comment - Attaching the updated patch. Please review and provide feedback. Thanks Knut and Dag for all the support so far.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks! The patch looks good to me. I'm running the regression tests now and will commit if they pass.

          Show
          Knut Anders Hatlen added a comment - Thanks! The patch looks good to me. I'm running the regression tests now and will commit if they pass.
          Hide
          Knut Anders Hatlen added a comment -

          I'm seeing four failures in GrantRevokeTest when running with the patch. See the attached log for details.

          Show
          Knut Anders Hatlen added a comment - I'm seeing four failures in GrantRevokeTest when running with the patch. See the attached log for details.
          Hide
          Hiranya Jayathilaka added a comment -

          Looks like the error messages generated contains schema information causing the assertions on message text to fail. I'll fix this up asap and provide a patch. Thanks for pointing this out.

          Show
          Hiranya Jayathilaka added a comment - Looks like the error messages generated contains schema information causing the assertions on message text to fail. I'll fix this up asap and provide a patch. Thanks for pointing this out.
          Hide
          Hiranya Jayathilaka added a comment -

          Updated the GrantRevokeTest. Verified the overall functionality against derbylang test suite.

          Show
          Hiranya Jayathilaka added a comment - Updated the GrantRevokeTest. Verified the overall functionality against derbylang test suite.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Hiranya.
          Committed revision 765087.

          Show
          Knut Anders Hatlen added a comment - Thanks, Hiranya. Committed revision 765087.
          Hide
          Dag H. Wanvik added a comment -

          Verified that the error messages now look clear for both function and procedure, closing.
          Thanks, Hiranya and Knut.

          Show
          Dag H. Wanvik added a comment - Verified that the error messages now look clear for both function and procedure, closing. Thanks, Hiranya and Knut.

            People

            • Assignee:
              Hiranya Jayathilaka
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development