Elixir: Switch Poison to Jason #16061
Conversation
|
@barttenbrinke First, thank you very much for the time you put in creating this PR! It helps us in bringing the various languages and generators to a higher level. I tried to compile the new generated samples, but it resulted in the following error: Were you able to compile the generated samples on your side? The steps to reproduce this are: |
| @@ -16,7 +16,7 @@ | |||
| } | |||
| end | |||
|
|
|||
| defimpl Poison.Decoder, for: {{&moduleName}}.Model.{{&classname}} do | |||
| defimpl Jason.Decoder, for: {{&moduleName}}.Model.{{&classname}} do | |||
There was a problem hiding this comment.
This results in the following error:
== Compilation error in file lib/openapi_petstore/model/_special_model_name_.ex ==
** (ArgumentError) Jason.Decoder is not a protocol
(elixir 1.14.4) lib/protocol.ex:330: Protocol.assert_protocol!/2
lib/openapi_petstore/model/_special_model_name_.ex:19: (file)
(elixir 1.14.4) lib/kernel/parallel_compiler.ex:340: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7
There was a problem hiding this comment.
|
@mrmstn Thanks! - I will try and resolve this this week. |
|
Ugh, Jason.Decoder no longer supports decoding into Structs by default (as it can be unsafe) - however as you are generating libs here, is not a concern - https://github.com/michalmuskala/jason#differences-to-poison Let me think about how to fix this. |
|
So, the only thing Poison does is generate a basic protocol definition: defprotocol Poison.Decoder do
@fallback_to_any true
@typep as :: map | struct | [as]
@type options :: %{
optional(:keys) => :atoms | :atoms!,
optional(:decimal) => boolean,
optional(:as) => as
}
@spec decode(t, options) :: any
def decode(value, options)
end
defimpl Poison.Decoder, for: Any do
def decode(value, _options) do
value
end
endAnd then relies on the different classes to provide their own, sneaking them in as an argument... Well that sort of works, but in reality you would want to pull this through an Ecto Changeset and then cast all the things :/ Like here - https://elixirforum.com/t/how-to-decode-a-json-into-a-struct-safely/14331/14?u=barttenbrinke |
* parse date-time values to Elixir DateTime * improve formatting in various places, so there's less changes by `mix format` later * fix Java version in flake.nix
|
I believe this should be a safe approach to turning maps into structs without resorting to There's one thing that I've noticed that seems to be off. When polymorphism is used in arrays or objects, then from my short testing, it seems that the concrete classes won't be used, but instead only the parent class is evaluated. I inserted a TODO in the relevant test for the posterity. |
|
@mrmstn @wpiekutowski did some awesome work and simplified the whole poison type decode magic to generating decoders for all types based on the swagger spec # NOTE: This file is auto generated by OpenAPI Generator 7.0.0-SNAPSHOT (https://openapi-generator.tech).
# Do not edit this file manually.
defmodule OpenapiPetstore.Model.EnumTest do
@moduledoc """
"""
@derive Jason.Encoder
defstruct [
:enum_string,
:enum_string_required,
:enum_integer,
:enum_number,
:outerEnum,
:outerEnumInteger,
:outerEnumDefaultValue,
:outerEnumIntegerDefaultValue
]
@type t :: %__MODULE__{
:enum_string => String.t | nil,
:enum_string_required => String.t,
:enum_integer => integer() | nil,
:enum_number => float() | nil,
:outerEnum => OpenapiPetstore.Model.OuterEnum.t | nil,
:outerEnumInteger => OpenapiPetstore.Model.OuterEnumInteger.t | nil,
:outerEnumDefaultValue => OpenapiPetstore.Model.OuterEnumDefaultValue.t | nil,
:outerEnumIntegerDefaultValue => OpenapiPetstore.Model.OuterEnumIntegerDefaultValue.t | nil
}
alias OpenapiPetstore.Deserializer
def decode(value) do
value
|> Deserializer.deserialize(:outerEnum, :struct, OpenapiPetstore.Model.OuterEnum)
|> Deserializer.deserialize(:outerEnumInteger, :struct, OpenapiPetstore.Model.OuterEnumInteger)
|> Deserializer.deserialize(:outerEnumDefaultValue, :struct, OpenapiPetstore.Model.OuterEnumDefaultValue)
|> Deserializer.deserialize(:outerEnumIntegerDefaultValue, :struct, OpenapiPetstore.Model.OuterEnumIntegerDefaultValue)
end
endThis is a very robust solution for now and the future imho 👍. |
There was a problem hiding this comment.
@wpiekutowski
Wow. I'm really impressed, great work! I really like the way you took with the decode function and the replacement with the strut with just its base type, fantastic!
Now there are just some small open questions according to the changes in the common area of the generator. As I'm just in the tech. Committee of elixir, I feel a lot better if one of the core team could have a lot of those changes, as they have a bigger picture of the impact overall. (cc @wing328 )
Thank you once again for your time and effort. Your contributions are immensely appreciated!
There was a problem hiding this comment.
@wpiekutowski Do we need those changes? Since this yaml is used by a lot of the sample clients, we would need to regenerate all those samples as well – and get the approval from all the different tech. committees
Alternately, @wing328 could you, or someone from the core team, have a look at those changes? Would it be possible to just generate the sample clients and go on with the PR? Or would it be preferred to split this PR up to get a clean split between elixir and common changes?
There was a problem hiding this comment.
@mrmstn Thanks for your kind words!
I agree this change to the YAML file is significant. The reason for this change is that Petstore API server is now redirecting HTTP to HTTPS and Elixir client isn't configured to follow redirects. This caused included Elixir tests to fail.
The way to avoid this change could be to add a redirection Tesla middleware (1) or set base_url in tests – avoid using the default one extracted from the API spec (2).
I'm not very keen on (1) as it doesn't seem that Ruby or Elm clients are following redirects. There's a risk of credentials leak.
(2) makes more sense – doesn't add redirect following and doesn't affect other clients.
In the long run, I think we should encourage using HTTPS by defaulting to it.
There was a problem hiding this comment.
I see that this fails CI, because now many samples have a different URL compiled in. It seems that HTTP -> HTTPS switch should be at least a sepearate PR, if we agree that this is something that needs to be done. I'll set the base_url in Elixir tests and revert changes to the API spec YAML file.
There was a problem hiding this comment.
I think petstore is running as a docker container in the circle CI env, so keeping this the same would be fine I guess?
There was a problem hiding this comment.
@wpiekutowski thanks for the further explanation, I absolutely share your opportunities and would also prefer point 2 and highly welcome a PR with the changes of the yaml file and the new clients in a separate PR. Thanks for shedding some light on the issue with HTTPS.
| @@ -13,7 +13,7 @@ | |||
| devShells.default = pkgs.mkShell | |||
| { | |||
| buildInputs = with pkgs;[ | |||
| jdk8 | |||
| jdk11 | |||
There was a problem hiding this comment.
@wing328 the run-in-docker.sh also uses jdk11, so this change looks good to me, any veto against this change in the common area?
There was a problem hiding this comment.
Exactly. Recent maven versions require JDK >= 11.
|
Nice, CI goes through. As I'm currently quite in a hurry, I'm excited to merge this PR in the evening, (~ 18:00 UTC) |
|
I ran the petstore tests locally but got these errors: (Elixir CI tests were run at shippible.io before which is no longer freely available for open-source project if I remember correctly) |
|
@wing328 Could you share more details in what environment are you running these tests? I'm running these in a NixOS vm using the environment setup from flake.nix. |
|
Tested again by running While ruby and php tests are fine, e.g. |
|
We'll rollback the https change, as it is creating too much confusion. |
HTTPS doesn't work for folks who setup petstore.swagger.io as described in docs/faq-contributing.md.
|
@wing328 I've reverted Elixir tests to HTTP and I'm going to use a local docker petstore server or a proxy for local testing. |
|
Now all tests passed locally: |
|
FYI. We're moving away from petstore to echo server for client tests. https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests#echo-server has more information. (not required to add it as part of this PR). |
|
Interesting. In case I commit myself one day to switching Elixir to the echo server, should I delete the petstore Elixir client sample? |
|
Looks like you need to record the response first? So it is like ExVCR? Ah, there is the global yaml for that. |
For python, java, etc, we keep both petstore and echo client for the time being as echo client doesn't cover file upload yet (we can for sure easily add an endpoint for that in echo server spec) After the echo client fully covers all the test cases (operations, models) in the petstore client, please feel free to remove the petstore client (elixir). |
Basically you get back what the client sends to the server in the response body. Please give it a try when you've time and I hope you like this approach for testing the client. |
|
Hi @wpiekutowski, @barttenbrinke I've just merged the PR. Thank you for your excellent work and for addressing the necessary changes! Your contribution has made a significant impact on our project. These updates will be incorporated into the upcoming 7.0.0 release, scheduled for the 21st of July. A big thanks to everyone who played a role in this contribution. Your efforts are greatly appreciated! |
likely we need postpone that as there are other enhancements (dart, ruby) that we want to include in 7.0.0 release |
|
Thanks for working with us on merging this. I think I might contribute some more stuff thanks to your supportive attitude. |
|
Thank you for your contributions too. This project wouldn't be a success without contributors like you guys. I'm sure the Elixir community appreciates all the efforts you guys have put into the Elixir client generator. Look forward to more PRs to further improve openapi-generator. |
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.For Windows users, please run the script in Git BASH.
master(6.3.0) (minor release - breaking changes with fallbacks),7.0.x(breaking changes without fallbacks)@mrmstn As Poison no longer has active support and Phoenix relies on Jason, I swapped the dependency. Tried to make the change as minimal as possible. As Jason is supported and 100% compatible with Poison, I basically did a global replace and regenerated the samples.