Ecto atom vs binary for attributes, and put_assoc helpers

Questions / Help
1

Hi!

I would like to know how do you handle the following cases.

In this example a manager can be set with a project, and when a manager’s project change, we check if the project is started and in that case, we force the manager to be “on site”. nil can also be set to remove the manager from any project.

Now the changeset attributes can come from different sources (http api or internal) so as many other applications we allow binary or atom attributes.

This gives us the following code.

# Context Functions

def update_manager(%Manager{} = manager, attrs) do
  manager =
    case attrs do
      %{project: _} -> Repo.preload(manager, :project)
      %{"project" => _} -> Repo.preload(manager, :project)
      %{} -> manager
    end

  manager
  |> Manager.changeset(attrs)
  |> Repo.update()
end


# Manager Schema

belongs_to :project, MyApp.Project


# Manager Functions

def changeset(manager, attrs) do
  manager
  |> cast(attrs, [:name, :on_site])
  |> validate_required([:name])      
  |> handle_assoc(:project, attrs)
end

defp handle_assoc(cset, :project, %{project: project}), do: set_project(cset, project)
defp handle_assoc(cset, :project, %{"project" => project}), do: set_project(cset, project)
defp handle_assoc(cset, :project, _), do: cset

defp set_project(cset, project) do
  case project do
    nil ->
      put_assoc(cset, :project, nil)

    %Project{started: started?} ->
      cset = put_assoc(cset, :project, project)
      
      if started?, 
        do: put_change(cset, :on_site, true), 
        else: cset
  end
end

I am not satisfied with that. We need to check atom and binary to preload (as we cannot preload during changeset operations – I found a topic about that but it’s still considered as a hack), and then we need to check both keys again so put the association.

How would you do this in a clean way?

2

Validate at the boundary and convert to atom keys.

A nitpick but I would have three matches on that case statement in set_project, one where started is true and one where it is false.

1 Like
3

Sounds like you’re relating existing projects with existing managers. In this case neither put_assoc or cast_assoc are the way to go imo. I would split up updating the managers project from updating the project with on_site: true if it is already started.

1 Like
4

Yeah I wanted to force atom keys in the context but I am not sure the changeset function should only be called from that one place.

A nitpick but I would have three matches

Sure. I wanted a single put_assoc call in the code but I agree.

5

It’s the manager that is on site. The value is forced to true when assigned to a started project. When assigned to and idle project or nil, on_site does not change.

Sounds like you’re relating existing projects with existing managers. In this case neither put_assoc or cast_assoc are the way to go imo.

How to you associate two existing entities then? On other entities we just set the related_thing_id FK field, which is simpler, but here we want to get the full related struct as we have to dig into it.

6

You shouldn’t futz with attrs, just preload the project and reference it directly.

def update_manager(%Manager{} = manager, attrs) do
  manager
  |> Repo.preload(:project)
  |> Manager.changeset(attrs)
  |> Repo.update()
end

You don’t need to put project in attrs. To remove a project, I would just set the project_id directly as opposed to put_assoc nil.

def changeset(manager, attrs) do
  manager
  |> cast(attrs, [:name, :project_id])
  |> validate_required([:name])      
  |> set_on_site(manager.project)
end

defp set_on_site(changeset, nil) do
  put_change(changeset, on_site: false)
end

defp set_on_site(changeset, project) do
  will_have_project? = !is_nil(get_field(changeset, :project_id))

  put_change(changeset, on_site: will_have_project? and project.started)
end

The “only set on_site if project exists” logic there was written pretty quickly so you may want to futz with that.

EDIT: Sorry, I did a few edits since posting. My focus was on not shoving the project into attrs over how to deal with nilifying the project.

1 Like
7 
|> set_on_site(manager.project)

I am not sure this will work as manager.project is the old project, or nil, not the project that you set with project_id. This is why I need the new project struct.

Also while it is simpler I am not sure about preloading project all the time. It will not change often, while manager updates are very frequent. But that may be a premature optimization vs. careless data loading problem too.

8

Ohhhhhh ok, sorry I was a bit confused and then was going to ask for clarification but then mistakeningly thought I understood. In that case ya, what others have said, split this up and try and wrangle it all within a changeset. I would maybe do something very similar to what I did but add a parameter for the project, especially if you already have the project in assigns:

def changeset(manager, project, attrs) do
  manager
  |> cast(attrs, [:name])
  |> validate_required([:name])      
  |> put_assoc(:project, project)
  |> set_on_site(project)
end
9

Yes, use the foreign key. You can do a transaction of update manager’s project_id, load project, conditionally update project.

Sounds like you should make updating a manager and changing the project of a manager two separate “things” to be done. Why couple one to the other if they have different complexities and requirements to be checked.

1 Like
10

Yeah that’s what I am thinking now too, also coherent with @sodapopcan last comment. You’re right, there is unnecessary coupling here.

load project, conditionally update project.

There is no change to the project. Only the manager is updated.

So that would be opening a transaction, update the manager with the attrs, then if the project_id was set in the attrs (that’s still two keys to check though but once is fine) then load the project and re-update the manager with the on_site attr ; and commit.

It’s kind of what I do already. I’ll move the check and preloading in the changeset function directly at least.

edit None of this is hard but really is there not a put_assoc(changeset, :some_key , attrs) helper kind of thing?

11

Ah I missed that. For that you could also look into prepare_changes. That will give you the changeset in the transaction it is updated in. There you can then check the manager changeset for a change in project_id, if there is one load the project and potentially amend the manager changeset with the on_site change.

*_assoc functions in changesets are mostly meant for editing “projects” as part of a manager. Assigning a project to a manager is simpler handled without those, even if they technically support that as well.

1 Like
12

Thanks! This allows to put all the logic in one place, and still do things conditionnally.

1 Like
13

Just FWIW I generally only use prepare_changes for stuff like counter caches (like in the example in the docs) and not for more crucial business rules just because it sorta hides that it’s happening. This is why I pass pertinent schemas as explicit parameters (just looking at the context function you have no clue that project is important). I still do the import stuff in the changeset (which I also put in the schema) so YMMV but just wanted to express my perhaps somewhat muddled thought process. I’m certainly not trying to convince you not to if it feels right to you!

2 Likes
14

A general piece of advice: if a function’s taking different input shapes with different expectations, it’s probably not one function.

2 Likes
15

I agree but in that case most changeset functions are basic wrappers around Ecto.Changeset.cast that accepts both forms of keys for attributes. I am not seeing myself duplicating all the changeset functions!