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

Avoid double parenthesis when nesting functions and query expressions #11410

Merged
merged 2 commits into from Nov 6, 2017

Conversation

Projects
None yet
4 participants
@raul338
Contributor

raul338 commented Nov 6, 2017

For current behavior/examples see my post: COUNT DISTINCT using Query->newExpr()?
In short: the following code generates an invalid SQL (at least for mysql)

return $query->select([
    'required_approved' => $query->func()->count(
        $query->func()->DISTINCT(
            [
                $query->newExpr()->addCase(
                    (clone $baseExpr)
                        ->add($query->newExpr()->eq('DocumentTypes.required', 1))
                        ->add($query->newExpr()->eq('Documents.approved', 2)),
                    new IdentifierExpression('DocumentTypes.id')
                )
            ],
            ['literal'],
            ['integer']
        )
    ),
    // ...
);
SELECT (
    COUNT((  -- double parens breaking sql
        DISTINCT((
            CASE WHEN (
              DocumentTypes.entity_id = 1
              AND (DocumentTypes.id) IS NOT NULL
              AND IF(
                Providers.relation = 1, DocumentTypes.applyDep = 1,
                DocumentTypes.applyAuto = 1
              )
              AND DocumentTypes.required = 1
              AND Documents.approved = 1
            ) THEN DocumentTypes.id END
        ))
    ))
) AS `required_approved`,
-- ....
FROM ....

Then I thought that the double parenthesis are being generated somewhere, I found QueryExpression adds when it has more than one condition, and FunctionExpressions adds again even if QueryExpression is complex, resulting in two parenthesis

This PR attempts to fix that, I didn't know if split in two commits (first add a test and then the fix)

@markstory markstory added this to the 3.5.6 milestone Nov 6, 2017

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Nov 6, 2017

Member

Changes look reasonable, the tests are failing presently though.

Member

markstory commented Nov 6, 2017

Changes look reasonable, the tests are failing presently though.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 6, 2017

Codecov Report

Merging #11410 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11410      +/-   ##
============================================
- Coverage     93.12%   93.11%   -0.02%     
  Complexity    13007    13007              
============================================
  Files           436      436              
  Lines         33697    33697              
============================================
- Hits          31381    31376       -5     
- Misses         2316     2321       +5
Impacted Files Coverage Δ Complexity Δ
src/Database/Expression/FunctionExpression.php 97.87% <100%> (ø) 18 <0> (ø) ⬇️
src/Cache/CacheEngine.php 89.36% <0%> (-4.26%) 19% <0%> (ø)
src/Cache/CacheRegistry.php 96% <0%> (-4%) 11% <0%> (ø)
src/Cache/Engine/FileEngine.php 89.07% <0%> (-1.1%) 73% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27166a6...444f487. Read the comment docs.

codecov-io commented Nov 6, 2017

Codecov Report

Merging #11410 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11410      +/-   ##
============================================
- Coverage     93.12%   93.11%   -0.02%     
  Complexity    13007    13007              
============================================
  Files           436      436              
  Lines         33697    33697              
============================================
- Hits          31381    31376       -5     
- Misses         2316     2321       +5
Impacted Files Coverage Δ Complexity Δ
src/Database/Expression/FunctionExpression.php 97.87% <100%> (ø) 18 <0> (ø) ⬇️
src/Cache/CacheEngine.php 89.36% <0%> (-4.26%) 19% <0%> (ø)
src/Cache/CacheRegistry.php 96% <0%> (-4%) 11% <0%> (ø)
src/Cache/Engine/FileEngine.php 89.07% <0%> (-1.1%) 73% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27166a6...444f487. Read the comment docs.

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Nov 6, 2017

Member

I'm surprised that this didn't break more stuff. Thanks for fixing it!

Member

lorenzo commented Nov 6, 2017

I'm surprised that this didn't break more stuff. Thanks for fixing it!

@lorenzo lorenzo merged commit e6e3179 into cakephp:master Nov 6, 2017

5 checks passed

codecov/patch 100% of diff hit (target 93.12%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +6.87% compared to 27166a6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

@raul338 raul338 deleted the raul338:FunctionExprNestQueryExpr branch Nov 6, 2017

@raul338 raul338 referenced this pull request Apr 27, 2018

Closed

SQL cast using query builder #12018

1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment