Prefers the built-in JSON library from Elixir 1.18.x when it's available #650
Conversation
Unfortunately, the JSON module doesn't support the `pretty` option, so that will use either Jason or Poison to encode the API specs in the `openapi.spec.json` mix task.
|
Hi! any chance that somebody can review this? And/or how I can help? Happy to help! |
lib/mix/tasks/openapi.spec.json.ex
Outdated
| defp encode(spec, %{pretty: pretty}) do | ||
| spec | ||
| |> OpenApiSpex.OpenApi.json_encoder().encode(pretty: pretty) | ||
| |> json_encoder().encode(pretty: pretty) |
There was a problem hiding this comment.
We can provide a helpful error message when so suitable encoder is configured.
Currently it fails with:
** (UndefinedFunctionError) function nil.encode/2 is undefined
nil.encode(%{"components" => %{"responses" => %{"unprocessable_entity" => %{"content" => %{"application/json" => %{"schema" => %{"type" => "object", "x-struct" => nil, "x
We can follow this pattern in Plug. https://github.com/elixir-plug/plug/blob/main/lib/plug/parsers/json.ex#L66
defp encode(spec, %{pretty: pretty}) do
encoder = json_encoder()
validate_encoder!(encoder)
encoder.encode(pretty: pretty)
endThere was a problem hiding this comment.
Ah right that is a good one. Will fix that 👍 ❤️
There was a problem hiding this comment.
It appears we'll need a few more changes since the built-in JSON module does not export an encode/2 function but only an encode!/2.
… the `pretty` option to `true`
| @@ -238,8 +238,8 @@ defmodule OpenApiSpex do | |||
| - `:struct?` (boolean) - When false, prevents the automatic generation | |||
| of a struct definition for the schema module. | |||
| - `:derive?` (boolean) When false, prevents the automatic generation | |||
| of a `@derive` call for either `Poison.Encoder` | |||
| or `Jason.Encoder`. Using this option can | |||
| of a `@derive` call for either `JSON.Encoder` | |||
There was a problem hiding this comment.
Add comma after JSON.Encoder.
| {:ok, json} -> | ||
| "#{json}\n" | ||
|
|
||
| {:error, error} -> | ||
| Mix.raise("could not encode #{inspect(spec)}, error: #{inspect(error)}.") | ||
| end | ||
| end | ||
|
|
||
| defp json_encoder(), do: Enum.find([Jason, Poison, JSON], &Code.ensure_loaded?/1) |
There was a problem hiding this comment.
The module used to encode should be the one derived in https://github.com/open-api-spex/open_api_spex/pull/650/files#diff-80ea122ee3ae0be84e69b60bc10913073a339ec09ff0654e4805e108190754bbR265
I don't see why we should prefer the builtin over a library the user has picked.
There was a problem hiding this comment.
Right. I didn’t really think this through. Sorry. I’ll move this PR back to draft or close it and will think this a bit more through.
| @@ -80,7 +80,7 @@ defmodule OpenApiSpex.OpenApi do | |||
|
|
|||
| def json_encoder, do: @json_encoder | |||
|
|
|||
| for encoder <- [Poison.Encoder, Jason.Encoder] do | |||
| for encoder <- [JSON.Encoder, Poison.Encoder, Jason.Encoder] do | |||
There was a problem hiding this comment.
My suggestion would be to make JSON.Encoder the last option, so that it's only picked up as a fallback if the application doesn't depend on a library.
There was a problem hiding this comment.
Could be but I think it's even nicer to let the user actively choose the preferred encoder. Just like Phoenix does: https://hexdocs.pm/phoenix/Phoenix.html
It could well be that another hex package in a developers project already include Jason or Poison.
But that will require some more effort to implement 😄
There was a problem hiding this comment.
I'd advise against application configuration for the JSON encoder in open_api_spex for the following reasons:
- https://hexdocs.pm/elixir/1.14.3/library-guidelines.html#avoid-application-configuration
- It's only used for exporting the spec, so the main reason most people carefully pick a JSON library (performance) is not important in this case
- It increases the probability that things won't "just work"
By searching for the
JSONmodule, OpenApiSpex can use the built-in library from Elixir 1.18.x.Note: Unfortunately, the JSON module doesn't support the
prettyoption. For theopenapi.spec.jsonmix task it will try to use either Jason or Poison.