Allowing loading of resource on :new and :create actions.

[jordantdavis]

If someone fetches a resource when the action is :new or :create, then they get nil back every time.

If we have a nested resource such as:

/api/v1/questions/:question_id/answers

where questions are owned by users, then we can never load and/or authorize a resource on these creation actions.

Should we allow resources to be loaded on :new and :create actions so we can deal with nested resources? Thoughts?

[cpjk]

While it definitely feels like this is something we should support, the question is: how should a new resource be created, and what form should it take?

For the new action, it might be reasonable to allow the user to specify, as an opt, a function that returns a new resource/changeset. Canary would then call this function to load a new resource/changeset.

For the create action, I'm wondering if it really makes sense to load the resource. Maybe we could do a similar thing as for the new action and specify a function to which canary would pass conn.params[$model_name]. Or canary could simply create a new struct of the given model using the attributes in conn.params[$model_name.

Let me know what you think.

[jordantdavis]

Sorry, I realized I was not clear with my suggestion.

Let's say I have some routes that look like this:

web/router.ex

// other routes/pipelines

resources "/questions", QuestionController do
  resources "/answers", AnswerController
end

Question has a one-to-many relationship to Answers and is owned by a User. My aim was to use canary as follows:

web/controllers/answer_controller.ex

plug :load_and_authorize_resource, [model: Question, id_name: "question_id"] when action in [:create]

// other plugs

def create(conn, %{"answer" => answer_params}) do
  question = conn.assigns[:question]
  answer = Ecto.Model.build(question, :answers)
  changeset = Answer.changeset(answer, answer_params)

  Repo.insert(changeset)

  // other creation stuff
end

// other actions

I would load the Question when I hit /questions/:question_id/answers, authorize the current user to create a new Answer for the given Question, create the new Answer, and move on.

The only thing that keeps me from doing this is the return of nil instead of hitting the Repo whenever the action is either :create or :new. My suggestion is to allow loads when the action is either :create or :new. Does that clear things up?

[cpjk]

OK, I understand now. I'm looking into adding this now. It doesn't look like it will be a very big change. :+1:

[jordantdavis]

I am curious as to how this functionality would be added to the authorize_resource and load_and_authorize_resource plugs. I know you use the model name to authorize when it's an :index, :new, or :create action and the fetched model authorize in other situations. Would we be adding another option to the plug call or do you have another idea on this one?

[cpjk]

I was first thinking of just changing load_resource/2 so that for new and create actions, it will try to load the resource specified, but only if there is an id in conn.params.$id_name. I think the downside to this approach is that it still requires the user to ensure that conn.params.$id_name never contains an id for a non-persisted resource.

I think a better option might be to add :load_on_new and :load_on_create opts, and maybe also an :always_load opt or something similar to mean load on both new and create, to cut down on the wordiness.

What do you think?

[jordantdavis]

Let's assume the nested Answer resource looks like this:

get /questions/:question_id/answers, AnswersController, :index
get /questions/:question_id/answers/new, AnswersController, :new
post /questions/:question_id/answers, AnswersController, :create
get /answers/:id, AnswersController, :show
get /answers/:id/edit, AnswersController, :edit
put /answers/:id, AnswersController, :update
delete /answers/:id, AnswersController, :delete

The first solution would have the form:

load_and_authorize_resource [model: Question, id_name: "question_id"] when action in [:new, :create]
authorize_resource [model: Answer] when action in [:new, :create]
load_and_authorize_resource [model: Answer, preload: :question] when action in [:edit, :update, :delete]

The second solution would have the form:

load_and_authorize_resource [model: Question, id_name: "question_id", always_load: true] when action in [:new, :create]
authorize_resource [model: Answer] when action in [:new, :create]
load_and_authorize_resource [model: Answer, preload: :question] when action in [:edit, :update, :delete]

I like the second solution because it's more explicit. The combination of action being :new or :create and the id_name option being present in the first solution could get the job done, but I feel that declaring a model load as being a "parent" or "nested" may be the better solution here.

I'm thinking of something along these lines:

load_and_authorize_resource [model: Question, id_name: "question_id", parent: true] when action in [:new, :create]

What do you think about that?

[cpjk]

Maybe I'm reading this incorrectly, but my understanding is that the only difference between your solution and my second proposed solution is the replacement of :always_load with parent: true. Is that correct?

[jordantdavis]

That is correct. I don't know which option name makes more sense here, but I accidentally snuck :parent in there. Would this functionality be used in other situations besides when dealing with nested resources? If so we can use :always_load or something more general. If not then we could something specific like :nested, :parent, etc. Once we decide on a flag name then I can submit a pull request with this addition. Let me know what you think.

[cpjk]

I think that parent: true feels comfortable, but I would rather have the terminology be more general in this case. Also, I hate to change my mind again, but I think that persisted: true would best reflect that you are marking the resource as persisted, and therefore, Canary can safely try to load it from the db.

If you want to submit a PR, that would be awesome! :smiley:

[jordantdavis]

One more thing I ran into while adding the persisted option. Can we use the persisted option to override the default for :index actions (fetch_all)?

So we'd have something like this:

plug load_and_authorize_resource [model: Question, 
                                  id_name: "question_id", 
                                  persisted: true,
                                  preload: :answers] when action in [:index]

And it would load/authorize that Question resource, then preload its associated Answers.

What do you think? It's an easy addition on my end.

[cpjk]

Sorry I didn't respond to this more quickly. I think that behaviour makes sense.

[cuongtnx]

doesn't seem that this issue is resolved? any possible update @cpjk @jordantdavis?