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

Many to Many relation query filtering issue #1822

Closed
aalex21 opened this issue Feb 8, 2021 · 5 comments · Fixed by #1843
Closed

Many to Many relation query filtering issue #1822

aalex21 opened this issue Feb 8, 2021 · 5 comments · Fixed by #1843

Comments

@aalex21
Copy link

aalex21 commented Feb 8, 2021

A use case is explained below, when implemented using Elide/ (may be ORM issue), the filtering is not working as expected.

Scenario is,

  1. Books can be associated with multiple Tags (ex: comic, new_releases, horror etc).
  2. Books and Tags relation is many-to-many
  3. I need to query the books which are "comic AND new-releases"

The DB design is as follows,

  1. Table 1 - book (id, description, ...)
  2. Table 2 - tag (id, description, ...)
  3. Table 3 - book_tag (book_id, tag_id) (relationship table which holds the foreign key with books and tags tables)

Query formed using elide is as follows
GET /api/v1/books?filter=tags.id==comic;tags.id==newReleases

The resulting query created is
select distinct book0_.id as id1_0_ from book book0_ left outer join book_tag tag1_ on book0_.id=tag1_.book_id left outer join tag tag2_ on tag1_.tag_id=tag2_.id where (lower(tag2_.id) in (lower(?))) and (lower(tag2_.id) in (lower(?))) limit ?

This query will never satisfy, as the SQL search in the same row with different Id.

The actual query to be formed should be based on "groupby() having count() = X"

Question:

  1. Can we form a filter condition in Elide to support this ?
  2. Is it expected from Elide to have some logic from its side to detect primary key filtering and form the SQL differently ?
@aklish
Copy link
Member

aklish commented Feb 9, 2021

I understand the issue. It looks a lot like https://vladmihalcea.com/sql-query-parent-rows-all-children-match-filtering-criteria/.

To answer your questions, Elide does not support this kind of filter today in the database.

Generating the JPQL in a general way for this kind of query seems like it could be tricky. Another alternative might be to evaluate part of the filter expression in memory - possibly leveraging the logic Elide uses to evaluate filter expressions on computed fields (@ComputedAttribute).

As a workaround/test, try adding another computed attribute to the Tag model that just mirrors/returns the ID field. Then change the client filter replacing tag.id with the computed field instead.

If you are worried about the performance of this, it should be possible to filter by at least one of the IDs in the database either by:

  • Modifying the client query to filter on a combination of the actual ID and the computed attribute ID.
  • Building a custom datastore

Let me know if you try the workaround above or have questions.

@aalex21
Copy link
Author

aalex21 commented Feb 11, 2021

The above workarounds solves to some extend, but not as a complete solution. Some concerns of this solution

  1. In case of a complex query (including tags and other attribute (self as well other relationships attribute based)). The reformation of filter at code level (based on computed attribute) is very complex/ some case impossible
  2. Pagination is not working as expected since some computations are happening in memory

It will be great, if SQL formation can be controlled by Elide itself which will be a complete solution.
Assume tagging scenarios are very common use cases.

Let me know, if Elide any plan to look into this ?

@aklish
Copy link
Member

aklish commented Feb 11, 2021

I've spent some time looking at this and have a few thoughts:

  1. There is another workaround (see example below), where you can register JPQL filter fragment generators for each operator, model, and field.
  2. I agree tags is a common scenario. Elide supports the hasmember operator in filters which works for attribute collections, but unfortunately is disabled when traversing to-many relationships (related to limitations in the MEMBER OF JPQL operator). Regardless, I think the workaround below can be generalized so Elide can have a complete solution here.

I tried the following workaround today on Elide 5 with the following caveats:

  • I didn't test Elide 4 so it may require one or more patches to work.
  • The code will require a few small edits in Elide 4 because the contract is different (Class instead of Type).
  • It is using a Book with multiple authors example (many to many relationship). You will have to adjust the example for your models.
  • The JPQL fragment below relies on the table aliases that Elide generates. I will explain below.
  • Type coercion is not taking place because the client filtered field and the JPQL filtered field are different. Assuming your ID is a String, it shouldn't be an issue. Otherwise, a patch may be required to get this to work.

Here is my example query. Note that the filter is on the relationship instead of the relationship ID:
/book?filter[book]=authors==1;authors==2

I registered a custom JPQLPredicateGenerator for the IN operator on the Book type for the field "authors". A fragment like this can be added somewhere in your service startup logic.

        FilterTranslator.registerJPQLGenerator(Operator.IN, new ClassType(Book.class), "authors", (columnAlias, params) -> {
            String param = params.get(0).getPlaceholder();

            return String.format("EXISTS (SELECT 1 FROM Author a WHERE a.id = example_Book_authors.id AND a.id = %s)", param);
        });

This fragment will run a separate EXISTS subquery for each filter predicate in the expression. The table alias (example_Book_authors) is generated by Elide from the full class name (example.Book) and the relationship (authors). Note that the example doesn't generate a correct expression for multiple parameters (that is easy to fix, but this is just a test/example).

In short, I think we can fix Elide 5 (elide 4 would require a contract change) with a similar approach that will work in the general case (without having to register a JPQL fragment generator). Let me know if this workaround unblocks you.

@aalex21
Copy link
Author

aalex21 commented Feb 16, 2021

Appreciate for the understanding and quick PRs...

There is a concern, May be, EXISTS cannot solve this issue,

We need the result as -- "All books which has BOTH tag1 AND tag2"

Generated SQL should be like -- (in case of 2 tag condition)
"select distinct book0_.id as id1_0_ from book book0_ inner join book_tag tags1_ on book.id=tag.book_id inner join tag tag2_ on tags1_.tag_id=tag2_.id where (tag2_.id=? or tag2_.id=?) group by book0_.id having count(book0_.id) = 2 order by book0_.id asc limit ?"

SQL level can solve only by "groupBy() and Having() clauses"

@aklish
Copy link
Member

aklish commented Feb 16, 2021

The latest PR should address the issue. See one of the tests below.
I agree the SQL above would also address the issue (likely in a more performant way), but it is difficult to generalize and handle all of the edge cases for predicate logic.

GET /book?filter=authors.id=hasmember=1;authors.id=hasmember=2

{
    "data": [
        {
            "type": "book",
            "id": "1",
            "attributes": {
                "awards": [
                    "National Book Award",
                    "Booker Prize"
                ],
                "chapterCount": 0,
                "editorName": null,
                "genre": "Literary Fiction",
                "language": "English",
                "publishDate": 0,
                "title": "The Old Man and the Sea"
            },
            "relationships": {
                "authors": {
                    "data": [
                        {
                            "type": "author",
                            "id": "1"
                        },
                        {
                            "type": "author",
                            "id": "2"
                        }
                    ]
                },
                "chapters": {
                    "data": [
                        
                    ]
                },
                "editor": {
                    "data": {
                        "type": "editor",
                        "id": "1"
                    }
                },
                "publisher": {
                    "data": {
                        "type": "publisher",
                        "id": "1"
                    }
                }
            }
        }
    ]
}

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.

2 participants