Add support for querying models by primary-key value #102

Closed
wants to merge 1 commit into
from

2 participants

@tmoertel

This (tiny!) patch allows you to query for a model instance by simply
providing its primary-key value as a query argument. You can use this
new convention anywhere Q objects are allowed, but the most-common
expected use case is this:

blog = Blog.get(1)  # get blog w/ primary key of 1

Rationale.

Why this patch? In short, because it simplifies one common use-case
and eliminates the need to break encapsulation for another.

Currently, to fetch a model instance when you know its primary-key
value, you must also know its primary-key name:

blog = Blog.get(id=1)  # relies on knowledge that Blog's pkey is "id"

Since the model already has this knowledge, why not just let the model
take care of it?

blog = Blog.get(1)

Moreover, when writing generic code that takes a model as a parameter,
you must pry into peewee's internals to learn what the model's primary
key is called. For example:

def get_required_web_model_instance(model, id):
    try:
        return model.get(**{model._meta.pk_name: id})  # naughty!
    except model.DoesNotExist:
        raise flask.abort(404)

If we let the model take care of the details, however, we need not
breach peewee's encapsulation barriers to write model-generic code:

def get_required_web_model_instance(model, id):
    try:
        return model.get(id)  # nice!
    except model.DoesNotExist:
        raise flask.abort(404)
Tom Moertel Add support for querying models by primary-key value
This patch allows you to query for a model instance by simply
providing its primary-key value as a query argument.  You can use this
new convention anywhere Q objects are allowed, but the most-common
expected use case is this:

    blog = Blog.get(1)  # get blog w/ primary key of 1

Rationale.

Why this patch?  In short, because it simplifies one common use-case
and eliminates the need to break encapsulation for another.

Currently, to fetch a model instance when you know its primary-key
value, you must also know its primary-key name:

    blog = Blog.get(id=1)  # relies on knowledge that Blog's pkey is "id"

Since the model already has this knowledge, why not just let the model
take care of it?

    blog = Blog.get(1)

Moreover, when writing generic code that takes a model as a parameter,
you must pry into peewee's internals to learn what the model's primary
key is called.  For example:

    def get_required_web_model_instance(model, id):
        try:
            return model.get(**{model._meta.pk_name: id})  # naughty!
        except model.DoesNotExist:
            raise flask.abort(404)

If we let the model take care of the details, however, we need not
breach peewee's encapsulation barriers to write model-generic code:

    def get_required_web_model_instance(model, id):
        try:
            return model.get(id)  # nice!
        except model.DoesNotExist:
            raise flask.abort(404)
43e5161
@coleifer
Owner

There is a small problem in the implementation, which is that it does not take into account non-integer primary keys. That could be worked around though. I agree with this patch from the point-of-view that it simplifies the API but am not sure if it is worth changing the behavior of the entire where method (which calls parseq) to allow "naked" values to be passed in and implicitly taken to express == <pk>. I would suggest, if you want this, to implement a get_pk method on Model.

@tmoertel

Thanks for your feedback.

Leaving aside for the moment non-integer primary keys, which I'll be happy to fix, let's assume that we add get_pk. Even if we have this method, won't we still need the equivalent for where filtering and the like?

For example, consider the following code, in which we want to get a blog entry given by an entry identifier, provided that the entry is owned by the blog that matches a given blog identifier and that the blog is owned by the currently logged in user:

class User(Model):
    """A person who can own blogs."""
    # more fields here

class Blog(Model):
    """A blog, which can own entries."""
    owner = ForeignKeyField(User)
    # more fields here

class Entry(Model):
    """A post to a blog."""
    blog = ForeignKeyField(Blog)
    # more fields here


def read_entry(user, blog_id, entry_id):
    """Handle request for /blog/<blog_id>/entry/<entry_id>.

    Args:
      user:  Authenticated User-model instance for the logged-in user.
      blog_id:  Blog identifier from URL.
      entry_id:  Entry identifier from URL.

    Returns:
      Rendered HTML.

    Raises:
      Abort(404):  if no such blog entry is visible to the user.

    """
    query = (Entry.select().where(id=entry_id)
             .join(Blog).where(id=blog_id, owner=user))
    try:
        entry = query.get()
    except Entry.DoesNotExist:
        raise Abort(404)
    return render("entry", entry=entry)

It's easy to write this kind of code for specific model instances, where you know the names of all the primary keys in advance. But for generic code that gets passed Entry-like models and Blog-like models and their respective instance identifiers at run time, you can't write the code (reliably) without having it pry into peewee's internals to find what the models' primary keys are called:

def read_general_entry(user, blog_model, blog_id, entry_model, entry_id):
    blog_pkname = blog_model._meta.pk_name
    entry_pkname = entry_model._meta.pk_name
    query = (Entry.select().where(**{entry_pkname: entry_id})
             .join(Blog).where(owner=user, **{blog_pkname: blog_id}))
    # the rest as before

Without something like the change I'm proposing, is there any clean way to solve problems like the above from user code?

As an alternative to "naked" identifier values, would you be more agreeable to adding a PK class that could be used wherever Q could? So you could write

entry = entry_model.get(PK(entry_id))
entry = entry_model.select().where(PK(entry_id).join(blog_model).where(PK(blog_id), user=user)

Thoughts?

@coleifer
Owner

While all of your suggestions have merit, I think I'm going to pass for now on adding something like this. Adding yet another method of querying to the (already large) available methods seems like a bad plan. "Syntactic sugar causes cancer of the semicolon".

@coleifer coleifer closed this Aug 1, 2012
@tmoertel

Thanks for your response.

Can I ask a favor? Given the current API, how would you recommend that I write model-generic code? (I can't figure out how to do it without prying into peewee's implementation internals (or requiring that all models use the same primary-key name). Am I missing something?)

@coleifer
Owner

If you take a look here, https://github.com/coleifer/flask-peewee/blob/master/flask_peewee/admin.py#L142

Basically, you're right:

def get_pk(klass, pk):
    return klass.get(**{klass._meta.pk_name: pk})
@tmoertel

What do you think about adding a public get_pk_name() method to models? It won't be super-convenient to use (**{klass.get_pk_name(): pk} doesn't exactly flow off the keyboard), but at least people won't have to make their code dependent upon peewee's internal implementation when they need knowledge of the primary key. That's the kind of thing that gets flagged at code reviews.

But maybe such a change would go against the grain of peewee's design? (My hunch about why there's not already something like get_pk_name() is that you wanted peewee to be smart enough that people wouldn't have to know the names of keys: during joins and other common operations, peewee would just take care of that stuff. I figured that leaving the method off was your way of ensuring that users would come to you with those cases where peewee's current design wasn't able to be fully transparent about keys, so you could consider ways of making it transparent for those cases. All of this is to say: I'll understand if you don't want to add such a method. I wouldn't.)

@coleifer
Owner

i've gone ahead and added get_pk_dict and get_pk_name methods to Model -- added in 04dace3 and 023d481

Hope this helps!

@tmoertel

Works for me :-)

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment