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

Migrate Poison to Jason #2237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leandroradusky
Copy link
Contributor

Close #2046.

Changed all references to Poison into references to Jason.

  • For some structs, @derive Jason.Encoder has to be added since this needs to be explicit for Jason.
  • The hardcoded return values of some tests needs to be modified since now the keys of the JSON returned are sorted alphabetically.

Just one replacement is missing:

  #lib/ask/o_auth_token.ex:37
  def access_token(token) do
    Poison.Decode.decode(token.access_token, as: %OAuth2.AccessToken{})
  end

Don't know why but changing here Poison.Decode.decode to Jason.decode would break lots of tests, don't know if this has to do something with the external wiring of the project, or if this schema should have something similar to the @derive statement mentioned above.
Because of that, Poison was not removed from the mix file. Jason version (1.0) was kept in the mix file, while there are newer versions available.

@ysbaddaden
Copy link
Contributor

Ah, that one was meant for @nthiad but she'll may be able to look at the issue you reached.

@leandroradusky
Copy link
Contributor Author

Ohh Sorry!! I've been doing some peer programming with Caspian today and I chose from the board what seems doable to show him some code! Sorry @nthiad!!

@nthiad
Copy link
Contributor

nthiad commented Apr 6, 2023

this appears to be why that line in particular doesn't work:

no support for decoding into data structures (the as: option).

from https://github.com/michalmuskala/jason#differences-to-poison

@nthiad
Copy link
Contributor

nthiad commented Apr 6, 2023

After some prodding, the access_token function receives an Ask.OAuthTokenServer object so there is no parsing that needs to happen because it has a member access_token which is a map that itself has an access_token key that can be used to instantiate a new OAuth2.AccessToken.

This makes the tests happy and I can still log in and out via the web interface:

  def access_token(token) do
    OAuth2.AccessToken.new(token.access_token["access_token"])
  end

@leandroradusky
Copy link
Contributor Author

I applied the change proposed by @nthiad and it worked neatly :) Thanks!

Despite that, I couldn't remove Poison from the mix file because of this. I've left a comment there to remove it in the future.

@ysbaddaden, I @ you to re-ask for your review :)

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

@leandroradusky That looks good to me, but I notice that oauth2 is already v0.7.0 in mix.exs 🤔

@leandroradusky
Copy link
Contributor Author

leandroradusky commented Apr 12, 2023

Exactly, in v0.7.0 Poison is only required as a test dependency it say.

  • If I remove Poison from mix.exs, tests will fail when using OAuth
test/ask_web/controllers/oauth_client_controller_test.exs:171
** (UndefinedFunctionError) function Poison.decode!/1 is undefined (module Poison is not available)
  • And, if I add , only: test to the Poison dependency, when trying to log in in the app I get:
exited in: :gen_server.call(:jose_server, {:json_module, Poison}) ** (EXIT) an exception was raised: ** (UndefinedFunctionError) function Poison.encode!/1 is undefined (module Poison is not available) Poison.encode!(%{"a" => 1, "b" => 2, "c" => %{"d" => 3, "e" => 4}}) (jose 1.11.2) src/jose_server.erl:380: :jose_server.check_json_module/1 (jose 1.11.2) src/jose_server.erl:114: :jose_server.handle_call/3 (stdlib 3.12.1.2) gen_server.erl:661: :gen_server.try_handle_call/4 (stdlib 3.12.1.2) gen_server.erl:690: :gen_server.handle_msg/6 (stdlib 3.12.1.2) proc_lib.erl:249: :proc_lib.init_p_do_apply/3

I'm a little bit confused here... There's something I'm misinterpreting?

@ysbaddaden
Copy link
Contributor

Yeah, same here, I'll try.

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.

Migrate from Poison to Jason (Phoenix 1.4)
3 participants