OpenJPA
  1. OpenJPA
  2. OPENJPA-975

Oracle needs ability to not have an escape character for search strings.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 1.3.0, 2.0.0-M2
    • Fix Version/s: 1.0.4, 1.2.2, 1.3.0, 2.0.0-M2
    • Component/s: jdbc
    • Labels:
      None
    • Environment:
      Oracle

      Description

      Oracle application has a JPQL query that attempts to set a parameter to '\'. '\' is not a reserved character with Oracle and the query will work if you execute it "normally" with JDBC.

      OpenJPA automatically adds ESCAPE ('\') to every search SQL query. As a result the \ is interpreted as an escape indicator and the SQL will not execute as expected.

      1. OPENJPA-975-trunk.patch
        12 kB
        B.J. Reed
      2. OPENJPA-975c_1.0.x.patch
        13 kB
        Michael Dick
      3. OPENJPA-975c_1.0.x.patch
        14 kB
        Jody Grassel
      4. OPENJPA-975c.patch
        13 kB
        B.J. Reed
      5. OPENJPA-975doc.patch
        2 kB
        B.J. Reed

        Issue Links

          Activity

          Hide
          Milosz Tylenda added a comment -

          Mike, I tried to build the manual in trunk and 1.3.x, now builds went fine. I also glanced the files affected in other branches and they looked good also.

          Show
          Milosz Tylenda added a comment - Mike, I tried to build the manual in trunk and 1.3.x, now builds went fine. I also glanced the files affected in other branches and they looked good also.
          Hide
          Michael Dick added a comment -

          Fixes committed. Both properties are under the DBDictionary section.

          Show
          Michael Dick added a comment - Fixes committed. Both properties are under the DBDictionary section.
          Hide
          Michael Dick added a comment -

          Same bug in the branches, sorry. Fix forthcoming.

          Show
          Michael Dick added a comment - Same bug in the branches, sorry. Fix forthcoming.
          Hide
          Milosz Tylenda added a comment -

          I am afraid we have the problem with the patch applied to trunk.

          1. Building the manual fails with message: 'The id "DBDictionary.SearchStringEscape" already exists in this document.' This is because of property description being doubled - it exists in generic database section and in Oracle. Should be in generic only.

          2. The DBDictionary.RequiresSearchStringEscapeForLike should be rather moved from Oracle section to generic one as this is a DBDictionary property.

          I did not check the health of branches.

          Show
          Milosz Tylenda added a comment - I am afraid we have the problem with the patch applied to trunk. 1. Building the manual fails with message: 'The id "DBDictionary.SearchStringEscape" already exists in this document.' This is because of property description being doubled - it exists in generic database section and in Oracle. Should be in generic only. 2. The DBDictionary.RequiresSearchStringEscapeForLike should be rather moved from Oracle section to generic one as this is a DBDictionary property. I did not check the health of branches.
          Hide
          Michael Dick added a comment -

          Thanks for the patch(es) B.J. and Jody. I've committed the changes.

          Show
          Michael Dick added a comment - Thanks for the patch(es) B.J. and Jody. I've committed the changes.
          Hide
          B.J. Reed added a comment -

          trunk patch contains document change and code change. Does not alter the escape character default behavior for Oracle.

          Show
          B.J. Reed added a comment - trunk patch contains document change and code change. Does not alter the escape character default behavior for Oracle.
          Hide
          Milosz Tylenda added a comment -

          Another minor editorial remark is we tend to omit braces in one-line "if" statements.

          Show
          Milosz Tylenda added a comment - Another minor editorial remark is we tend to omit braces in one-line "if" statements.
          Hide
          Michael Dick added a comment -

          Before committing the patch I'd like to make sure that the current behavior is the default. Ie on Oracle (or any database) we should still include ESCAPE ('/') unless the user configures otherwise.

          The line in OracleDictionary that sets requiresSearchStringEscapeForLike should be removed, and the testcase will need updating to reflect that (set the property in order to test it at least when the DBDictionary is Oracle. The patch also includes an updated setClobString() method for Oracle that doesn't seem to be related to the problem. Is that intentional or did it creep in as part of another change? One other minor nit, there was a spot where the patch went beyond 80 characters, no big deal but it is something to keep an eye on.

          Otherwise the patch looks good and I'd be happy to include it in 1.0.4 when the issues listed above are resolved.

          Show
          Michael Dick added a comment - Before committing the patch I'd like to make sure that the current behavior is the default. Ie on Oracle (or any database) we should still include ESCAPE ('/') unless the user configures otherwise. The line in OracleDictionary that sets requiresSearchStringEscapeForLike should be removed, and the testcase will need updating to reflect that (set the property in order to test it at least when the DBDictionary is Oracle. The patch also includes an updated setClobString() method for Oracle that doesn't seem to be related to the problem. Is that intentional or did it creep in as part of another change? One other minor nit, there was a spot where the patch went beyond 80 characters, no big deal but it is something to keep an eye on. Otherwise the patch looks good and I'd be happy to include it in 1.0.4 when the issues listed above are resolved.
          Hide
          Michael Dick added a comment -

          Updating Joe's patch. Removing hard coded directory names( C:/$

          {something}

          ).

          Show
          Michael Dick added a comment - Updating Joe's patch. Removing hard coded directory names( C:/$ {something} ).
          Hide
          Jody Grassel added a comment -

          OpenJPA 1.0.x version of the patch.

          Show
          Jody Grassel added a comment - OpenJPA 1.0.x version of the patch.
          Hide
          B.J. Reed added a comment -

          It appears that most DB's do want the escape character by default, Oracle is an exception, but probably not the only one. Latest patch cleans up the test case code and adds another test to use a different escape string.

          Show
          B.J. Reed added a comment - It appears that most DB's do want the escape character by default, Oracle is an exception, but probably not the only one. Latest patch cleans up the test case code and adds another test to use a different escape string.
          Hide
          Michael Dick added a comment -

          Is this change specific to Oracle, or do other databases also allow the \ character to be used?

          I was thinking of this as more of a compatibility option. Ie in openjpa < 1.3.0 we automatically append the ESCAPE clause (unless configured otherwise). In OpenJPA >= 1.3.0 we do not (since the clause seems extraneous to me).

          I think this would be more tolerable for JDO providers who build on top of OpenJPA's kernel. They could set the compatibility option in their ProductDerivation (similar to what was proposed for detach options) and then all databases would use the old behavior. If the setting is in the DBDictionary then reverting to the old behavior requires more code.

          To answer the broader question about whether we care for JDO in OpenJPA version x. My opinion is that we care, but our obligation is to provide JPA expected behavior by default. When this conflicts with JDO's expected behavior we should provide a configuration option to allow JDO like behavior, but we have no obligation to provide it "out of the box".

          Show
          Michael Dick added a comment - Is this change specific to Oracle, or do other databases also allow the \ character to be used? I was thinking of this as more of a compatibility option. Ie in openjpa < 1.3.0 we automatically append the ESCAPE clause (unless configured otherwise). In OpenJPA >= 1.3.0 we do not (since the clause seems extraneous to me). I think this would be more tolerable for JDO providers who build on top of OpenJPA's kernel. They could set the compatibility option in their ProductDerivation (similar to what was proposed for detach options) and then all databases would use the old behavior. If the setting is in the DBDictionary then reverting to the old behavior requires more code. To answer the broader question about whether we care for JDO in OpenJPA version x. My opinion is that we care, but our obligation is to provide JPA expected behavior by default. When this conflicts with JDO's expected behavior we should provide a configuration option to allow JDO like behavior, but we have no obligation to provide it "out of the box".
          Hide
          Milosz Tylenda added a comment -

          This quote from the manual

          "SearchStringEscape: The default escape character used when generating SQL LIKE clauses. The escape character is used to escape the wildcard meaning of the _ and % characters. Note: since JPQL provides the ability to define the escape character in the query, this setting is primarily used when translating other query languages, such as JDOQL. Defaults to "
          " (a single backslash in Java speak)."

          worries me as it suggests the resolution of this issue conflicts with JDOQL support.

          Could some more experienced developer comment on this?
          Is this "always append ESCAPE to LIKE" behaviour a JDOQL requirement?
          Do we care for JDO in OpenJPA 1.3 and 2.0?

          Show
          Milosz Tylenda added a comment - This quote from the manual "SearchStringEscape: The default escape character used when generating SQL LIKE clauses. The escape character is used to escape the wildcard meaning of the _ and % characters. Note: since JPQL provides the ability to define the escape character in the query, this setting is primarily used when translating other query languages, such as JDOQL. Defaults to " " (a single backslash in Java speak)." worries me as it suggests the resolution of this issue conflicts with JDOQL support. Could some more experienced developer comment on this? Is this "always append ESCAPE to LIKE" behaviour a JDOQL requirement? Do we care for JDO in OpenJPA 1.3 and 2.0?
          Hide
          B.J. Reed added a comment -

          OPENJPA-975doc.patch is the update to the documentation for this new DB Dictionary property.

          Show
          B.J. Reed added a comment - OPENJPA-975 doc.patch is the update to the documentation for this new DB Dictionary property.
          Hide
          B.J. Reed added a comment -

          Thanks Milosz for the review.

          I had noticed all the "required" properties a few days ago and there was some subtle difference on the name that I landed on.....but that logic totally escapes me now, so I've renamed to requiresSearchStringEscapeForLike - also want to make it as obvious as possible that it works with the "SearchStringEscape" property.

          I have it on my list of TODO's to update the documentation.

          Have opened up http://issues.apache.org/jira/browse/OPENJPA-976 to do more investigation about which DB's prefer the search string escape. At this time my knowledge of all the DB's is rather limited, but I'll be happy to do that research.

          Show
          B.J. Reed added a comment - Thanks Milosz for the review. I had noticed all the "required" properties a few days ago and there was some subtle difference on the name that I landed on.....but that logic totally escapes me now, so I've renamed to requiresSearchStringEscapeForLike - also want to make it as obvious as possible that it works with the "SearchStringEscape" property. I have it on my list of TODO's to update the documentation. Have opened up http://issues.apache.org/jira/browse/OPENJPA-976 to do more investigation about which DB's prefer the search string escape. At this time my knowledge of all the DB's is rather limited, but I'll be happy to do that research.
          Hide
          Milosz Tylenda added a comment -

          Another small request. Is it possible that you rename the property to requiresEscapeForLike? We already have a few requires*For* properties and it would fit nicely into that name pattern.

          Show
          Milosz Tylenda added a comment - Another small request. Is it possible that you rename the property to requiresEscapeForLike? We already have a few requires*For* properties and it would fit nicely into that name pattern.
          Hide
          Milosz Tylenda added a comment -

          B.J., thanks for the patch.

          1. Wouldn't you mind updating also the documentation? We have a section in the manual which covers public dictionary properties.

          2. It could be beneficial to research other databases in that matter. I have a feeling that many of them do not require the ESCAPE clause. A separate issue can be opened for that.

          Show
          Milosz Tylenda added a comment - B.J., thanks for the patch. 1. Wouldn't you mind updating also the documentation? We have a section in the manual which covers public dictionary properties. 2. It could be beneficial to research other databases in that matter. I have a feeling that many of them do not require the ESCAPE clause. A separate issue can be opened for that.
          Hide
          B.J. Reed added a comment -

          Attached patch has a new field in the DBDictionary: alwaysAddSearchStringEscape = true

          The OracleDictionary sets this to false.

          If this field is true, then JDBCExpressionFactory acts as it always did and uses the searchStringEscape that is defined for the Dictionary. If it is false, then the additional ESCAPE ('\') will not be added.

          Test case also included.

          Show
          B.J. Reed added a comment - Attached patch has a new field in the DBDictionary: alwaysAddSearchStringEscape = true The OracleDictionary sets this to false. If this field is true, then JDBCExpressionFactory acts as it always did and uses the searchStringEscape that is defined for the Dictionary. If it is false, then the additional ESCAPE ('\') will not be added. Test case also included.

            People

            • Assignee:
              B.J. Reed
              Reporter:
              B.J. Reed
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development