Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Incorrect query for "all" and "has" filter #576

Closed
wellingguzman opened this issue Nov 7, 2018 · 15 comments
Closed

Incorrect query for "all" and "has" filter #576

wellingguzman opened this issue Nov 7, 2018 · 15 comments

Comments

@wellingguzman
Copy link
Contributor

The all and has filter is generating a bad query.

@wellingguzman wellingguzman added the bug Something isn't working label Nov 7, 2018
@wellingguzman wellingguzman added this to High priority in Bug Triage Nov 7, 2018
@wellingguzman wellingguzman added this to To do in v2.0.6 Nov 7, 2018
@wellingguzman wellingguzman moved this from To do to In progress in v2.0.6 Nov 7, 2018
@wellingguzman wellingguzman moved this from In progress to To do in v2.0.6 Nov 7, 2018
@beac0n
Copy link

beac0n commented Nov 20, 2018

using something like filter[locatios.id][eq]=1, where locations is a many-to-many relation, also leads to a bady query.

@wellingguzman : I'd like to help, as I have run into this issue myself multiple times.
I could provide a pull request, if you would point me into the right direction, where to look in the codebase :-)

@wellingguzman wellingguzman added this to To do in v2.0.10 Nov 26, 2018
@wellingguzman
Copy link
Contributor Author

@beac0n what's the bad query you are getting?

To point you to the right direction, the filters are created here in the doFilter method.

I don't know where is the actual problem, but here is the QueryBuilder this is the object that creates the query based on the filters passed from the doFilter method.

@sebj54
Copy link
Contributor

sebj54 commented Nov 27, 2018

Hi!
I'm having this problem too! And I'd like to help too :)

First of all, what's the correct query? As I never made it work, I'm not sure what I'm doing is correct.

