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

Adds model.query documentation and allows model.query to be a callable #270

Closed
wants to merge 6 commits into from

Conversation

@andir
Copy link
Contributor

@andir andir commented Dec 30, 2013

Documentation on model.query was missing so I decided to write at least a short
paragraph about it. The callable functionally is required in one of my projects
so I can filter the model ressources using a rather "complex" SQLAlchemy query
with multiple joins which can't be easily translated into the search_params-"Language".

andir added 2 commits Dec 30, 2013
Documentation on ``model.query`` was missing so I decided to write at least a short
paragraph about it. The callable functionally is required in one of my projects
so I can filter the model ressources using a rather "complex" SQLAlchemy query
with multiple joins which can't be easily translated into the search_params-"Language".
…by to provide wrong results.

The .filter_by() method which was being used by query_by_primary_key always filters
on the table the query was last joined onto.

A query of the format session.query(Table1).join(Table2).filter_by(**{pk_name: pk_value})
would filter on the PK of Table2 where instead Table1 would've be required.

With this fix .filter(Table1.PK == primary_key_value) is being used to workaround that issue.
@@ -448,7 +457,7 @@ def query_by_primary_key(session, model, primary_key_value):
# force unicode primary key name to string; see unicode_keys_to_strings
pk_name = str(primary_key_name(model))
query = session_query(session, model)
return query.filter_by(**{pk_name: primary_key_value})
return query.filter(getattr(model, pk_name) == primary_key_value)
Copy link
Owner

@jfinkels jfinkels Jan 11, 2014

Choose a reason for hiding this comment

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

Why is this necessary?

Loading

Copy link
Contributor Author

@andir andir Jan 11, 2014

Choose a reason for hiding this comment

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

With a query like q = session.query(model).join(someOtherModel), q.filter_by(id=someId) would filter using the someOtherModel.id field not the original model.id field. (filter_by always applies to the last joined model)

If you'd access a resource /model/123 which is made from a query containing a join statement you'd end up with the wrong response.

Loading

Copy link
Owner

@jfinkels jfinkels Jan 11, 2014

Choose a reason for hiding this comment

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

Makes sense, thanks.

Loading

@jfinkels
Copy link
Owner

@jfinkels jfinkels commented Jan 11, 2014

I am not opposed to this. However, you need better tests. Some of the tests you gave are testing implementation, not behavior. Some of the tests were redundant, that is, they were testing old functionality for which tests already exist as opposed to the newly introduced functionality. Finally, your main test does not take advantage of the callable query attribute in a meaningful way.

I have pushed a branch that has my changes to your pull request at master...callable_query_attribute. Before I pull this in, though, I would like to see a test that 1) modifies an existing model class like Planet or Star to add a callable query attribute that includes a join, 2) is placed in test_views.TestAPI and is similar to the other tests (of behavior) there, and 3) demonstrates how the join makes the ultimate HTTP request/response different.

Loading

@andir
Copy link
Contributor Author

@andir andir commented Jan 11, 2014

I've implemented something similar to what you suggested. Instead of Planet/Star I went with CarModel/CarManuf. since that did make more sens in context with a join.
Please leave comments if you need further changes.

Loading

@jfinkels
Copy link
Owner

@jfinkels jfinkels commented Mar 13, 2014

This is merged. Thanks!

Loading

@jfinkels jfinkels closed this Mar 13, 2014
@andir andir deleted the callable_query_attribute branch Mar 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants