search

analogueorm / analogue

Analogue ORM : Data Mapper ORM for Laravel/PHP
MIT License
634 stars 51 forks source link

Query fix #281

Closed qxzkjp closed 4 years ago

qxzkjp commented 4 years ago

The query object currently does not recognise the primary key as being present if it's in the form of other_column AS primary_key. This is not usually an issue, but if the query joins a table that has a column with the same name as the primary key (quite likely if the primary key is called id) then it becomes impossible to run the query without getting an "ambiguous column" error.

This makes it impossible to do sorting or filtering of objects by related tables, which was an inconvenience when we were using Analogue to get paginated lists of users.

This pull request changes the logic in Query::enforceIdColumn. It was checking for a column whose name was exactly equal to the primary key, and adds it if it was not found. It now checks for a column that ends with the primary key name, and then checks that it is a valid alias. The performance impact in the usual case (no aliasing) should be minimal.

I have also changed the automatically inserted id column from being primary_key to table.primary_key AS primary_key, to avoid the situation mentioned above happening with the automatically added identity column.

A test for this case has been added to QueryTest.

adrorocker commented 4 years ago

@qxzkjp Could you make all test to pass before I merge this?

qxzkjp commented 4 years ago

@adrorocker I don't think I can. The CI errors are not test failures, they look like the pipeline is not set up correctly. Something about the source directory having uncommitted changes.

adrorocker commented 4 years ago

@qxzkjp Sorry it took me so long to fixed the build. Can you merge 5.6 branch into your's and then I'll merge the PR.

Thanks!

odahcam commented 4 years ago

Merge should affect just two files. You could create a new branch from current master, merge this PR there, and then merge to master, so @qxzkjp wouldn't need to go back to this.