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

Query fix #281

Merged
merged 2 commits into from
Feb 11, 2020
Merged

Query fix #281

merged 2 commits into from
Feb 11, 2020

Conversation

qxzkjp
Copy link

@qxzkjp qxzkjp commented Sep 2, 2019

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
Copy link
Member

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

@qxzkjp
Copy link
Author

qxzkjp commented Sep 19, 2019

@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
Copy link
Member

@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
Copy link

odahcam commented Feb 11, 2020

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.

@adrorocker adrorocker changed the base branch from 5.6 to pr.281 February 11, 2020 17:00
@adrorocker adrorocker merged commit 07e67be into analogueorm:pr.281 Feb 11, 2020
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