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

Alias relations in search order. #462

Closed
wants to merge 5 commits into from
Closed

Alias relations in search order. #462

wants to merge 5 commits into from

Conversation

jwg4
Copy link

@jwg4 jwg4 commented Aug 25, 2015

The function sqlalchemy.orm.aliased() creates an aliased version of a model for use in joins, ie "JOIN foo ON foo.id = bar.foo_id" is replaced by "JOIN foo AS foo1 ON foo1.id = bar.foo_id". Aliasing every join in search order is the easiest way of making sure that there is no conflict between two orders on the same model.

This patch fixes the unit test broken on master, tests.test_search.TestQueryCreation.test_order_by_two_different_fields_of_the_same_relation

jwg4 added 5 commits December 10, 2015 23:13
These lines were put in place to fix a bug with Flask-SQLAlchemy adding listeners in a way which causes problems for a pure SQLAlchemy session. It seems that this is fixed in version 2.0 of Flask-SQLAlchemy, and the fix is now a bug. Adding the condition with "contains" means that this should now work on both older and newer version of Flask-SA (although it is untested on older versions).
The methods not existing makes this function fail even if we dont try to remove them. Ignore AttributeError here.
The function sqlalchemy.orm.aliased() creates an aliased version of a model for use in joins, ie "JOIN foo ON foo.id = bar.foo_id" is replaced by "JOIN foo AS foo1 ON foo1.id = bar.foo_id". Aliasing every join in search order is the easiest way of making sure that there is no conflict between two orders on the same model.
@jfinkels
Copy link
Owner

jfinkels commented Feb 4, 2016

Please check if this is still an issue with the development version, and if so, create a new pull request with a test case demonstrating the erroneous behavior.

@jfinkels jfinkels closed this Feb 4, 2016
@jwg4
Copy link
Author

jwg4 commented Feb 15, 2016

Thanks for all your work on the new version, but I feel the way this has been handled wasn't very respectful of other collaborators' time. For months and months lots of pull requests were completely ignored, including ones fixing bugs which were important to users, and a fix to one bug which broke the tests, making development progress harder for everyone. During this time lots of people took the time to make contributions, and heard nothing at all back. Now all this work has been made obsolete all of a sudden, and you are expecting people to update their tests, create new patches, and resubmit.
I understand that it's difficult to keep integrating contributions against a moving target of a new version, even if they fix important bugs. But it would have been nice if sometime between August, when I first opened the PR, and December, when I made changes to it, you could have found the time to let me know that there was a new version in development, and no changes to the old version were likely to be accepted.

@jfinkels
Copy link
Owner

Hi, @jwg4! You are absolutely right, of course, and I apologize. I'm working on improving my communication. If you have any other concerns, please email me, my contact information is on the README page.

@jwg4
Copy link
Author

jwg4 commented Feb 15, 2016

Thanks for your courteous message @jfinkels. I appreciate all the work that you are putting in. Looking forward to figuring out how I can contribute to the new version of Flask-Restless!

@jfinkels
Copy link
Owner

I have added the fix you suggested here in commit 64a5711. Thank you, and sorry again for the delay!

@jfinkels jfinkels added the bug label Mar 16, 2016
jfinkels added a commit that referenced this pull request May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants