Feature/untangle criteria class #1747

mringler · 2021-06-04T12:33:38Z

I was in the mood for some refactoring, and decided it is time to go for the white whale, aka the Criteria classes.

I believe those are Propel's achilles heel, since they are the center classes of ActiveQuery and parent class to all Query classes, and completely unmaintainable - with together almost 6000 lines of code, a fuzzy extension hierarchy, and them doing literally dozens of things at once.
In practice, that is acceptable, as long as everything works perfectly. But almost all bugs I have fixed were in those three classes, and I am pretty sure there are more. Also, I am pretty sure that a lot of performance is lost by hacky workarounds, of which I have seen plenty. I think those bugs and workarounds show that the information flow in those classes is not understandable anymore.

So here is a proposal to start getting things back in order by removing sql query generation and execution out of those classes. This already removes more than 500 lines of code.

The basic idea is to have the doSelect(), doInsert(), do...() methods delegate to dedicated classes. Here is an overview of the new classes (not sure if the tree output renders in all browsers):

ActiveQuery
├── BaseModelCriteria.php
├── Criteria.php
├── ModelCriteria.php
├── ...
├── QueryExecutor
│   ├── AbstractQueryExecutor.php
│   ├── CountQueryExecutor.php
│   ├── DeleteAllQueryExecutor.php
│   ├── DeleteQueryExecutor.php
│   ├── InsertQueryExecutor.php
│   ├── QueryExecutionException.php
│   ├── SelectQueryExecutor.php
│   └── UpdateQueryExecutor.php
└── SqlBuilder
    ├── AbstractSqlQueryBuilder.php
    ├── CountQuerySqlBuilder.php
    ├── DeleteQuerySqlBuilder.php
    ├── InsertQuerySqlBuilder.php
    ├── SelectQuerySqlBuilder.php
    └── UpdateQuerySqlBuilder.php

The changes are very thoroughly tested through the Criteria classes and a few new tests.

mringler · 2021-06-04T12:41:35Z

Hmm, tests fail before they even start:

Tests started in temp /tmp.
Could not read "tests/sqlite.phpunit.xml".
Error: Process completed with exit code 2.

I know it is what everybody says, but I don't see how this is my fault. I have not changed the phpunit.xml files.

Please let me know how to proceed.

codecov-commenter · 2021-06-10T14:48:14Z

Codecov Report

Merging #1747 (a70d586) into master (a8f3afa) will increase coverage by 0.03%.
The diff coverage is 94.31%.

@@             Coverage Diff              @@
##             master    #1747      +/-   ##
============================================
+ Coverage     87.89%   87.93%   +0.03%     
- Complexity     7827     7870      +43     
============================================
  Files           267      281      +14     
  Lines         22546    22670     +124     
============================================
+ Hits          19817    19934     +117     
- Misses         2729     2736       +7

Flag	Coverage Δ
5-max	`87.93% <94.31%> (+0.03%)`	⬆️
7.4	`87.93% <94.31%> (+0.03%)`	⬆️
agnostic	`67.44% <71.76%> (-5.91%)`	⬇️
mysql	`70.38% <92.71%> (+0.19%)`	⬆️
pgsql	`70.33% <93.50%> (+0.20%)`	⬆️
sqlite	`68.30% <91.33%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...untime/ActiveQuery/Criterion/AbstractCriterion.php	`89.10% <ø> (ø)`
.../ActiveQuery/QueryExecutor/InsertQueryExecutor.php	`81.25% <81.25%> (ø)`
...tiveQuery/QueryExecutor/DeleteAllQueryExecutor.php	`83.33% <83.33%> (ø)`
...me/ActiveQuery/SqlBuilder/CountQuerySqlBuilder.php	`84.00% <84.00%> (ø)`
src/Propel/Runtime/ActiveQuery/ModelCriteria.php	`95.31% <87.50%> (+0.48%)`	⬆️
src/Propel/Runtime/Adapter/Pdo/PdoAdapter.php	`90.96% <92.30%> (-0.47%)`	⬇️
.../ActiveQuery/QueryExecutor/UpdateQueryExecutor.php	`94.44% <94.44%> (ø)`
...e/ActiveQuery/SqlBuilder/UpdateQuerySqlBuilder.php	`94.64% <94.64%> (ø)`
...e/ActiveQuery/SqlBuilder/SelectQuerySqlBuilder.php	`94.66% <94.66%> (ø)`
...e/ActiveQuery/SqlBuilder/InsertQuerySqlBuilder.php	`96.15% <96.15%> (ø)`
... and 31 more

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 a8f3afa...a70d586. Read the comment docs.

mringler · 2021-06-10T15:39:25Z

Added test fixed from #1751. I'll remove them and rebase when that is resolved.

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

src/Propel/Runtime/Adapter/SqlAdapterInterface.php

src/Propel/Runtime/ActiveQuery/SqlBuilder/SelectQuerySqlBuilder.php

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

src/Propel/Runtime/ActiveQuery/QueryExecutor/CountQueryExecutor.php

src/Propel/Runtime/ActiveQuery/QueryExecutor/InsertQueryExecutor.php

src/Propel/Runtime/ActiveQuery/SqlBuilder/CountQuerySqlBuilder.php

dereuromark · 2021-08-09T13:53:57Z

Small conflict btw.

mringler · 2021-08-10T09:41:54Z

This is probably not going to get merged, so I am just going to close the PR.
Should you actually want to see this merged, let me know, and I'll fix the conflict and re-open.

dereuromark · 2021-08-10T09:43:09Z

From what I can see it cleans up the baseline quite a bit
So it seems worth finishing this up IMO.

mringler · 2021-08-10T10:03:51Z

Great, I have fixed the conflict

src/Propel/Runtime/ActiveQuery/SqlBuilder/InsertQuerySqlBuilder.php

src/Propel/Runtime/ActiveQuery/SqlBuilder/DeleteQuerySqlBuilder.php

src/Propel/Runtime/ActiveQuery/QueryExecutor/InsertQueryExecutor.php

dereuromark · 2021-08-10T10:15:44Z

@nederdirk can also maybe help with the review for this to be merged today?

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

nederdirk

I'm not done yet, but it's lunch time for me now 😄

src/Propel/Runtime/ActiveQuery/ModelCriteria.php

nederdirk · 2021-08-10T10:34:06Z

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

+use Propel\Runtime\ServiceContainer\ServiceContainerInterface;
+use Throwable;
+
+abstract class AbstractQueryExecutor


This may be a good opportunity to add comments about 'what is the structure of the subsystem these classes represent', i.e.

`QueryExecutor`'s responsibility is to translate a `Criteria` object to the specific RDBMS dialect and execute the query. Use it like this: <insert example here>

Comments can also be added later if we don't have time now, as they are not changing the functional parts.

nederdirk · 2021-08-10T10:38:10Z

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

+     * @param \Propel\Runtime\ActiveQuery\Criteria $criteria
+     * @param \Propel\Runtime\Connection\ConnectionInterface|null $con
+     */
+    public function __construct(Criteria $criteria, ?ConnectionInterface $con = null)


with abstract classes, it might be good to think about whether the constructor should be final: given a class-string<AbstractQueryExecutor> $className, should users of the Propel library be able to do new $className($criteria, $connection);?

If concrete implementing classes (i.e. extends AbstractQueryExecutors) should be able to override the constructor, consider having AbstractQueryExecutor implement an interface ConstructableQueryExecutor { function __construct(Criteria $criteria, ?ConnectionInterface $con = null); }, so implementers are bound to that interface, too.

I am not sure, what's the advantage here?

We could probably rediscuss this also later. Right now I am a bit concerned about the time line as this PR needs to land today for us to go into the release phase.

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php

gechetspr

Few minor things. Not sure if they are blocker, but IMO nice to fix

src/Propel/Runtime/ActiveQuery/Criteria.php

src/Propel/Runtime/Adapter/Pdo/MssqlAdapter.php

src/Propel/Runtime/Adapter/Pdo/PdoAdapter.php

dereuromark · 2021-08-11T15:06:42Z

I see 1-2 important comments, otherwise we can probably continue for the beta release.
Lets see if we can merge this today then.

dereuromark · 2021-08-11T15:55:11Z

This one fail

$DB_USER not set. Using 'root'.
/usr/bin/mysql Ver 14.14 Distrib 5.7.35, for Linux (x86_64) using EditLine wrapper

looks unrelated

dereuromark · 2021-08-11T15:55:39Z

Should this be a squash merge?

mringler · 2021-08-11T15:56:26Z

One of the mysql tests always fails, but I think it is a problem with the pipeline (it's a different one each time)
https://github.com/propelorm/Propel2/pull/1747/checks?check_run_id=3302733292
https://github.com/propelorm/Propel2/runs/3302662191?check_suite_focus=true

mringler · 2021-08-11T15:58:10Z

Should this be a squash merge?

I'd prefer it

mringler mentioned this pull request Jun 5, 2021

fix tests #1748

Merged

mringler force-pushed the feature/untangle_criteria_class branch 2 times, most recently from b4bcf15 to 60889bc Compare June 10, 2021 14:45

mringler force-pushed the feature/untangle_criteria_class branch 2 times, most recently from b263a3e to a723dc3 Compare June 10, 2021 15:33

dmytro-dymarchuk reviewed Jun 14, 2021

View reviewed changes

mringler force-pushed the feature/untangle_criteria_class branch from a723dc3 to b2f0f0a Compare June 14, 2021 10:20

mringler added 3 commits June 15, 2021 08:12

Fixed bug when resolving nested query columns

1bd5d30

removed query building and execution from Criteria class

dbdcff7

review changes

8c06fca

mringler force-pushed the feature/untangle_criteria_class branch from 105c69f to 8c06fca Compare June 15, 2021 06:12

dereuromark added the Improvement label Jun 22, 2021

mringler closed this Aug 10, 2021

mringler added 2 commits August 10, 2021 11:49

Merge branch 'master' into feature/untangle_criteria_class

905f28c

Merge branch 'master' into feature/untangle_criteria_class

27480b7

mringler reopened this Aug 10, 2021

dereuromark reviewed Aug 10, 2021

View reviewed changes

src/Propel/Runtime/ActiveQuery/SqlBuilder/InsertQuerySqlBuilder.php Outdated Show resolved Hide resolved

dereuromark reviewed Aug 10, 2021

View reviewed changes

src/Propel/Runtime/ActiveQuery/SqlBuilder/DeleteQuerySqlBuilder.php Outdated Show resolved Hide resolved

dereuromark reviewed Aug 10, 2021

View reviewed changes

src/Propel/Runtime/ActiveQuery/QueryExecutor/InsertQueryExecutor.php Outdated Show resolved Hide resolved

dereuromark reviewed Aug 10, 2021

View reviewed changes

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php Outdated Show resolved Hide resolved

refined return types

80322bc

mringler force-pushed the feature/untangle_criteria_class branch from 81b74e2 to 46c4303 Compare August 10, 2021 10:31

mringler force-pushed the feature/untangle_criteria_class branch from 46c4303 to 2aa9adc Compare August 10, 2021 10:32

nederdirk reviewed Aug 10, 2021

View reviewed changes

mringler force-pushed the feature/untangle_criteria_class branch from 2aa9adc to 4fc2186 Compare August 10, 2021 11:03

dereuromark reviewed Aug 10, 2021

View reviewed changes

src/Propel/Runtime/ActiveQuery/QueryExecutor/AbstractQueryExecutor.php Outdated Show resolved Hide resolved

mringler force-pushed the feature/untangle_criteria_class branch 3 times, most recently from 371c49e to 0fd9718 Compare August 10, 2021 11:25

gechetspr reviewed Aug 11, 2021

View reviewed changes

mringler force-pushed the feature/untangle_criteria_class branch from 0fd9718 to 313fac4 Compare August 11, 2021 15:43

dereuromark approved these changes Aug 11, 2021

View reviewed changes

mringler force-pushed the feature/untangle_criteria_class branch from 313fac4 to a70d586 Compare August 11, 2021 15:50

gechetspr approved these changes Aug 11, 2021

View reviewed changes

review changes

364ae31

mringler force-pushed the feature/untangle_criteria_class branch from a70d586 to 364ae31 Compare August 11, 2021 16:00

dereuromark merged commit faf29e2 into propelorm:master Aug 11, 2021

emicks mentioned this pull request Oct 8, 2021

BC break: QueryExecutionException instead of PropelException #1787

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/untangle criteria class #1747

Feature/untangle criteria class #1747

mringler commented Jun 4, 2021

mringler commented Jun 4, 2021

codecov-commenter commented Jun 10, 2021 •

edited

Loading

mringler commented Jun 10, 2021

dereuromark commented Aug 9, 2021

mringler commented Aug 10, 2021

dereuromark commented Aug 10, 2021

mringler commented Aug 10, 2021

dereuromark commented Aug 10, 2021

nederdirk left a comment

nederdirk Aug 10, 2021

dereuromark Aug 11, 2021

nederdirk Aug 10, 2021

mringler Aug 10, 2021

dereuromark Aug 11, 2021

gechetspr left a comment

dereuromark commented Aug 11, 2021

dereuromark commented Aug 11, 2021

dereuromark commented Aug 11, 2021

mringler commented Aug 11, 2021

mringler commented Aug 11, 2021 •

edited

Loading

Feature/untangle criteria class #1747

Feature/untangle criteria class #1747

Conversation

mringler commented Jun 4, 2021

mringler commented Jun 4, 2021

codecov-commenter commented Jun 10, 2021 • edited Loading

Codecov Report

mringler commented Jun 10, 2021

dereuromark commented Aug 9, 2021

mringler commented Aug 10, 2021

dereuromark commented Aug 10, 2021

mringler commented Aug 10, 2021

dereuromark commented Aug 10, 2021

nederdirk left a comment

Choose a reason for hiding this comment

nederdirk Aug 10, 2021

Choose a reason for hiding this comment

dereuromark Aug 11, 2021

Choose a reason for hiding this comment

nederdirk Aug 10, 2021

Choose a reason for hiding this comment

mringler Aug 10, 2021

Choose a reason for hiding this comment

dereuromark Aug 11, 2021

Choose a reason for hiding this comment

gechetspr left a comment

Choose a reason for hiding this comment

dereuromark commented Aug 11, 2021

dereuromark commented Aug 11, 2021

dereuromark commented Aug 11, 2021

mringler commented Aug 11, 2021

mringler commented Aug 11, 2021 • edited Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading

mringler commented Aug 11, 2021 •

edited

Loading