Skip to content
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

Fix the build_payload #11

Merged
merged 2 commits into from
Apr 7, 2020
Merged

Fix the build_payload #11

merged 2 commits into from
Apr 7, 2020

Conversation

scottming
Copy link
Contributor

After Absinthe 1.4, only update the value is not enough, we also need to update the errors.

@simonprev
Copy link
Member

Can you add a test to see what was the problem with the behaviour before your changes? We run absinthe_error_payload on many project and we never encountered a bug with the current behaviour 😃

@scottming
Copy link
Contributor Author

scottming commented Mar 15, 2020

Hi, @simonprev , writing a test to show the problem is hard, but I know why the build_payload not work, but I can give you a demo: https://github.com/scottming/plate_slate/blob/master/test/plate_slate_web.

@scottming
Copy link
Contributor Author

scottming commented Mar 16, 2020

You can check this test to reproduce the problem.

https://github.com/scottming/plate_slate/blob/master/test/plate_slate_web/schema/mutation/create_menu_item_returns_payload_test.exs

They are the same logic, but the first test not work, and the second works. The difference is the first uses build_payload/1 middleware way, and the second uses the manual way.

So, I think the problem is we need to update the errors to [] after resolve middleware, and the tests of this lib not recovered it.

500f7fa

@jeregrine
Copy link

Tested this change locally and it fixes issue #15

@rwngallego
Copy link

@simonprev is it possible to approve this one? The package is totally broken at this moment with the latest Absynthe version.

@simonprev simonprev merged commit b5e7743 into mirego:master Apr 7, 2020
@simonprev
Copy link
Member

Just published 1.1.2 with the changes in this PR 🎉

@maennchen
Copy link

This Merge Request produces problems for me using absinthe 1.4.16.

Our app handles mutation errors and global errors differently. Before this change, a Middleware could write into the Absinthe.Resolution{errors} and the error would be sent directly to the user. Now the user instead gets an ok response with successful false.

If this is expected behavior, I would expect a breaking change in the versioning.

@maennchen
Copy link

Example Middleware in Question:

defmodule Acme.Schema.Middleware.CheckAuthenticationRequired do
  @moduledoc false

  @behaviour Absinthe.Middleware

  def call(
        %Absinthe.Resolution{context: %{creator_subject: creator_subject}} = resolution,
        callback
      )
      when is_function(callback, 1) and not is_nil(creator_subject) do
    resolution
  end

  def call(resolution, callback) when is_function(callback, 1) do
    if callback.(resolution),
      do: Absinthe.Resolution.put_result(resolution, {:error, "Unauthenticated"}),
      else: resolution
  end
end

@ppff
Copy link

ppff commented May 5, 2020

This PR indeed broke 10% of our unit tests. We'll stick to v1.1.1 for now.

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

6 participants