spring-data-jpa icon indicating copy to clipboard operation
spring-data-jpa copied to clipboard

detectAlias() in QueryUtils incorreclty detects order by clause alias

Open agzamovr opened this issue 2 years ago • 5 comments

In the Spring Data shipped with Spring Boot 2.7.0 an order by clause alias is detected incorrectly. It seems like the detectAlias method in QueryUtils class is being confused by subquery in the where clause of the following query:

    @Test
    void testQueryAlias() {
        String query = """
                SELECT o
                  FROM Order o
                   AND EXISTS(SELECT 1
                                FROM Vehicle vehicle
                               WHERE vehicle.vehicleOrderId = o.id
                                 AND LOWER(COALESCE(vehicle.make, '')) LIKE :query)
                """;
        String alias = QueryUtils.detectAlias(query);
        assertThat(alias).isEqualTo("vehicle");
    }

This is a simplified version of the real query but the issue is still reproduced with this one. When running this query with PageRequest.of(0, 20, Sort.by("number")) pageable object to sort orders by number Spring Data generates order by vehicle.number asc clause.

There is a similar issue that was fixed and released in Spring Data 2.7.0. However, it looks like the bug is still there.

agzamovr avatar Jun 04 '22 16:06 agzamovr

So far, this appears to be a wrinkle to our subquery detecting logic, which is removes (from ....) subqueries. You have the classic select 1 from ... variation.

It seems possible that if we optionally supported this version, the rest of the logic would flow and find the proper alias as expected.

gregturn avatar Jun 06 '22 15:06 gregturn

@gregturn Thank you for the response. Is this something that will be fixed in upcoming releases or should I change my query?

agzamovr avatar Jun 13 '22 08:06 agzamovr

@agzamovr Is this still an issue? I'm not able to reproduce it on the 2.7.0 tag and up. I've added the following unit test, which passes:

assertThat(detectAlias(
  "SELECT o FROM Order o AND EXISTS(SELECT 1 FROM Vehicle vehicle WHERE vehicle.vehicleOrderId = o.id AND LOWER(COALESCE(vehicle.make, '')) LIKE :query)"))
  .isEqualTo("o");

Similarly, the code that removes the subquery passes this unit test:

assertThat(normalizeWhitespace(removeSubqueries(
  "SELECT o FROM Order o AND EXISTS(SELECT 1 FROM Vehicle vehicle WHERE vehicle.vehicleOrderId = o.id AND LOWER(COALESCE(vehicle.make, '')) LIKE :query)")))
  .isEqualTo("SELECT o FROM Order o AND EXISTS()");

Perhaps I'm just misunderstanding the issue, so additional info may be helpful.

darinmanica avatar Jul 21 '22 23:07 darinmanica

@darinmanica the bug is still reproducible with the latest Spring Boot version 2.7.2. But your point is valid, if I remove new lines from the same query it works as expected:

String query = """
                SELECT o
                  FROM Order o
                 WHERE EXISTS(SELECT 1
                                FROM Vehicle vehicle
                               WHERE vehicle.vehicleOrderId = o.id
                                 AND LOWER(COALESCE(vehicle.make, '')) LIKE :query)
                """;

String alias = detectAlias(query);
assertThat(alias).isEqualTo("vehicle");
// remove all new lines
query = query.replaceAll("\n", "");
alias = detectAlias(query);
assertThat(alias).isEqualTo("o");

Seems like switching from regular Java string to text blocks created the issue. This query is just a toy example of a real query. Tests that execute the real query with the Spring Data repository still fail.

agzamovr avatar Jul 22 '22 06:07 agzamovr

@agzamovr This was very helpful, thank you. I have this fixed in my local environment and will get a PR submitted soon.

darinmanica avatar Jul 22 '22 15:07 darinmanica

@darinmanica is there any reason why your PR #2603 with the fix hasn't been merged yet?

agzamovr avatar Sep 13 '22 17:09 agzamovr

@agzamovr No reason that I'm aware of. PR #2557 and #2582 similarly solve the multi-line problem. I personally feel that #2582 should be merged while #2557 and #2603 (mine) should be discarded. Yet, all 3 remain in waiting-for-triage so I'm not sure which will be merged and which will be discarded as duplicates.

darinmanica avatar Sep 13 '22 22:09 darinmanica

The fix was merged into https://github.com/spring-projects/spring-data-jpa/commit/91cd84e7061c0c655128624d34a0ff41d1f58d7a and additional test cases from the other PRs were folded into https://github.com/spring-projects/spring-data-jpa/commit/f7737014944e606686ca5111574229426ec4484b.

gregturn avatar Sep 28 '22 21:09 gregturn

Backported to 2.7.x. and 2.6.x.

gregturn avatar Sep 29 '22 14:09 gregturn