Jackrabbit Content Repository
  1. Jackrabbit Content Repository
  2. JCR-3089

javax.jcr.RepositoryException when a JOIN SQL2 query is send via Davex and has results

    Details

      Description

      see the following thread for details:
      http://www.mail-archive.com/users@jackrabbit.apache.org/msg17975.html

      assuming a data structure as follows:
      /foo [nt:unstructured]
      /foo/bar [nt:unstructured]
      /foo/bar@lala = huii (lala is string property of bar)
      /ding [nt:unstructured]
      /ding@dong = ##barUUID### (dong is a property of type "Reference")

      then the following code will throw an exception:

      DavexClient Client = new DavexClient(url);
      Repository repo = Client.getRepository();
      Credentials sc = new SimpleCredentials("admin","admin".toCharArray());
      Session s = repo.login(sc,workspace);

      QueryManager qm = s.getWorkspace().getQueryManager();

      String sql = "SELECT data.* FROM [nt:unstructured] AS data WHERE data.lala= 'huii'";
      sql = "SELECT * FROM [nt:unstructured] AS data INNER JOIN [nt:unstructured] AS referring ON referring.[dong] = data.[jcr:uuid] WHERE data.lala = 'huii'";
      sql = "SELECT * FROM [nt:unstructured] AS data INNER JOIN [nt:unstructured] AS referring ON ISDESCENDANTNODE(data, referring) WHERE data.lala = 'huii'";
      Query query = qm.createQuery(sql, Query.JCR_SQL2);
      QueryResult qr = query.execute();

      The first query works just fine and I can iterate over the result. Neither the second nor the third query works.
      In both cases I end up with a javax.jcr.RepositoryException. Note the exception only happens if the query returns results. Aka a join will work just fine if it matches no rows.

      1. JCR-3089.patch
        4 kB
        Alex Parvulescu
      2. JCR-3089-v2.patch
        3 kB
        Alex Parvulescu

        Activity

        Hide
        Alex Parvulescu added a comment -

        proposed patch + test.

        As Jukka suggested, the problem was the SearchResourceImpl didn't know how to handle join results.

        I'm not sure what's the best way to handle this (I just used the first selector in the join, similar to what already existed in the implementation in the case of multiple selectors in a select).

        I also added a test for JCR-2543, a limit-enabled query, to show that the limit constraint is being respected on 2.3

        Show
        Alex Parvulescu added a comment - proposed patch + test. As Jukka suggested, the problem was the SearchResourceImpl didn't know how to handle join results. I'm not sure what's the best way to handle this (I just used the first selector in the join, similar to what already existed in the implementation in the case of multiple selectors in a select). I also added a test for JCR-2543 , a limit-enabled query, to show that the limit constraint is being respected on 2.3
        Hide
        Christian Stocker added a comment -

        Works without exception now (but didn't check the result, if it's useful )
        But you have to remove
        <scope>test</scope>
        from the pom.xml for jackrabbit-core to make it compile

        https://gist.github.com/81882d2d3654c21c49d9

        Show
        Christian Stocker added a comment - Works without exception now (but didn't check the result, if it's useful ) But you have to remove <scope>test</scope> from the pom.xml for jackrabbit-core to make it compile https://gist.github.com/81882d2d3654c21c49d9
        Hide
        angela added a comment -

        quickly had a look at the proposed patch and noticed that it uses jackrabbit-core specific functionality:

        +import org.apache.jackrabbit.core.query.lucene.join.JoinRow;

        the aim of the jcr-server project however was not to rely on jackrabbit-core specific features.
        as far as i remember there is otherwise not dependency to jackrabbit-core (there was one in
        the transaction handling which we commented later on with the remark:
        "// commented, since server should be jackrabbit independent".)

        i would therefore suggest that we try to find a solution that doesn't rely on jackrabbit-core. was
        that feasible?

        Show
        angela added a comment - quickly had a look at the proposed patch and noticed that it uses jackrabbit-core specific functionality: +import org.apache.jackrabbit.core.query.lucene.join.JoinRow; the aim of the jcr-server project however was not to rely on jackrabbit-core specific features. as far as i remember there is otherwise not dependency to jackrabbit-core (there was one in the transaction handling which we commented later on with the remark: "// commented, since server should be jackrabbit independent".) i would therefore suggest that we try to find a solution that doesn't rely on jackrabbit-core. was that feasible?
        Hide
        Alex Parvulescu added a comment -

        ouch, my bad

        I'll dig some more for a cleaner solution

        Show
        Alex Parvulescu added a comment - ouch, my bad I'll dig some more for a cleaner solution
        Hide
        Alex Parvulescu added a comment -

        ...and I'm back
        patch v2 clean as a baby's bottom, hehe.

        it appears that if you call row.getPath("name") works for simple rows and join rows, as opposed to row.getPath() which fails in the case of a join.

        Show
        Alex Parvulescu added a comment - ...and I'm back patch v2 clean as a baby's bottom, hehe. it appears that if you call row.getPath("name") works for simple rows and join rows, as opposed to row.getPath() which fails in the case of a join.
        Hide
        angela added a comment -

        > it appears that if you call row.getPath("name") works for simple rows and join rows, as opposed to row.getPath() which fails in the case of a join.

        could you add that short explanation in the code as well? that makes the code more readable than just referring to a bug nummer.
        anybody looking at the code will immediately understand what you are doing and why...

        something like:

        • String itemPath = row.getPath();
          + /* Use Row#getPath(String) which works for both simple rows and join rows (in contrast to Row#getPath()
          + see also https://issues.apache.org/jira/browse/JCR-3089 */
          + final String itemPath = row.getPath(sn.get(0));

        thanks.

        Show
        angela added a comment - > it appears that if you call row.getPath("name") works for simple rows and join rows, as opposed to row.getPath() which fails in the case of a join. could you add that short explanation in the code as well? that makes the code more readable than just referring to a bug nummer. anybody looking at the code will immediately understand what you are doing and why... something like: String itemPath = row.getPath(); + /* Use Row#getPath(String) which works for both simple rows and join rows (in contrast to Row#getPath() + see also https://issues.apache.org/jira/browse/JCR-3089 */ + final String itemPath = row.getPath(sn.get(0)); thanks.
        Hide
        Alex Parvulescu added a comment -

        yes, sure. I've added it to the code.

        @Christian, I didn't check the results either if the queries stop working, somebody will say something probably.

        thanks Angela for keeping an eye out!

        if all is ok, I'll commit the patch directly. I don't see a need for a v3.

        Show
        Alex Parvulescu added a comment - yes, sure. I've added it to the code. @Christian, I didn't check the results either if the queries stop working, somebody will say something probably. thanks Angela for keeping an eye out! if all is ok, I'll commit the patch directly. I don't see a need for a v3.
        Hide
        Alex Parvulescu added a comment -

        I'll take the lack of additional comments as a +1

        fixed on trunk in revision: 1179124.

        Show
        Alex Parvulescu added a comment - I'll take the lack of additional comments as a +1 fixed on trunk in revision: 1179124.
        Hide
        Lukas Kahwe Smith added a comment - - edited

        I noticed that now I get some columns without a dcr:value
        Is this expected?

        <dcr:column>
        <dcr:name>jcr:path</dcr:name>
        <dcr:selectorName>referring</dcr:selectorName>
        </dcr:column>

        Show
        Lukas Kahwe Smith added a comment - - edited I noticed that now I get some columns without a dcr:value Is this expected? <dcr:column> <dcr:name>jcr:path</dcr:name> <dcr:selectorName>referring</dcr:selectorName> </dcr:column>
        Hide
        Alex Parvulescu added a comment -

        that depends on what queries you are running.

        For inner joins, the answer should be no. it wouldn't be normal to have null values for some selectors.
        But you can also have outer joins, where you'll probably have this situation, and it would be ok.

        Can you run the query directly against the repo and see what results you get?
        I didn't add any functional tests to the patch, just a dummy one to make sure that joins over davex don't break anymore.

        Show
        Alex Parvulescu added a comment - that depends on what queries you are running. For inner joins, the answer should be no. it wouldn't be normal to have null values for some selectors. But you can also have outer joins, where you'll probably have this situation, and it would be ok. Can you run the query directly against the repo and see what results you get? I didn't add any functional tests to the patch, just a dummy one to make sure that joins over davex don't break anymore.
        Hide
        Lukas Kahwe Smith added a comment -

        yes this happened for a LEFT OUTER JOIN.
        but thinking about it the problem is more that in my PHP layer we currently just ignore the selectorName, which means that the jcr:path in the result set is probably getting overwritten which is actually what is causing issues on my side.

        so likely everything is ok here ..

        Show
        Lukas Kahwe Smith added a comment - yes this happened for a LEFT OUTER JOIN. but thinking about it the problem is more that in my PHP layer we currently just ignore the selectorName, which means that the jcr:path in the result set is probably getting overwritten which is actually what is causing issues on my side. so likely everything is ok here ..
        Hide
        Alex Parvulescu added a comment -

        > yes this happened for a LEFT OUTER JOIN
        phiew, I was expecting a bug here, good news

        you kinda lost me for a moment, mostly because I'm not 100% sure what's supposed to be in the webdav response.

        if you have some scenarios/tests in mind that you'd like to add, just list them here, I'm sure we'll get around to adding them.
        (unit tests are a strong candidate for documentation sometimes)

        Show
        Alex Parvulescu added a comment - > yes this happened for a LEFT OUTER JOIN phiew, I was expecting a bug here, good news you kinda lost me for a moment, mostly because I'm not 100% sure what's supposed to be in the webdav response. if you have some scenarios/tests in mind that you'd like to add, just list them here, I'm sure we'll get around to adding them. (unit tests are a strong candidate for documentation sometimes)
        Hide
        angela added a comment -

        regarding tests: the only thing that counts with respect to jcr-remoting is if the setup jcr2spi-spi2dav(ex)-jcrserver works properly and
        returns the expected result when using JCR API operations on the jcr-end of the chain. that's our reference and use-case.

        and it can be easily verified by adding tests to jcr2spi test suite. as far as i remember the jcr2dav module has an integrationTesting profile
        that runs all available tests for the mentioned setup.

        Show
        angela added a comment - regarding tests: the only thing that counts with respect to jcr-remoting is if the setup jcr2spi-spi2dav(ex)-jcrserver works properly and returns the expected result when using JCR API operations on the jcr-end of the chain. that's our reference and use-case. and it can be easily verified by adding tests to jcr2spi test suite. as far as i remember the jcr2dav module has an integrationTesting profile that runs all available tests for the mentioned setup.
        Hide
        Alex Parvulescu added a comment -

        yes, I agree.

        when I said test scenarios I was referring to, as you very well put it: "returns the expected result".
        the patch came with a few tests but It did not cover the 'outer join' scenario, as Lukas pointed out.
        the point of the comment was not to find different ways to test, but to find (and add) useful tests.

        I may be wrong but before the patch there were no sql2 query tests. As sql2 would eventually be the default query engine, I'd say welcome to anybody contributing to the test effort, even if it's just scenarios.
        ...as you can see I'm a big fan of sql2

        Show
        Alex Parvulescu added a comment - yes, I agree. when I said test scenarios I was referring to, as you very well put it: "returns the expected result". the patch came with a few tests but It did not cover the 'outer join' scenario, as Lukas pointed out. the point of the comment was not to find different ways to test, but to find (and add) useful tests. I may be wrong but before the patch there were no sql2 query tests. As sql2 would eventually be the default query engine, I'd say welcome to anybody contributing to the test effort, even if it's just scenarios. ...as you can see I'm a big fan of sql2
        Hide
        Jukka Zitting added a comment -

        Merged to the 2.2 branch in revision 1202790.

        Show
        Jukka Zitting added a comment - Merged to the 2.2 branch in revision 1202790.

          People

          • Assignee:
            Alex Parvulescu
            Reporter:
            Lukas Kahwe Smith
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development