after_encode_and_sign result not used

[jon-mcclung-fortyau]

Steps to Reproduce

I have updated the after_encode_and_sign result to conditionally return a different token from the one passed to me. However, this token is not used at all by the code.

Expected Result

I expected that by returning {:ok, different_token}, Guardian.encode_and_sign would return {:ok, different_token, claims}

Actual Result

It returns {:ok, original_token, claims}.

You can see the problem code here. By using the underscore instead of token, the value of after_encode_and_sign is ignored except to check success/failure.

  @spec encode_and_sign(module, any, Guardian.Token.claims(), options) ::
          {:ok, Guardian.Token.token(), Guardian.Token.claims()} | {:error, any}
  def encode_and_sign(mod, resource, claims \\ %{}, opts \\ []) do
    claims =
      claims
      |> Enum.into(%{})
      |> Guardian.stringify_keys()

    token_mod = Guardian.token_module(mod)

    with {:ok, subject} <- returning_tuple({mod, :subject_for_token, [resource, claims]}),
         {:ok, claims} <- returning_tuple({token_mod, :build_claims, [mod, resource, subject, claims, opts]}),
         {:ok, claims} <- returning_tuple({mod, :build_claims, [claims, resource, opts]}),
         {:ok, token} <- returning_tuple({token_mod, :create_token, [mod, claims, opts]}),
         {:ok, _} <- returning_tuple({mod, :after_encode_and_sign, [resource, claims, token, opts]}) do
      {:ok, token, claims}
    end
  end

I could have just made a Pull Request, but I thought perhaps this behavior is what some people actually want? At a minimum I would expect it to look for :ok instead of a tuple since that implies you use the result.

[jon-mcclung-fortyau]

Would something like this be a good workaround?

defmodule MyApp.Guardian do
  use Guardian, otp_app: :my_app
  defoverridable [encode_and_sign: 3]
  
  ...

  @spec encode_and_sign(any, Guardian.Token.claims(), Guardian.options()) ::
          {:ok, Guardian.Token.token(), Guardian.Token.claims()} | {:error, any}
  def encode_and_sign(resource, claims \\ %{}, opts \\ []) do
    mod = __MODULE__

    claims =
      claims
      |> Enum.into(%{})
      |> Guardian.stringify_keys()

    token_mod = Guardian.token_module(mod)

    with {:ok, subject} <- Guardian.returning_tuple({mod, :subject_for_token, [resource, claims]}),
         {:ok, claims} <- Guardian.returning_tuple({token_mod, :build_claims, [mod, resource, subject, claims, opts]}),
         {:ok, claims} <- Guardian.returning_tuple({mod, :build_claims, [claims, resource, opts]}),
         {:ok, token} <- Guardian.returning_tuple({token_mod, :create_token, [mod, claims, opts]}),
         {:ok, token} <- Guardian.returning_tuple({mod, :after_encode_and_sign, [resource, claims, token, opts]}) do
      {:ok, token, claims}
    end
  end

  ...

end

[yordis]

After a quick search: https://github.com/search?q=after_encode_and_sign+language%3AElixir&type=code&l=Elixir&p=1

They are many people who misuse Guardian.DB.after_encode_and_sign/4 function which does not return {:ok, Guardian.Token.token()} | {:error, atom} but instead they it returns {:ok, {resource, type, claims, jwt}}

Based on the contract alone, it is safe to assume they are making a mistake today.

But I digress.

---

I could have just made a Pull Request, but I thought perhaps this behavior is what some people actually want?

The token was already created at this point in the pipeline, so you wouldn't "modify" the token.

I do agree with you that it is weird expecting {:ok, token} back instead of :ok. The tricky situation is that it would be a breaking change to change the signature.

@doomspork do you remember what was your intent here? It feels that it should be :ok as a return (aside from the breaking change situation).