Skip to content

Add helper to insert values into context #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 26, 2020

Conversation

jayjun
Copy link
Contributor

@jayjun jayjun commented May 25, 2020

Absinthe.Plug.put_options(conn, context: context) overwrites any previously set context.

This pull request adds assign_context which merges values instead. This allows multiple plugs that configure context to be composed.

conn
|> Absinthe.Plug.assign_context(current_user: current_user)
|> Absinthe.Plug.assign_context(foo: "bar")

I considered naming it put_in_context but conventionally put and put_in functions do not merge. Phoenix’s assign appears closest in behaviour.

Comment on lines 366 to 369
context =
absinthe
|> Map.get(:context, %{})
|> Map.merge(Enum.into(assigns, %{}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge order here is probably counter to what you want. assigns should be merged into the current context not the other way around, so that if you do want to override an existing assign it works as expected.

Suggested change
context =
absinthe
|> Map.get(:context, %{})
|> Map.merge(Enum.into(assigns, %{}))
context =
assigns
|> Map.new
|> Map.merge(Map.get(absinthe, :assigns, %{}))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that right wins.

iex> Map.merge(%{foo: "bar"}, %{foo: "baz"})      
%{foo: "baz"}

So the current code un-pipelined

context = Map.get(absinthe, :context, %{})
context = Map.merge(context, Map.new(assigns))

looks correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct! I got Map.merge and Enum.into backwards, my bad!

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea, just a couple of changes and we'll have it merged in, thank you!

end

def assign_context(conn, assigns) do
put_options(conn, context: Enum.into(assigns, %{}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
put_options(conn, context: Enum.into(assigns, %{}))
put_options(conn, context: Map.new(assigns))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! put_options itself needs the same change. Let me know if you want those included here.

assert conn.private.absinthe.context.current_user.id == 1
assert conn.private.absinthe.context.foo == "bar"
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case that tests assigning a value that is already assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@benwilson512 benwilson512 merged commit 7f7dcc6 into absinthe-graphql:master May 26, 2020
@jayjun jayjun deleted the context branch May 26, 2020 13:16
@jeroenvisser101
Copy link

@benwilson512 are you planning to released this in 1.5.1(-rc)? We'd love to use this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants