silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

SQLSelect regression with subqueries #11276

Open marwan38 opened 2 weeks ago

marwan38 commented 2 weeks ago

Module version(s) affected

5

Description

Regression within the SQLSelect class causes sub queries to be duplicated. This can be very easily reproduced in a blank silverstripe project.

How to reproduce

The regression itself:

Creating the select statement

$sql = SQLSelect::create('*', '( SELECT DISTINCT "SiteTree"."ClassName" FROM "SiteTree" ) as FINAL');

Output

SELECT *
 FROM ( SELECT DISTINCT "SiteTree"."ClassName" FROM "SiteTree" ) as FINAL AS "( SELECT DISTINCT
SiteTree.ClassName FROM SiteTree ) as FINAL" 

Note that the subquery is being duplicated.

Possible Solution

no response

Additional Context

I noticed that the issue cannot be reproduced when double quotations (") are left out of the query. Following the same reproduction steps WITHOUT the double quotations results in the correct output:

$sql = SQLSelect::create('*', '( SELECT DISTINCT SiteTree.ClassName FROM SiteTree ) as FINAL');

Output

SELECT *
 FROM ( SELECT DISTINCT SiteTree.ClassName FROM SiteTree ) as FINAL 

The reproduction code is an MVP, our actual use case is a little bit more complex with additional where statements, and froms. This worked just fine in silverstripe v4. A closer reproduction would be something like

$siteTree = SiteTree::get();
$query = $siteTree->dataQuery()->sql();
$finalQuery = SQLSelect::create('*', '(' . $query . ') as FINAL');
Debug::dump($finalQuery->sql());

Output (Note the duplication after the aliasing "AS FINAL AS "...."):

SELECT *
 FROM (SELECT DISTINCT "SiteTree_Live"."ClassName", "SiteTree_Live"."LastEdited",
"SiteTree_Live"."Created", "SiteTree_Live"."CanViewType", "SiteTree_Live"."CanEditType",
"SiteTree_Live"."Version", "SiteTree_Live"."URLSegment", "SiteTree_Live"."Title",
"SiteTree_Live"."MenuTitle", "SiteTree_Live"."Content", "SiteTree_Live"."MetaDescription",
"SiteTree_Live"."ExtraMeta", "SiteTree_Live"."ShowInMenus", "SiteTree_Live"."ShowInSearch",
"SiteTree_Live"."Sort", "SiteTree_Live"."HasBrokenFile", "SiteTree_Live"."HasBrokenLink",
"SiteTree_Live"."ReportClass", "SiteTree_Live"."ParentID", "SiteTree_Live"."ID", 
            CASE WHEN "SiteTree_Live"."ClassName" IS NOT NULL THEN "SiteTree_Live"."ClassName"
            ELSE 'SilverStripe\\CMS\\Model\\SiteTree' END AS "RecordClassName"
 FROM "SiteTree_Live" 
 ORDER BY "SiteTree_Live"."Sort" ASC) as FINAL AS "(SELECT DISTINCT SiteTree_Live.ClassName,
SiteTree_Live.LastEdited, SiteTree_Live.Created, SiteTree_Live.CanViewType,
SiteTree_Live.CanEditType, SiteTree_Live.Version, SiteTree_Live.URLSegment, SiteTree_Live.Title,
SiteTree_Live.MenuTitle, SiteTree_Live.Content, SiteTree_Live.MetaDescription,
SiteTree_Live.ExtraMeta, SiteTree_Live.ShowInMenus, SiteTree_Live.ShowInSearch, SiteTree_Live.Sort,
SiteTree_Live.HasBrokenFile, SiteTree_Live.HasBrokenLink, SiteTree_Live.ReportClass,
SiteTree_Live.ParentID, SiteTree_Live.ID, 
            CASE WHEN SiteTree_Live.ClassName IS NOT NULL THEN SiteTree_Live.ClassName
            ELSE 'SilverStripe\\CMS\\Model\\SiteTree' END AS RecordClassName
 FROM SiteTree_Live 
 ORDER BY SiteTree_Live.Sort ASC) as FINAL" 

Changing the alias to be resolved as the array index, like

SQLSelect::create('*', ['Final' => '(' . $siteTreeSql . ')']);

resolves the same way.

Validations

GuySartorelli commented 2 weeks ago

You say this is a regression - what is the latest version you saw this working in?

marwan38 commented 2 weeks ago

You say this is a regression - what is the latest version you saw this working in?

We're on version 4.13.42 of the framework I believe. Upgrading straight to 5.2

michalkleiner commented 2 weeks ago

@marwan38 the example SQL ($sql = SQLSelect::create('*', '( SELECT DISTINCT "SiteTree"."ClassName" FROM "SiteTree" ) as FINAL');) is the same in both how to reproduce and possible solution. It won't fix the issue, but maybe will give more context on what you found out so far. Perhaps should be different?

marwan38 commented 2 weeks ago

@michalkleiner

It's mentioned just above the example,.. my mistake I've updated it. An important note right below is that it's not exactly a "possible solution" but rather something I discovered which may help debug the issue.

GuySartorelli commented 2 weeks ago

I'm going to move that into the "additional context" section then - since that's what you're saying it is.