Problem/Motivation

Views aggregate queries do not escape the fields that is argument of the aggregation function, nor of the GROUP BY part.

See example below, from Drupal\Tests\views\Kernel\QueryGroupByTest:

SELECT "entity_test"."id" AS "id", "entity_test"."langcode" AS "entity_test_langcode", MIN(entity_test.name) AS "entity_test_name", MIN(entity_test.id) AS "id_1"\n
FROM\n
{entity_test} "entity_test"\n
GROUP BY entity_test.id, entity_test_langcode\n
LIMIT 10 OFFSET 0

it is mixing up quoted and non quoted identifiers, now all identifiers should be quoted.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 3191623-23.patch3.59 KBmondrake
#12 3190285-12-test-only.patch937 bytesMedha Kumari
#2 3190285-5-test-only.patch944 bytesmondrake

Issue fork drupal-3191623

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
3190285-5-test-only.patch944 bytes
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Parent issue: #3190285: Entity QueryAggregate does not escape the field » #3189880: Use square brackets syntax in sql queries

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake opened merge request !785

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Views aggregate queries do not escape the fields » Views aggregate queries do not escape the GROUP BY fields
Status: Active » Needs review

Also the aggregate functions field parameters do not get escaped, but let's start from somewhere. Limiting scope to the GROUP BY fields.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Component: views.module » database system
Status: Needs review » Needs work

I start thinking this should be addressed on the database layer instead of on the views’ one. Will work on another patch.

Medha Kumari’s picture

Medha Kumari
Location Jaipur
CreditAttribution: Medha Kumari as a volunteer and at Dotsquares Ltd. commented
Status: Needs work » Needs review
FileSize
3190285-12-test-only.patch937 bytes

Rerolled the patch #5 in Drupal 10.1.x.

Status: Needs review » Needs work

The last submitted patch, 12: 3190285-12-test-only.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review

Here's a new approach based on #11, trying to fix this at the database layer level.

@Medha Kumari thanks for your attempt, but I removed your patch in #12 as it is a wrong reroll and in any case it is not addressing the fix.

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs review » Needs work

All the changes look good to me.
Only as this is a fix on the Database API, I would like to test this in a Database API test.
Therefor could we add a test to Drupal\KernelTests\Core\Database\SelectComplexTest::testGroupBy().
Something like: 

    // Test that the group by field has been quoted.
    $this->assertMatchesRegularExpression('/GROUP BY .task./', $query->__toString());
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Views aggregate queries do not escape the GROUP BY fields » Select queries do not escape the GROUP BY fields
Status: Needs work » Needs review

Thanks @daffie, done.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Uhm no, DruDbal driver fails the SelectComplexTest changes since DBAL uses a different query builder. Need to find a less strict test.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Fixed tests to be less strict so to allow testing with contrib drivers.

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
The fix has testing in the Database API test.
For me it is RTBC.

@mondrake: Thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

Can we add a group by test to the reserved word testing too? That's a bit more concrete test coverage than the regex testing added here.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: Unassigned » mondrake

Good idea.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Done

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
3191623-23.patch3.59 KB

MR testing seems more and more hopelessly broken. https://drupal.slack.com/archives/C51GNJG91/p1673357627190639

Changed to patch workflow.

mondrake closed merge request !785

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs review » Reviewed & tested by the community

The remark of @alexpott has been addressed.
Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 5a193a820f to 10.1.x and 513933dee7 to 10.0.x and b9b6fa5864 to 9.5.x. Thanks!

Backported to 9.5.x to fix that branch too.

  • alexpott committed 5a193a82 on 10.1.x 
    Issue #3191623 by mondrake, Medha Kumari, daffie, alexpott: Select...

  • alexpott committed 513933de on 10.0.x 
    Issue #3191623 by mondrake, Medha Kumari, daffie, alexpott: Select...

  • alexpott committed b9b6fa58 on 9.5.x 
    Issue #3191623 by mondrake, Medha Kumari, daffie, alexpott: Select...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I'm removing credit for #12 since there was already a valid MR here and no need for a re-rolled patch.