Creating comments under blog post - correctly building associations?

Phoenix Forum Questions / Help
,
1

Hi, I have very basic post has_many comments, and I am adding comments via a form on post show-view.

This is my create action in comment_controller:

def create(conn, %{"comment" => comment_params, "post_id" => post_id}) do

    changeset = %Comment{}
      |> Comment.changeset(comment_params)
      |> Ecto.Changeset.put_change(:post_id, String.to_integer(post_id))

    case Staticblog.Repo.insert(changeset) do
      {:ok, comment} ->
        conn
        |> put_flash(:info, "Comment created successfully.")
        |> redirect(to: Routes.post_path(conn, :show, post_id))

      {:error, %Ecto.Changeset{} = changeset} ->
        render(conn, "new.html", changeset: changeset)
    end
  end

the way I use the post_id from params and convert it from string to integer kinda smells to me…

previously I had this

    post = Blog.get_post!(post_id) # TODO is the database hit necessary here?
    changeset = post
      |> Ecto.build_assoc(:comments)
      |> Blog.change_comment(comment_params)

but I feel like fetching the post yet again here is redundant, isn’t it?

What is the proper way to do this? Can I somehow pass the whole post to the create action? Should I? Looking for best practice here, nothing fancy.

Thank you!

2

Could you just add :post_id to the cast in Comment.changeset and include post_id in the call to Comment.changeset? Let Ecto handle the type casts for you…

3

Thank you. Did you mean like this? Seems to work:

def create(conn, %{"comment" => comment_params, "post_id" => post_id}) do
    changeset = %Comment{}
      |> Comment.changeset(Map.put(comment_params, "post_id", post_id)) # this Map.put is ok, or is there nicer way? 
    ...
end

and

def changeset(comment, attrs) do
    comment
    |> cast(attrs, [:text, :post_id]) # added post_id here
    |> validate_required([:text, :post_id]) # added post_id here
  end

Thank you

1 Like
4

Yep - but seems a bit weird the comment_params already a map. This would be more typical (in my limited experience):

# Check for specific map keys to match the right function head
def create(conn, %{"text" => _text, "post_id" => _post_id}=params) do
    changeset = %Comment{}
      |> Comment.changeset(params)
    ...
end

or a bit safer (i.e. no unknown params slip through into your changeset, which should be containing damage anyway…)

def create(conn, %{"text" => text, "post_id" => post_id}) do
    changeset = %Comment{}
      |> Comment.changeset(%{text: text, post_id: post_id})
    ...
end
5

comment_params is already a map, because that’s how it’s sent from the form. so I receive params like this:

%{
  "_csrf_token" => "*********",
  "comment" => %{"text" => "this is a new comment"},
  "post_id" => "3"
}

and then I need to send a single map of attrs to create the changeset. so if I don’t want to amend the changeset method, seems like merging post_id into the existing map is fine?

6

Makes sense - so yes, you will have to merge post_id in…

1 Like
7

I think above code is a better version, because it ensure the post is valid.

Now, look at another version:

def create(conn, %{"comment" => comment_params, "post_id" => post_id}) do
    changeset = %Comment{}
      |> Comment.changeset(Map.put(comment_params, "post_id", post_id)) # this Map.put is ok, or is there nicer way? 
    ...
end

The post_id is fetched from user input, it is not reliable. What if an attacker submits an invalid post_id? In this condition, you have to do more works at schema level or database level to make sure the post is valid.

I think, you shouldn’t care about the extra database hit for now. Make sure everything is reliable is more important. And, a simple database query is fast, very fast.

1 Like
8

You are right, there’s no protection against messing with params. Thank you.

Also thanks for this, I’m gonna follow this advice:

I think, you shouldn’t care about the extra database hit for now. Make sure everything is reliable is more important. And, a simple database query is fast, very fast.

:+1: