propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 399 forks source link

Bugfix/column names from subquery #1714

Closed mringler closed 3 years ago

mringler commented 3 years ago

Trying to get ModelCriteria::addSelectQuery(): working.

Found several issues:

This addresses subquery column name replacement in outer query.

Problem example:

$numberOfBooksQuery = BookQuery::create()
->addAsColumn('NumberOfBooks', 'COUNT(Book.Id)')
->select(['NumberOfBooks', 'AuthorId'])
->groupBy('Book.AuthorId');

AuthorQuery::create()
->addSelectQuery($numberOfBooksQuery, 'numberOfBooks', false)
->where('Author.Id = numberOfBooks.AuthorId')     // note that 'AuthorId' is an alias from the subquery
->withColumn('numberOfBooks.NumberOfBooks', 'NumberOfBooks');

leads to query

SELECT ... 
FROM author, (
  SELECT
    COUNT(book.id) AS NumberOfBooks,
    book.author_id AS "AuthorId" 
  FROM book
  GROUP BY book.author_id
) AS numberOfBooks 
WHERE author.id = numberOfBooks.author_id;     -- column author_id does not exist on subquery 

The error is quite simple: When replacing the column literal from a subquery, the column map was also set, which causes the column to be treated as a column belonging to the outer table. Not setting the column map resolves the issue.

Note that the whole column name replacement is very smelly. Look at what happens with the currentAlias class variable if you want to get a whiff. I initially cleaned this up, but realistically, a large commit like this is not going to get reviewed properly, so boiling it down seems like the best option. During refactoring, I found that tests for name replacement were scattered across three files. That cleanup I kept, because it is probably helpful for others. It is in its own commit, should you want to review it separately.

Oh, I have cleaned up two other minor, yet pretty stunning semantic mistakes that I found:

I'll highlight them in the review, so it does not confuse anyone. Btw. I think theese mistakes highlight a larger issue with the Criteria classes, where it is not clear from their name what they do (what`s a "Critera", particularly in contrast to a "Criterion", and why does it create sql code?), and where extension hierarchy was abused to decompose the classes, leading to arbitrary logical splits between them. This has apparently and understandably left people confused what goes where, including me.

dereuromark commented 3 years ago

Shall we move forward? How can we resolve the open comments?

mringler commented 3 years ago

Shall we move forward? How can we resolve the open comments?

Marked my walkthrough comments as resolved.