I have a master collection inspirations which has a many-to-many field called saisons (sorry for French names, I had to!). It is linked with the saisons table via the ìnspirations_saisons` associative table.

So we have:

----------------   ------------------------   -----------
| inspirations |   | inspirations_saisons |   | saisons |
|--------------|   |----------------------|   |---------|
|              |   | id                   |   |         |
| id           | ← | inspiration          |   |         |
|              |   | saison               | → | id      |
----------------   ------------------------   -----------

If I want to search for all inspirations with 1 as ID, what should be my API query?

First attempt: saisons.saison.id

http://local.directus.com/_/items/inspirations?fields=*.*,saisons.saison.*&filter[saisons.saison.id][has]=1

When I hit this URL, I get the following error:

{
    "error": {
        "code": 202,
        "message": "Unable to find field \"saison\"",
        "class": "Directus\\Database\\Exception\\FieldNotFoundException",
        "file": "C:\\wamp64\\www\\directus\\src\\core\\Directus\\Database\\SchemaService.php",
        "line": 333
    }
}

Second attempt: saisons

http://local.directus.com/_/items/inspirations?fields=*.*,saisons.saison.*&filter[saisons][has]=1

When I hit this URL, I get the following error:

{
    "error": {
        "code": 9,
        "message": "Failed generating the SQL query. Statement could not be executed (42000 - 1066 - Not unique table\/alias: 'inspirations')",
        "query": "SELECT `inspirations`.`id` AS `id`,`inspirations`.`id` AS `id`,`inspirations`.`titre` AS `titre`,`inspirations`.`couverture` AS `couverture`,`inspirations`.`accroche` AS `accroche`,`inspirations`.`pays` AS `pays`,`inspirations`.`groupe` AS `groupe`,`inspirations`.`duree` AS `duree`,`inspirations`.`type` AS `type`,`inspirations`.`activite` AS `activite`,`inspirations`.`encadrement` AS `encadrement`,`inspirations`.`vol` AS `vol`,`inspirations`.`tags` AS `tags`,`inspirations`.`repas` AS `repas`,`inspirations`.`transport` AS `transport`,`inspirations`.`budget` AS `budget`,`inspirations`.`a_savoir` AS `a_savoir`,`inspirations`.`reference` AS `reference`,`inspirations`.`image_contenu_1` AS `image_contenu_1`,`inspirations`.`image_contenu_2` AS `image_contenu_2`,`inspirations`.`image_contenu_3` AS `image_contenu_3`,`inspirations`.`image_contenu_4` AS `image_contenu_4`,`inspirations`.`image_contenu_5` AS `image_contenu_5`,`inspirations`.`avis_expert` AS `avis_expert`,`inspirations`.`texte_fin` AS `texte_fin`,`inspirations`.`texte_debut` AS `texte_debut`,`inspirations`.`texte_milieu` AS `texte_milieu`,`inspirations`.`carte` AS `carte` FROM `inspirations` WHERE `inspirations`.`id` IN (SELECT `inspirations`.`saisons` AS `saisons` FROM `inspirations` RIGHT JOIN `inspirations` ON `inspirations`.`id` = `inspirations`.`saisons` GROUP BY `inspirations`.`saisons` HAVING COUNT(*) >= '1') ORDER BY `inspirations`.`id` IS NULL,`inspirations`.`id` ASC,`inspirations`.`id` ASC LIMIT 200 OFFSET 0",
        "class": "Directus\\Database\\Exception\\InvalidQueryException",
        "file": "C:\\wamp64\\www\\directus\\src\\core\\Directus\\Database\\TableGateway\\BaseTableGateway.php",
        "line": 732
    }
}

If you don't understand my example, I followed this guide: https://docs.directus.io/app/admin
/relationships.html#many-to-many.
Just replace:

  • movies by inspirations
  • genres by saisons
  • movie_genres by inspirations_saisons

Thanks in advance for your help!

@sebj54
Copy link
Contributor

sebj54 commented Nov 28, 2018

I tried to make it work all this afternoon without success. From what I understood, there is actually no handling at all for filters on a M2M field, that's right?

And to answer myself, I guess the correct filter is my first attempt, but I'm not getting the same message now.

I created a test table with a many to many relation with types table.

--------   --------------   ---------
| test |   | test_types |   | types |
|------|   |------------|   |-------|
|      |   | id         |   |       |
| id   | ← | test       |   |       |
|      |   | type       | → | id    |
--------   --------------   ---------

Now, when I hit this URL: http://local.directus.com/_/items/test?fields=*.*&filter[types.type.id][has]=2, I get this error:

{
    "error": {
        "code": 12,
        "message": "Operator \"has\" only works for one-to-many fields",
        "class": "Directus\\Exception\\UnprocessableEntityException",
        "file": "C:\\wamp64\\www\\directus\\src\\core\\Directus\\Database\\TableGateway\\RelationalTableGateway.php",
        "line": 1404
    }
}

I tried to change some code in QueryBuilder like you said @wellingguzman but I think the problem comes from RelationalTableGateway, one of these two methods: parseDotFilters or doFilter.

@benhaynes
Copy link
Sponsor Member

So sorry for the late reply @sebj54@wellingguzman will be checking this out today to offer some guidance!

@wellingguzman
Copy link
Contributor Author

@sebj54 you are correct, the QueryBuilder uses whatever the doFilter add to the query builder conditionals value. (Ref 1, Ref 2).

I don't know if you shared me your schema before, but I either way I would like if you share the one you are using so I can help you better understand the filters based on your schema.

You are on the right path.

@stale
Copy link

stale bot commented Feb 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@theharshin
Copy link
Contributor

Hey @benhaynes, @bjgajjar is looking into this. Just mentioning here to keep this issue alive 😄

@benhaynes
Copy link
Sponsor Member

Excellent!! This would be a great one to get resolved.

@rijkvanzanten rijkvanzanten added this to In Progress in v2.0.21 Mar 20, 2019
itsmerhp pushed a commit to itsmerhp/api that referenced this issue May 2, 2019
itsmerhp pushed a commit to itsmerhp/api that referenced this issue May 4, 2019
itsmerhp pushed a commit to itsmerhp/api that referenced this issue May 4, 2019
@itsmerhp
Copy link
Contributor

itsmerhp commented May 8, 2019

Hello @benhaynes
I have a question about M2M filter expected behaviour.
For eg: Collection C1 has a many-to-many field cm_relate which is linked with collection C2 and junction collection is JC.

Question : Filter on M2M field cm_relate(c1?fields=*.*&filter[cm_relate.id][eq]=1 OR c1?fields=*.*&filter[cm_relate][like]=search_term) should be applied on junction collection JC or related collection C2?

Actually, based on current structure in M2M relationship, C1 is related with junction collection JC with O2M relationship, and due to that filter is applying currently on junction collection.

@benhaynes
Copy link
Sponsor Member

benhaynes commented May 10, 2019

It's an interesting question! Directus technically treats a M2M as: O2M+M2O. This is nice since it simplifies how many relational types we have, and allows for data management on the junction table.

For example, you might have a schema like this:

C1

  • id
  • title
  • description

JC

  • id
  • c1_id
  • c2_id
  • quantity

C2

  • id
  • title
  • description

The important field above is jc.quantity. So we need to make sure that our platform supports managing the junction collection data too.

Admins not using this feature might be confused by why they need *.*.* (three levels) to access items related through a M2M (they might only expect 2 levels). But I think it's best to be consistent and follow the actual data model.

Soooo, in summary: I think we should use c1?fields=*.*.*&filter[cm_relate.c2_id.id][eq]=1 and apply all filters to the junction. If a user wants to filter on the related data they would go one level deeper.

Another idea (for the future) would be to add a new parameter as a helper for M2M filtering to keep syntax shorter/cleaner. eg: c1?m2m[cm_relate.id][eq]=1 — maybe this could even automatically get the field depth so you don't need to include the fields param. But that's a different discussion.

@rijkvanzanten or community thoughts?

@itsmerhp
Copy link
Contributor

itsmerhp commented May 10, 2019

Hello @benhaynes
Thank you for your explanation.

Please go through following two cases :

  1. c1?fields=*.*.*&filter[cm_relate.quantity][eq]=X
    Expected : Filter should apply on quantity field of Junction Collection JC and records should be fetched from collection C1 accordingly.
  2. c1?fields=*.*.*&filter[cm_relate.c2_id.title][like]=X
    Expected : Filter should apply on title field of C2 collection and records should be fetched from collection C1 accordingly.

Please correct me if anything wrong in above both cases.

And if these two cases are correct, then it is already implemented in this PR, please review it.

@benhaynes
Copy link
Sponsor Member

That looks correct to me!

Does your PR change the basic way that the API works though? I ask because then this is a breaking change, which is a big deal for backwards compatibility. Also, it's a bummer that we need to reference all m2m data in a third relational level... but I can't think of a better way to handle this.

@bjgajjar @rijkvanzanten — thoughts?

@binal-7span
Copy link
Contributor

@benhaynes

The logic which @itsmerhp explains seems correct to me too! I will check the PR ASAP.

binal-7span pushed a commit that referenced this issue May 15, 2019
* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #576 - In progress

* #576 O2M and M20 nested filters

* #576 Fix O2M and M2O nested filters
@binal-7span
Copy link
Contributor

binal-7span commented May 15, 2019

Fixed in #926

Bug Triage automation moved this from High priority to Closed May 15, 2019
binal-7span pushed a commit that referenced this issue Jun 17, 2019
* pgsql 10 initial support

* email_notification column must be set as a boolean

* Handle unique column collisions

* BUG delta in revisions can be null

* BUG transformed the remaining lastInsertValue into getLastGeneratedId()

* Pass new item flag to o2m new items

Closes https://github.com/directus/app/issues/1418

* Don't show popover for 0 items / no template

Closes https://github.com/directus/app/issues/1397

* Bug fix (#848)

* Merge conflict resolve

* Handle item not found exception in collection detail API

* Extended the list of safe tags (#849)

As described in issue #832

* Issue fix #819 (#851)

* Mark adding new item as new in m2m

* Bump version

* Revert composer changes

* Issue fix #843 (#852)

* BUG searches with LIKE on non-textual columns

* Remove the extensions from the API

* Issue fix #847 (#857)

* Issue fix #833 (#859)

* Initial commit for documentation (#844)

* Revert "Initial commit for documentation (#844)" (#868)

This reverts commit 6e85d59.

* BUG Bypass Zend-db choice not to allow nullable boolean fields

* BUG field length were not taken into account

* CHORE dupliacted line

* BUG o2m working + post-alter table event dispatching

* Return object in delete after hook instead of onli ID (#882)

* Add fix for big file sizes

Closes #750

* Add migrations for hash and single-file

* Show correct fields in roles.users

For some reason the database column for options was empty

Closes https://github.com/directus/app/issues/1471

* Delete ISSUE_TEMPLATE.md

* security notice

* Add check for mod_php before setting php_value for upload size

This will prevent errors on systems that don't allow overriding the php
values from within the .htaccess files. This will only check for php 7+
though, as the mod_php directive is version specific. This is okay for
now, as we officially only support PHP 7.1+

* Change field width from integer to string

This will allow the app to render the fields in the correct widths
starting with v7.2.

* Issue fix #854 (#896)

* Add migrations for setting field notes and widths

Lays out the settings a bit nicer and adds setting descriptions.

Fixes https://github.com/directus/app/issues/1379

* Fix sort order of fields on install

* Increase specificity of migrations so it doesn't target non-settings

* Move collection notes to the DB

I'm aware that this makes them english only for the time being.
Once we implement the using the translation column in the app, we
can make them properly translatable.

* Fix abstraction name

* Add migrations for misc fields

Sorting of files, making a couple interfaces required, etc

* Bump version

* Fix: Wrong MIME for extentions in uppercase (#895)

* FEAT more events that invalidate the cache (#892)

* Allowing string relations (#800)

* emoji support for comments and bookmark names

* Use JSON interface for system collections

* Fixing custom primary key primary key column name (#881)

Swapping this variable seems to resolve the issue.

* Bump version

* Issue #885 (#898)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* #885 Removed Test cases

* Issue #886 (#899)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* Issue#886 - Done

* #886 Reverted unwanted code

* Issue #884 (#901)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* Issue #884 - Change (#907)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* #884 change

* Fix#810 (#908)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #810 done

* #810 Reverting Test Cases

* Issue Fix #902 (#909)

* Issue fix #902

* Add migration for allow value nullable in settings table

* Set texttype for value field

* Doc issue fix #84 (#910)

* Issue fix #841 (#911)

* Increase expiry time of tokens from 5 to 20 minutes (#913)

It should still be pretty secure. This allows the app to go easier on
the refreshing, and it makes sure that you can upload large files
without having the token expire halfway through.

* Fix missing ref to 5 min exp

* Issue Fix #863 (#916)

* Issue fix #853 (#918)

* Issue Fix #920 (#922)

* Issue Fix #920

* Issue Fix #920

* Issue fix #879 (#924)

* [thumbnailer] Support for files in subdirectories (#856)

Many websites store images in a complex directory structure. This PR
makes it possible to use thumbnailer in such cases.

For instance :
`/thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg

* defaults cors.max-age to 600 (#921)

* Bump version

* Fix 943 (#947)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #943

* Fix 717 (#944)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #717

* Fix 576 (#926)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #576 - In progress

* #576 O2M and M20 nested filters

* #576 Fix O2M and M2O nested filters

* get proper string length (#933)

Not tested... I only based this PR on:

Ref: 0fce6a4#commitcomment-33408113

* fixed settings logo (#940)

* added collection/table to InvalidFieldException (#956)

* Fix 931 (#936)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #931

* #931

* Issue fix #917 (#960)

* reuse item service instead of using a new instance (#959)

* Issue fix 762 (#961)

* Plain text mail issu resolve (#966)

* Bump version
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
Bug Triage
  
Closed
v2.0.10
  
To do
v2.0.11
  
To do
v2.0.12
  
Awaiting triage
v2.0.21
  
In Progress
v2.0.6
  
To do
Development

No branches or pull requests

8 participants