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

filterAny uses AND when queries are aggregated resulting in poor DX. #10744

Closed
LiamKearn opened this issue Mar 30, 2023 · 2 comments
Closed

filterAny uses AND when queries are aggregated resulting in poor DX. #10744

LiamKearn opened this issue Mar 30, 2023 · 2 comments

Comments

@LiamKearn
Copy link
Contributor

LiamKearn commented Mar 30, 2023

Affected Version

4.8 - Not supported but is probably present in new versions based on the lack of changes to filterAny I could fine while looking for this bug.

Description

This is all adhoc code (won't run), should be easy to recreate without using the below:

<?php

class MyPage extends DataObject
{
    private static $many_many = [
        'RelationAInstances' => RelationA::class,
        'RelationBInstances' => RelationB::class,
    ];
}

class RelationA extends DataObject { ... }
class RelationB extends DataObject { ... }

$p = [];

$s = MyPage::get()->filterAny([
    'RelationAInstances.Count():GreaterThan' => 0,
    'RelationBInstances.Count():GreaterThan' => 0,
])->sql($p);

var_dump($s, $p);
die;

Results in incorrect (at least from an API level) SQL:

SELECT DISTINCT
    ...
FROM
    ...
GROUP BY
    ...
HAVING
    (COUNT("relationas_RelationAInstances"."ID") > 0)
     AND (COUNT("relationbs_RelationBInstances"."ID") > 0);

Acceptance criteria

  • filterAny on ManyMany relations use OR operator and return results if either conditions are true.
  • Fix is targeted at 5.1. We won't fix this in CMS 4.
  • Change log explicitely calls out this bug

Note

  • While this is a bug and it might be able to fix it without any API change, the behavioural change is substantial and beyond what we would feel comfortable shipping in a patch release.
  • We didn't explicitly test the result of the query ... just the generated SQL.

PRs

@maxime-rainville
Copy link
Contributor

Manage to replicate the issue with this code snippet and frameworktest installed.

<?php

use SilverStripe\Dev\BuildTask;

class Boom extends BuildTask
{

    public function run($request)
    {
        $p = [];
        $s = RelationFieldsTestPage::get()->filterAny([
            'ManyManyCompanies.Count():GreaterThan' => 0,
            'ManyManyPages.Count():GreaterThan' => 0,
        ])->sql($p);

        var_dump($s, $p);
        die;
    }
}

That's the SQL I got back.

SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited", "SiteTree"."Created", "SiteTree"."CanViewType", "SiteTree"."CanEditType", "SiteTree"."Version", "SiteTree"."URLSegment", "SiteTree"."Title", "SiteTree"."MenuTitle", "SiteTree"."Content", "SiteTree"."MetaDescription", "SiteTree"."ExtraMeta", "SiteTree"."ShowInMenus", "SiteTree"."ShowInSearch", "SiteTree"."Sort", "SiteTree"."HasBrokenFile", "SiteTree"."HasBrokenLink", "SiteTree"."ReportClass", "SiteTree"."RelationFieldsTestPageID", "SiteTree"."ParentID", "RelationFieldsTestPage"."HasOneCompanyID", "RelationFieldsTestPage"."HasOnePageID", "RelationFieldsTestPage"."HasOnePageWithSearchID", "SiteTree"."ID", 
                        CASE WHEN "SiteTree"."ClassName" IS NOT NULL THEN "SiteTree"."ClassName"
                        ELSE 'SilverStripe\\CMS\\Model\\SiteTree' END AS "RecordClassName"
 FROM "SiteTree" LEFT JOIN "RelationFieldsTestPage" ON "RelationFieldsTestPage"."ID" = "SiteTree"."ID" LEFT JOIN "RelationFieldsTestPage_ManyManyCompanies" AS "manymanycompanies_RelationFieldsTestPage_ManyManyCompanies" ON "manymanycompanies_RelationFieldsTestPage_ManyManyCompanies"."RelationFieldsTestPageID" = "SiteTree"."ID" LEFT JOIN "Company" AS "manymanycompanies_Company" ON "manymanycompanies_RelationFieldsTestPage_ManyManyCompanies"."CompanyID" = "manymanycompanies_Company"."ID" LEFT JOIN "RelationFieldsTestPage_ManyManyPages" AS "manymanypages_RelationFieldsTestPage_ManyManyPages" ON "manymanypages_RelationFieldsTestPage_ManyManyPages"."RelationFieldsTestPageID" = "SiteTree"."ID" LEFT JOIN "SiteTree" AS "manymanypages_SiteTree" ON "manymanypages_RelationFieldsTestPage_ManyManyPages"."SiteTreeID" = "manymanypages_SiteTree"."ID"
 WHERE ("SiteTree"."ClassName" IN (?))
 GROUP BY "SiteTree"."ID", "SiteTree"."ID"
 HAVING (COUNT("manymanycompanies_Company"."ID") > ?)
 AND (COUNT("manymanypages_SiteTree"."ID") > ?)
 ORDER BY "SiteTree"."Sort" ASC

I would expect the HAVING clause to be HAVING (COUNT("manymanycompanies_Company"."ID") > ?) OR (COUNT("manymanypages_SiteTree"."ID") > ?)

@GuySartorelli
Copy link
Member

PRs merged. Will be released in CMS 5.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants