Allow optional message on 404 from db query #85

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@robcowie

description arg on .get_or_404() and .first_or_404() is passed to abort, which in turn passes it to the NotFound exception.

For example, the following will return a 404 response with 'Unknown User' in it's body.

User.query.get_or_404('Unknown user')
@robcowie robcowie Allow optional message on 404 from db query
`description` arg on .get_or_404() and .first_or_404() is passed to abort, which in turn passes it to the NotFound exception.
68a8965
@robcowie
robcowie commented Jan 8, 2013

💤

@immunda
Collaborator
immunda commented Jun 9, 2015

Rather a specific use case, I don't think this belongs in the lib.

@immunda immunda closed this Jun 9, 2015
@robcowie
robcowie commented Jun 9, 2015

That was a long wait for rejection 😁

@immunda
Collaborator
immunda commented Jun 9, 2015

Sorry man, I hope this gave you closure. 🙏

@davidism
Collaborator
davidism commented Jun 9, 2015

@immunda I kind of like that this mirrors how abort(404) works. It's not a big change, but the alternative to this is overriding Query, which is not trivial right now, or catching the 404 and re-raising.

@immunda
Collaborator
immunda commented Jun 9, 2015

Maybe I'm biased as I've never really made use of the or_404 helpers 😉

A more flexible get_or_abort, which was basically just a proxy to the actual abort would be handy. For API compatibility, we could retain the or_404 methods as shortcuts to get_or_abort(404, ...)

So, how about this plan @davidism?

  • Merge this as is (@robcowie could you rebase this to master and ideally make a note in CHANGES?)
  • Bring in get_or_abort which is a pretty transparent proxy for abort as part of v3
  • Also eat get_or_abort dogfood for or_404 methods in v3
@davidism
Collaborator
davidism commented Jun 9, 2015

That sounds good. I'm not sure how "correct" an error besides 404 is for a missing resource, but I'm sure I've seen questions regarding that use case before.

@immunda
Collaborator
immunda commented Jun 9, 2015

One that springs to mind would be querying for a user by some credential and returning a 401 or 403 when that fails.

@bastianh

I don't think that query is the right place for get / get_or_404 in the first place.
I suggest creating a function in your class for that (using a mixin) like CRUDmixin.

@davidism
Collaborator

@bastianh No way, this is way too common and convenient to move somewhere else. Also, get is a method on SQLAlchemy's Query, so if you don't think it shouldn't be there you should file a bug with SQLAlchemy. Adding extra methods to Query is a standard practice, get_or_404 just mirrors what's already there.

@bastianh

I don't suggest removing the feature .. I just don't think that this feature should get extended even more to handle every possibility with different models.

If a model need special treatment (because it's a user model for example) it is quite easy to add such a function to the model. There is no need to alter the library or override query.

I think it's important for the library to do things sqlalchemy way.. like encouraging using a raw declarative_base of sqlalchemy and making it compatible with other sqlalchemy addon libraries instead of adding new functions that makes it even more incompatible. It should not alter the way you use sqlalchemy or make it easier or whatever...

class AbortMixin(object):
    @classmethod
    def get_or_abort(cls, id, code=404, reason=None):
        obj = db.session.query(cls).get(id)
        if not obj:
            abort(code, reason)
        return obj

This simple snippet will allow what the user wants.. you can simply add it to every model if you want and it will also work with plain sqlalchemy without bloating the library with a function for every possible usecase, changing the documentation and writing new unit tests.

@davidism
Collaborator

Adding methods to query is the "SQLAlchemy way", that's why SQLAlchemy exposes the query_cls argument to Session. Adding that mixin to every model is way less convenient and also requires every developer to know how and remember to add it every time. The point is not to "add a method to cover every use case" (we're not adding any methods in this pr), it's to cover the common use case which is: using Flask-SQLAlchemy, not raw SQLAlchemy, and using it for web related tasks (this mirrors Django's similar function. I'd love to chat about this more, but I don't think this is the right place for that.

@bastianh

I don't think this is a chat... it's a discussion about wether it is necessary or reasonable to add specific functionality to this library.

I think that the django example is supporting my case ... they don't have added that shortcut to the query class.. you have to import it seperatly from django.shortcuts.

Adding the functionality as a mixin was just an example. You could also adding it to your basemodel or make it a function you import from a shortcut class like django

The point is not to "add a method to cover every use case" (we're not adding any methods in this pr)

no .. but there were also comments in this pr where this is heading in the future (v3)

the base functionality is there and I don't think it is necessary to cover every case. last week I needed a get_or_404 function for a rest api that returns a json object with status code 404 so I wrote this function and added it to my project. Of couse I could have made a pull request for a get_or_404(id, as_json) but I think that is something to specific to add to to library.

@davidism
Collaborator

It's great that you came up with a solution for your JSON api. Given that you would have had to do that whether this function existed on query or not, I'm not clear what your objection is. The only thing your suggestion does is create more work for everyone who's not writing a custom 404 JSON response. Actually, this pr solves your use case as well, you could make the description the JSON data.

@justanr
Contributor
justanr commented Jun 11, 2015

I think a more general get_or and get_or_raise pair would be more approriate, but that's just me. I tend to avoid get_or_404 just because I don't want the ORM interfering with my routing and error handling. However, once #319 is hashed out, everyone can be satisfied.

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