Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elide incorrectly identifies embedded properties as relationships in RootCollectionFetchQueryBuilder.build() #2684

Closed
LamWizy opened this issue Jun 23, 2022 · 2 comments · Fixed by #2686

Comments

@LamWizy
Copy link
Contributor

LamWizy commented Jun 23, 2022

Expected Behavior

It should be possible to sort by an embedded property when using paging and filtering at the same time.

Current Behavior

When sorting by an embedded property, the error message "Combination of pagination, sorting over relationship and filtering over toMany relationships unsupported" is returned if paging and filtering is used.

Possible Solution

In RootCollectionFectQueryBuilder.build(), the sortOverRelationship variable is set to true if a Sorting item includes an embedded property, instead of being set to true only if a Sorting item includes relationship properties.

It should be possible to fix it by excluding Sorting items that only includes embedded properties, from the list of Sorting items to test for the sortOverRelationship variable.

Steps to Reproduce (for bugs)

I tried to reproduce the issue by adding a pagination option in the RootCollectionFetchQueryBuilderTest.testRootFetchWithRelationshipSortingFilters() unit test, but it doesn't seem to trigger the incorrect behavior.

Context

I am trying to filter a list of entities, while applying pagination and sorting. I've opened this issue because I'm trying to fix the bug by myself but I haven't had much luck so far, so any help or comment would be appreciated.

Your Environment

  • Elide version used: 6.1.7-snapshot
  • Environment name and version (Java 1.8.0_152): Java 11.0.12
  • Operating System and version: Windows 10
  • Link to your project: N/A
@LamWizy
Copy link
Contributor Author

LamWizy commented Jun 23, 2022

Here is a UT that reproduces the issue (to put in RootCollectionFetchQueryBuilderTest.java):

    @Test
    public void testRootFetchWithRelationshipSortingFiltersAndPaginationOnEmbedded() {
        Map<String, Sorting.SortOrder> sorting = new HashMap<>();
        sorting.put("price.total", Sorting.SortOrder.desc);

        Path.PathElement idBook = new Path.PathElement(Book.class, Chapter.class, "chapters");
        Path.PathElement idChapter = new Path.PathElement(Chapter.class, long.class, "id");
        Path idPath = new Path(List.of(new Path.PathElement[]{idBook, idChapter}));

        FilterPredicate chapterIdPredicate = new InPredicate(idPath, 1);

        PaginationImpl pagination = new PaginationImpl(ClassType.of(Book.class), 0, 3, 10, 10, true, false);

        EntityProjection entityProjection = EntityProjection
                .builder().type(Book.class)
                .pagination(pagination)
                .sorting(new SortingImpl(sorting, Book.class, dictionary))
                .filterExpression(chapterIdPredicate)
                .build();

        RootCollectionFetchQueryBuilder builder = new RootCollectionFetchQueryBuilder(
                entityProjection,
                dictionary,
                new TestSessionWrapper()
        );

        TestQueryWrapper query = (TestQueryWrapper) builder.build();

        String expected =
                "SELECT DISTINCT example_Book FROM example.Book AS example_Book"
                    + " LEFT JOIN example_Book.chapters example_Book_chapters"
                    + " WHERE example_Book_chapters.id IN (:chapters_id_XXX)"
                    + " order by example_Book.price.total desc";

        String actual = query.getQueryText();
        actual = actual.trim().replaceAll(" +", " ");
        actual = actual.replaceFirst(":chapters_id_\\w+", ":chapters_id_XXX");

        assertEquals(expected, actual);
    }

This will throw an InvalidValueException instead of generating a SQL query. I am currently trying to fix the code by myself, I will let you know if I succeed. :)

@LamWizy
Copy link
Contributor Author

LamWizy commented Jun 24, 2022

I have found a fix and created a pull request for it; since this is my first contribution, please let me know if there's anything wrong with it.

LamWizy pushed a commit to LamWizy/elide that referenced this issue Jun 27, 2022
LamWizy pushed a commit to LamWizy/elide that referenced this issue Jul 5, 2022
aklish added a commit that referenced this issue Jul 13, 2022
#2686)

* Fixed query builder so that it does not treat embedded properties as relationships anymore. (#2684)

* Removed unused imports. (#2684)

* Replaced isRelationship() method with dictionary.isRelation(). (#2684)

* Fix style formatting. (#2684)

Co-authored-by: Lâm Tran Duy <[email protected]>
Co-authored-by: Aaron Klish <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant