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

Bugfix: table alias and regular column name leads to cross join #1704

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

mringler
Copy link
Contributor

@mringler mringler commented Mar 1, 2021

Ok, so this drove me nuts for quite a while. When using aliases and regular column names (i.e. a.name instead of a.Name), queries broke horribly, as Propel inserted these columns with a cross join.

Example:

$c = new ModelCriteria('bookstore', 'Propel\Tests\Bookstore\Book');
$c->setModelAlias('b', true);
$c->where('b.title = ?', 'foo');    // note: using title, not Title
$c->join('b.Author a');
$c->where('a.first_name = ?', 'john'); // note: using first_name, not FirstName

led to query

SELECT  
FROM book b 
CROSS JOIN book     -- nooooooo
CROSS JOIN author   -- noooooooooooooo
INNER JOIN author a ON (b.author_id=a.id) 
WHERE b.title = :p1 AND a.first_name = :p2'

Same happened with regular query objects.

Turns out, around line 2100 in ModelCriteria.php, where column literals are resolved, checking if the aliased table name should be used was only done with phpNames, not normal names. This lead to the table being added twice to the from clause, once aliased and once normally, the latter without a join condition, causing the cross join.

Test cases only checked for phpNames, so it wasn't detected.

I kept running into this for quite a while, so I am happy to squash that bugger. Please, please make it go away for good.

Copy link
Contributor Author

@mringler mringler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

short run through the changes, if it helps

@mringler mringler changed the title fixed bug: table alias and regular column name leads to cross join Bug: table alias and regular column name leads to cross join Mar 2, 2021
@mringler mringler changed the title Bug: table alias and regular column name leads to cross join Bugfix: table alias and regular column name leads to cross join Mar 2, 2021
@codecov-io
Copy link

codecov-io commented Mar 4, 2021

Codecov Report

Merging #1704 (0f16793) into master (16292d3) will decrease coverage by 2.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1704      +/-   ##
============================================
- Coverage     88.24%   85.54%   -2.70%     
- Complexity     7704     7707       +3     
============================================
  Files           261      261              
  Lines         22084    22088       +4     
============================================
- Hits          19488    18896     -592     
- Misses         2596     3192     +596     
Flag Coverage Δ Complexity Δ
5-max 85.54% <100.00%> (-2.70%) 0.00 <19.00> (ø)
7.4 85.54% <100.00%> (-2.70%) 0.00 <19.00> (ø)
agnostic 73.58% <84.61%> (-0.01%) 0.00 <19.00> (ø)
mysql 70.23% <100.00%> (+<0.01%) 0.00 <19.00> (ø)
pgsql ? ?
sqlite ? ?

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

Impacted Files Coverage Δ Complexity Δ
src/Propel/Runtime/ActiveQuery/Criteria.php 89.51% <ø> (-0.34%) 348.00 <0.00> (ø)
src/Propel/Runtime/ActiveQuery/ModelCriteria.php 94.84% <100.00%> (-0.03%) 281.00 <15.00> (-1.00)
src/Propel/Runtime/Map/TableMap.php 92.06% <100.00%> (-0.76%) 78.00 <4.00> (+4.00) ⬇️
...rc/Propel/Generator/Reverse/SqliteSchemaParser.php 0.00% <0.00%> (-89.56%) 49.00% <0.00%> (ø%)
src/Propel/Generator/Reverse/PgsqlSchemaParser.php 0.00% <0.00%> (-87.21%) 71.00% <0.00%> (ø%)
src/Propel/Runtime/Adapter/Pdo/PgsqlAdapter.php 2.59% <0.00%> (-55.85%) 34.00% <0.00%> (ø%)
src/Propel/Generator/Platform/SqlitePlatform.php 57.61% <0.00%> (-36.20%) 92.00% <0.00%> (ø%)
src/Propel/Runtime/Adapter/Pdo/SqliteAdapter.php 41.66% <0.00%> (-20.84%) 13.00% <0.00%> (ø%)
src/Propel/Runtime/DataFetcher/PDODataFetcher.php 60.86% <0.00%> (-19.57%) 23.00% <0.00%> (ø%)
src/Propel/Generator/Platform/PgsqlPlatform.php 82.50% <0.00%> (-15.00%) 123.00% <0.00%> (ø%)
... and 12 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 16292d3...0f16793. Read the comment docs.

@dereuromark dereuromark merged commit 5301987 into propelorm:master Apr 9, 2021
@mringler
Copy link
Contributor Author

mringler commented Apr 9, 2021

@stereomon Thanks for the review/approval!

@dereuromark Thank you for merging!

What's up with Codecov recently? According to the page, this PR reduces coverage by 0.03% in one file, because I removed more lines than I added. Fair enough. But looking at the report, I apparently screwed up coverage across the whole project, mostly in unrelated files, like SqliteSchemaParser. Saw the same behavior in another PR.
Looking at the list of commits in the project page, there are lots of commits marked with unable to find commit on github, including this PR's base commit (see here). Maybe related?

@mringler mringler deleted the fix_wrong_alias branch April 9, 2021 09:25
@dereuromark
Copy link
Contributor

Maybe some codecov config is set wrongly?

@mringler
Copy link
Contributor Author

mringler commented Apr 9, 2021

Maybe. Though googling "codecov unable to find commit on github waiting for ci" finds Codecov support tickets, that don't indicate user errors.

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

Successfully merging this pull request may close these issues.

4 participants