load_and_authorize_resource loads the resource twice

[seaneshbaugh]

I'm not sure if this is intentional or not, but I suspect it isn't: when using load_and_authorize_resource the requested resource will be fetched two times. In the case of models backed by a database this is certainly undesirable.

load_and_authorize_resource calls authorize_resource which, in the case of actions like :show calls fetch_resource which loads the model. Then load_if_authorized is called. Assuming the current_user was authorized load_resource is called. In the case of actions like :show fetch_resource is called again.

Additionally, even when just using authorize_resource by itself, for actions like :show you still end up with a double load if you later load the model yourself in (for example) your controller. authorize_resource doesn't put the resource in the assigns so there's no way to reuse what was just loaded.

I can think of several ways around this. One might be storing the fetched resource in conn assigns. The documentation alludes to a fetched_resource assign, but nowhere is that found in the code itself. If you put the fetched resource there you could then just copy it to loaded_resource.

I'd do a pull request, but I'm not entirely sure this isn't the desired behavior.

[cpjk]

Hey, thanks for pointing this out and thanks for using canary!

I'm aware of this issue and had planned to fix it soon, but I figured that it would not be a significant issue for the first few minor versions of the package, so I had focused on fleshing out the API.

The references to conn.assigns.fetched_resource in the README were actually a mistake. This was the old name for conn.assigns.loaded_resource. Thanks for pointing that out, and sorry for the confusion! I've just pushed up a change to the README.

I agree that storing the fetched resource in the Conn for use later in the request would be a good solution. However, I'm wary of keeping the fetched resource in conn.assigns once the caller has access to it, since in phoenix apps, variables in conn.assigns are automatically made available in templates, which could pollute that namespace if the user did not intend to load the resource e.g. if they only called authorize_resource.

I think we could either store the fetched resource in conn.assigns.fetched_resource and remove it again before returning the Conn to the caller or use a different storage place in the Conn that would be less obtrusive to the user.

Let me know what you think.

I'm in class at the moment, but when I have a couple more minutes, I'll look at the options.

[seaneshbaugh]

I definitely agree that keeping the fetched_resource around is probably a not so great idea.

Putting it in conn.assigns and then taking out out again sounds like a good idea. But I'm not sure how that could be done since authorize_resource can be called on its own.

What if in load_and_authorize_resource load_resource was always called first? Then you could skip the fetch in authorize_resource if the resource is already in the conn.assigns. And then conn.assigns.loaded_resource could be set to nil if the user isn't authorized.

[cpjk]

That sounds like a great idea!

I'll try to bang something out tomorrow.

[cpjk]

I've pushed up some preliminary changes : https://github.com/cpjk/canary/commit/05facd7f52bdc45f6ddc119d8d4c16b19490deec to a dev branch in case you want to check them out.

There are no tests yet though.

[seaneshbaugh]

The changes look good to me!

I just tested it out in my application and the double hit to the database is, as expected, gone.

[cpjk]

Thanks!

Changes are now on master. :+1:

Let me know if you have any other issues!