OpenJPA
  1. OpenJPA
  2. OPENJPA-1992

java.lang.ArrayIndexOutOfBoundsException if positional parameter are not started from 1

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.0
    • Fix Version/s: 2.2.0
    • Component/s: kernel
    • Labels:
      None

      Description

      Query q = em.createQuery("SELECT x FROM Magazine x WHERE x.title = ?2 and x.price > ?3");
      q.setParameter(2, "JDJ").setParameter(3, 25.0);

      When the query is executed, java.lang.ArrayIndexOutOfBoundsException will be thrown. Since "JDJ" and "25.0" will be stored in arr[0] and arr[1] (when arr.size =2) but the query execution code tries to get them via arr[2] and arr[3].

      1. Main.java
        3 kB
        Han Hong Fang
      2. OPENJPA-1992.patch
        2 kB
        Han Hong Fang

        Issue Links

          Activity

          Han Hong Fang created issue -
          Hide
          Han Hong Fang added a comment -

          Patch is attached, please help to review. Thanks in advance!

          Show
          Han Hong Fang added a comment - Patch is attached, please help to review. Thanks in advance!
          Han Hong Fang made changes -
          Field Original Value New Value
          Attachment OPENJPA-1992.patch [ 12478144 ]
          Hide
          Han Hong Fang added a comment -

          Just for information. the problem doesn't exist in openjpa 1.2.2.

          Show
          Han Hong Fang added a comment - Just for information. the problem doesn't exist in openjpa 1.2.2.
          Hide
          Michael Dick added a comment -

          Thanks for the patch. Could you also provide a testcase the reproduces the error?

          The JPA spec does indicate that input parameters will start with 1 (section 4.6.4.1), but we should at least fail in a more informative way.

          Show
          Michael Dick added a comment - Thanks for the patch. Could you also provide a testcase the reproduces the error? The JPA spec does indicate that input parameters will start with 1 (section 4.6.4.1), but we should at least fail in a more informative way.
          Michael Dick made changes -
          Assignee Michael Dick [ mikedd ]
          Hide
          Han Hong Fang added a comment -

          Hi Michael,

          This exception can be reproduced with the hellojpa sample by following Getting Started with the Eclipse steps on http://openjpa.apache.org/getting-started.html. After step 10, replace the Main.java with the attached copy of Main.java, and then rerun step 9.

          I noticed that spec indicates that input parameters will start with 1 in both JPA spec(section 4.6.4.1) and EJB spec(section 9.2.6.4), but this exception blocks our TCK, which expects "SELECT x FROM Magazine x WHERE x.title = ?2 and x.price > ?3" can be successfully processed.

          Show
          Han Hong Fang added a comment - Hi Michael, This exception can be reproduced with the hellojpa sample by following Getting Started with the Eclipse steps on http://openjpa.apache.org/getting-started.html . After step 10, replace the Main.java with the attached copy of Main.java, and then rerun step 9. I noticed that spec indicates that input parameters will start with 1 in both JPA spec(section 4.6.4.1) and EJB spec(section 9.2.6.4), but this exception blocks our TCK, which expects "SELECT x FROM Magazine x WHERE x.title = ?2 and x.price > ?3" can be successfully processed.
          Hide
          Han Hong Fang added a comment -

          Attached modified Main.java for problem re-creation.

          Show
          Han Hong Fang added a comment - Attached modified Main.java for problem re-creation.
          Han Hong Fang made changes -
          Attachment Main.java [ 12478374 ]
          Hide
          Michael Dick added a comment -

          Hi Han,

          Which TCK are you talking about? OpenJPA 2.1.0 passed the JPA 1.0 and 2.0 TCKs prior to release. I'm assuming you're talking about a different test suite?

          Show
          Michael Dick added a comment - Hi Han, Which TCK are you talking about? OpenJPA 2.1.0 passed the JPA 1.0 and 2.0 TCKs prior to release. I'm assuming you're talking about a different test suite?
          Hide
          Pinaki Poddar added a comment -

          The binding parameter indexing, ordering and naming are somewhat sensitive to many implicit assumptions throughout our codebase. It is made more sensitive after we introduced Prepared Query Caches that rebinds parameters (that originally could have been named) to SQL (that only accepts integral parameter). The index of parameters affects the kernel and also note that kernel internals can insert its own parameters that are invisible to the user application.

          Please be careful with a change unless it is broken for regular, spec-compliant use case. If for example we allow numeric parameters not to begin with 1 and be monotonic and continuous, nothing stops a user to specific "SELECT a FROM A a where a.name=?100234 and A.something=?34562" – it will get real hairy to support such an innovative user

          An informative error message when integral parameters violate any of the following
          a) start with 1
          b) monotonic
          c) continuous
          is all that is required to keep life simple, code uncomplicated and majority of user satisfied.

          Show
          Pinaki Poddar added a comment - The binding parameter indexing, ordering and naming are somewhat sensitive to many implicit assumptions throughout our codebase. It is made more sensitive after we introduced Prepared Query Caches that rebinds parameters (that originally could have been named) to SQL (that only accepts integral parameter). The index of parameters affects the kernel and also note that kernel internals can insert its own parameters that are invisible to the user application. Please be careful with a change unless it is broken for regular, spec-compliant use case. If for example we allow numeric parameters not to begin with 1 and be monotonic and continuous, nothing stops a user to specific "SELECT a FROM A a where a.name=?100234 and A.something=?34562" – it will get real hairy to support such an innovative user An informative error message when integral parameters violate any of the following a) start with 1 b) monotonic c) continuous is all that is required to keep life simple, code uncomplicated and majority of user satisfied.
          Hide
          Michael Dick added a comment -

          I agree. This is about detection and producing an informative error message. At this time I don't think we can or should support numeric parameters that start at any point.

          Show
          Michael Dick added a comment - I agree. This is about detection and producing an informative error message. At this time I don't think we can or should support numeric parameters that start at any point.
          Hide
          Michael Dick added a comment -

          While in theory we could support positional parameters that do not start with 1, or contain gaps I don't think there's a compelling reason to add such support.

          We should not throw an ArrayIndexOutOfBoundsException during query execution though. I've checked in changes that will detect the condition when the query is created and throw a UserException that explains why the query string is bad.

          Show
          Michael Dick added a comment - While in theory we could support positional parameters that do not start with 1, or contain gaps I don't think there's a compelling reason to add such support. We should not throw an ArrayIndexOutOfBoundsException during query execution though. I've checked in changes that will detect the condition when the query is created and throw a UserException that explains why the query string is bad.
          Michael Dick made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 2.2.0 [ 12315910 ]
          Resolution Fixed [ 1 ]
          Michael Dick made changes -
          Link This issue is related to OPENJPA-1995 [ OPENJPA-1995 ]
          Hide
          David Blevins added a comment -

          From what I recall from EJB 3.0 when that group still had JPA, is that the intent of "input parameters are numbered starting from 1" is to simply say 1 as opposed to 0. The line that reads "the same parameter can be used more than once in the query string", should simply read "the same parameter can be used zero or more times in the query string". That was the intent as far as I recall.

          Not even printf requires you to use all your input parameters. It's ok to skip some, even the first one. The 'start at 1' is for the input side, not the output side (in this case the query itself)

          For example:

          public class PositionalParametersTest extends TestCase {
          
              public void testPattern() throws Exception {
          
                  final String query = "SELECT $2 FROM $3 AS $2";
          
                  final String result = "one two three four".replaceFirst("(one) (two) (three) (four)", query);
          
                  assertEquals("SELECT two FROM three AS two", result);
              }
          
              public void testFormatter() throws Exception {
          
                  final String query = "SELECT %2$s FROM %3$s AS %2$s";
          
                  final String result = String.format(query, "one", "two", "three", "four");
          
                  assertEquals("SELECT two FROM three AS two", result);
              }
          }
          

          It is very very useful to be able to edit any part of a query string while trying to get things right or debugging. One of those things you don't do because you want to, but because you're trying to get to where you want to go.

          Being able to tweak query strings without recompiling code is a pretty useful thing all on its own.

          Show
          David Blevins added a comment - From what I recall from EJB 3.0 when that group still had JPA, is that the intent of "input parameters are numbered starting from 1" is to simply say 1 as opposed to 0. The line that reads "the same parameter can be used more than once in the query string", should simply read "the same parameter can be used zero or more times in the query string". That was the intent as far as I recall. Not even printf requires you to use all your input parameters. It's ok to skip some, even the first one. The 'start at 1' is for the input side, not the output side (in this case the query itself) For example: public class PositionalParametersTest extends TestCase { public void testPattern() throws Exception { final String query = "SELECT $2 FROM $3 AS $2" ; final String result = "one two three four" .replaceFirst( "(one) (two) (three) (four)" , query); assertEquals( "SELECT two FROM three AS two" , result); } public void testFormatter() throws Exception { final String query = "SELECT %2$s FROM %3$s AS %2$s" ; final String result = String .format(query, "one" , "two" , "three" , "four" ); assertEquals( "SELECT two FROM three AS two" , result); } } It is very very useful to be able to edit any part of a query string while trying to get things right or debugging. One of those things you don't do because you want to, but because you're trying to get to where you want to go. Being able to tweak query strings without recompiling code is a pretty useful thing all on its own.
          Hide
          David Blevins added a comment -

          So here's what I got. Perspective of Linda is that "attempting to bind a parameter that did not correspond to a parameter in the query is intended to cause an exception." She didn't comment on how strict we should be with the 1 thing. Certainly the spec says 1 in no uncertain terms.

          I ran a test with Hibernate and EclipseLink and both of them will allow this:

          Query query = entityManager.createQuery("SELECT m from Movie as m WHERE m.title = ?2 AND m.year = ?4");
          query.setParameter(2, "Foo");
          query.setParameter(4, 2011);
          return query.getResultList();

          Both of them will throw an exception from the setParameter method if the index doesn't exist, but neither cares what numbers you do or don't use beyond that.

          So at this point I would just have to say you're in the clear from a letter of the spec perspective, but in terms of practicality (editing huge queries can take hours and relabeling index continuously just slows you down) and in compatibility with current EclipseLink and Hibernate implementations and past OpenJPA versions, it would be great to at least get a vendor specific flag to turn on/off the strict behavior.

          If we had something that could be set at the unit level, that would be a very big help to us and potentially other users migrating or upgrading.

          Show
          David Blevins added a comment - So here's what I got. Perspective of Linda is that "attempting to bind a parameter that did not correspond to a parameter in the query is intended to cause an exception." She didn't comment on how strict we should be with the 1 thing. Certainly the spec says 1 in no uncertain terms. I ran a test with Hibernate and EclipseLink and both of them will allow this: Query query = entityManager.createQuery("SELECT m from Movie as m WHERE m.title = ?2 AND m.year = ?4"); query.setParameter(2, "Foo"); query.setParameter(4, 2011); return query.getResultList(); Both of them will throw an exception from the setParameter method if the index doesn't exist, but neither cares what numbers you do or don't use beyond that. So at this point I would just have to say you're in the clear from a letter of the spec perspective, but in terms of practicality (editing huge queries can take hours and relabeling index continuously just slows you down) and in compatibility with current EclipseLink and Hibernate implementations and past OpenJPA versions, it would be great to at least get a vendor specific flag to turn on/off the strict behavior. If we had something that could be set at the unit level, that would be a very big help to us and potentially other users migrating or upgrading.
          Hide
          Michael Dick added a comment -

          I'd be fine with opening a new improvement to allow this, and if there is sufficient interest we'd look into it. Pinaki's point about the prepared SQL cache is particularly concerning (for me at least). I suspect any changes we make in this area will affect the cache as well. In the current implementation (and probably in previous releases) this sort of query will not work as expected (beyond the ArrayIndex exception).

          It's just my personal opinion, but I think it's unlikely that there will be many queries that use positional parameters and do not already start at one. I suspect most of those are copy / paste jobs from JDBC prepared statements which do start at 1. Those that aren't copied or pasted from existing statements would probably prefer the named parameter support (which is more friendly anyway). I could be all wet here though - and I can see where it would be a real pain if you had to go back and update the positional args.

          Show
          Michael Dick added a comment - I'd be fine with opening a new improvement to allow this, and if there is sufficient interest we'd look into it. Pinaki's point about the prepared SQL cache is particularly concerning (for me at least). I suspect any changes we make in this area will affect the cache as well. In the current implementation (and probably in previous releases) this sort of query will not work as expected (beyond the ArrayIndex exception). It's just my personal opinion, but I think it's unlikely that there will be many queries that use positional parameters and do not already start at one. I suspect most of those are copy / paste jobs from JDBC prepared statements which do start at 1. Those that aren't copied or pasted from existing statements would probably prefer the named parameter support (which is more friendly anyway). I could be all wet here though - and I can see where it would be a real pain if you had to go back and update the positional args.
          David Blevins made changes -
          Link This issue is related to OPENJPA-1999 [ OPENJPA-1999 ]
          Michael Dick made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Michael Dick
              Reporter:
              Han Hong Fang
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development