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

[BUG][ELIXIR] Date format not generating correct code, and date-time format not generating code #8804

Closed
5 of 6 tasks
tomjoro opened this issue Feb 23, 2021 · 12 comments
Closed
5 of 6 tasks

Comments

@tomjoro
Copy link

tomjoro commented Feb 23, 2021

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

This is actually 2 issues, but both would need to be considered at same time in order to fix.

For dates, and datetimes there are two date/datetime related formats in openapi:

  • date – full-date notation as defined by RFC 3339, section 5.6, for example, 2017-07-21
  • date-time – the date-time notation as defined by RFC 3339, section 5.6, for example, 2017-07-21T17:32:28Z

An example of date format in json:

"closedDate":{"title":"closed date","type":"string","format":"date","example":"2020-04-20"},

The generated deserializer.ex cannot parse "2020-04-20", because DateTime.from_iso8601 doesn't work with a date only. You must use Date.from_iso8601

iex(1)> DateTime.from_iso8601("2020-04-20")
{:error, :invalid_format}

The generated deserializer.ex

def deserialize(model, field, :date, _, _options) do
    value = Map.get(model, field)
    case is_binary(value) do
      true -> case DateTime.from_iso8601(value) do
                {:ok, datetime, _offset} ->
                  Map.put(model, field, datetime)
                _ ->
                  model
              end
      false -> model
    end
  end

For date-time example from swagger:

"closedDate":{"title":"closed date","type":"string","format":"date","example":"2020-04-20"}, `
"createdDTime":{"title":"Created date time","type":"string","format":"date-time"}

The @type is generated correctly like this:

    :closedDate => Date.t | nil,
    :createdDTime => DateTime.t | nil,

But there is no deserializer generated for the date-time format in the decode part of the json.ex file.

defimpl Poison.Decoder, for: InvoiceLineJson do
  import Project.Deserializer
  def decode(value, options) do
    value
    |> deserialize(:closedDate, :date, nil, options)
  end
end
openapi-generator version

5.0.1

OpenAPI declaration file content or url
Generation Details

Run openapi-generator on a swagger that includes "date" fields and "date-time" fields.

Steps to reproduce

Run openapi-generator on a swagger that includes "date" fields and "date-time" fields.

Related issues/PRs

I don't see any other Elixir related PRs on this?

Suggest a fix

I'm happy to help fix this, but just thought I'd document this if anyone else is running into this problem? It seems quite fundamental to most openapi generation.

Fix proposal:

  • date format
    • Change deserializer.ex to use Date.from_iso8601
  • date-time format
    • add "date-time" struct to the deserializer to use DateTime.from_iso8601
    • generate a line in each json schema file to call the deserializer (same as date works already)

Problem with this fix:

This fix could be problematic for anyone who is using a swagger file that has specified date format and then actually sending a date-time, i.e. 2021-02-22T10:22:19Z, because Date.from_iso8601 will not parse a date-time in Elixir.

I guess we could inspect the string that is provided and make a determination if it is Date or DateTime by searching for a "T" in the string and call either Date.from_iso8601 or DateTime.from_iso8601. This single change to deserializer.ex would make the date format work, but the generator still needs to be updated for date-time.

Comments? Ideas?

@tomjoro
Copy link
Author

tomjoro commented Feb 23, 2021

In deserializer.ex

This seems like a terrible hack, and I wonder if it would introduce other problems?

  def deserialize(model, field, :date, mod, options) do
    value = Map.get(model, field)
    case is_binary(value) do
      true ->
            if String.contains?(value, "T") do
              deserialize(model, field, :datetime, mod, options)
            else
              case Date.from_iso8601(value) do
                {:ok, date} ->
                  Map.put(model, field, date)
                _ ->
                  model
              end
            end
      false -> model
    end
  end
  def deserialize(model, field, :datetime, _, _options) do
    value = Map.get(model, field)
    case is_binary(value) do
      true -> case DateTime.from_iso8601(value) do
                {:ok, datetime, _offset} ->
                  Map.put(model, field, datetime)
                _ ->
                  model
              end
      false -> model
    end
  end

@leonardb
Copy link

In deserializer.ex

This seems like a terrible hack, and I wonder if it would introduce other problems?

@tomjoro Would it not be cleaner to pattern match the value? Apologies for erlang code.

...
    case Value of
        <<_:4/binary, "-", _:2/binary, "-", _:2/binary>> ->
            IsDate;
        <<_:4/binary, "-", _:2/binary, "-", _:2/binary,
            S:1/binary,
            _:2/binary, ":", _:2/binary, ":", _:2/binary,
            _/binary>>
        when S =:= <<"T">> orelse S =:= <<" ">> ->
            IsDateTime
    end
...

@ppicom
Copy link

ppicom commented Jul 28, 2023

Hi! 👋 I bumped on this issue and plan on fixing it. Is there someone working on it? 😄 Anyway, so far this is where I stand.

I generated a client with two models. One has a date, the other a datetime. I'll leave the OpenAPI spec here:

Click to see the JSON OpenAPI spec

/date returns a Date and /datetime, well, a datetime.

{
  "swagger": "2.0",
  "info": {
    "version": "1.0.0",
    "title": "Date and Datetime API",
    "description": "API to get a date and a datetime"
  },
  "host": "api.example.com",
  "basePath": "/v1",
  "schemes": [
    "http",
    "https"
  ],
  "paths": {
    "/date": {
      "get": {
        "summary": "Get the current date",
        "produces": [
          "application/json"
        ],
        "responses": {
          "200": {
            "description": "Successful response",
            "schema": {
              "type": "object",
              "properties": {
                "date": {
                  "type": "string",
                  "format": "date",
                  "example": "2023-07-28"
                }
              }
            }
          }
        }
      }
    },
    "/datetime": {
      "get": {
        "summary": "Get the current datetime",
        "produces": [
          "application/json"
        ],
        "responses": {
          "200": {
            "description": "Successful response",
            "schema": {
              "type": "object",
              "properties": {
                "datetime": {
                  "type": "string",
                  "format": "date-time",
                  "example": "2023-07-28T12:34:56Z"
                }
              }
            }
          }
        }
      }
    }
  }
}

With that, I generated an elixir client. This is what the models look like:

Click to see the Date model. Notice the call to `deserialize/4`

This model has a date in its response.

# NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
# https://openapi-generator.tech
# Do not edit the class manually.

defmodule ArgusClient.Model.DateGet200Response do
  @moduledoc """

  """

  @derive [Poison.Encoder]
  defstruct [
    :date
  ]

  @type t :: %__MODULE__{
          :date => Date.t() | nil
        }
end

defimpl Poison.Decoder, for: ArgusClient.Model.DateGet200Response do
  import ArgusClient.Deserializer

  def decode(value, options) do
    value
    |> deserialize(:date, :date, nil, options)
  end
end

Click to see the Datetime resulting model. No deserialization going on.

This is the model with a datetime in it.

# NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
# https://openapi-generator.tech
# Do not edit the class manually.

defmodule ArgusClient.Model.DatetimeGet200Response do
  @moduledoc """

  """

  @derive [Poison.Encoder]
  defstruct [
    :datetime
  ]

  @type t :: %__MODULE__{
          :datetime => DateTime.t() | nil
        }
end

defimpl Poison.Decoder, for: ArgusClient.Model.DatetimeGet200Response do
  def decode(value, _options) do
    value
  end
end

After taking a closer look at the template:

{{>licenseInfo}}
{{#models}}{{#model}}defmodule {{moduleName}}.Model.{{classname}} do
  @moduledoc """
  {{&description}}
  """

  @derive [Poison.Encoder]
  defstruct [
    {{#vars}}{{#atom}}{{&baseName}}{{/atom}}{{^-last}},
    {{/-last}}{{/vars}}
  ]

  @type t :: %__MODULE__{
    {{#vars}}{{#atom}}{{&baseName}}{{/atom}} => {{{datatype}}}{{#isNullable}} | nil{{/isNullable}}{{^isNullable}}{{^required}} | nil{{/required}}{{/isNullable}}{{^-last}},
    {{/-last}}{{/vars}}
  }
end

defimpl Poison.Decoder, for: {{&moduleName}}.Model.{{&classname}} do
{{#hasComplexVars}}
  import {{&moduleName}}.Deserializer
  def decode(value, options) do
    value
    {{#vars}}
    {{^isPrimitiveType}}
    {{#baseType}}|> deserialize({{#atom}}{{&baseName}}{{/atom}}, {{#isArray}}:list, {{&moduleName}}.Model.{{{items.baseType}}}{{/isArray}}{{#isMap}}:map, {{&moduleName}}.Model.{{{items.baseType}}}{{/isMap}}{{#isDate}}:date, nil{{/isDate}}{{#isDateTime}}:date, nil{{/isDateTime}}{{^isDate}}{{^isDateTime}}{{^isMap}}{{^isArray}}:struct, {{moduleName}}.Model.{{baseType}}{{/isArray}}{{/isMap}}{{/isDateTime}}{{/isDate}}, options)
    {{/baseType}}
    {{/isPrimitiveType}}
    {{/vars}}
{{/hasComplexVars}}
{{^hasComplexVars}}
  def decode(value, _options) do
    value
{{/hasComplexVars}}
  end
end
{{/model}}{{/models}}

The issue seems to be that hasComplexVars is false for Datetime and true for Date. I will be working on it over the weekend, but I wanted to confirm that this is, indeed, a bug. Thanks!

@halostatue
Copy link
Contributor

I don’t think that this has been worked on in a while, and I have not had time to work on my replacement for this template in more than a month.

@ppicom
Copy link

ppicom commented Jul 30, 2023

I don’t think that this has been worked on in a while, and I have not had time to work on my replacement for this template in more than a month.

I'm sorry, what do you mean by the "replacement for this template"?

@halostatue
Copy link
Contributor

The Elixir template is unable to be upgraded to a more modern version of Poison or to a better parser like Jason (I tried; upgrading to any later version of Poison results in a double-decoding that breaks almost every test because of a change in how Poison itself works; switching to Jason is impossible because it does not have a decoding protocol like Poison). It has a number of choices in it which make this very difficult to try to remediate, and it doesn’t generate code that looks much like what I believe an Elixir project should look like in 2023. Then you get missing or broken features like like this date parsing issue.

There are also some questionable decisions on how certain things get interpreted with ambiguous OpenAPI specifications (such as that from Magento), but those may be unavoidable. Without doing more testing, I cannot be sure.

I believe that this template/generator could be changed to fix most of these issues, but I’m going to pursue it as a new Elixir generator because the approach that I plan on taking will be very different in that it will keep the data as parsed JSON as long as possible rather than immediately trying to atomize the keys. When I make the core template, I’m planning on making it use Jason by default, but allow the use of Poison if preferred. I might make it so that the JSON parser is configurable since both Jason and Poison use nearly identical decoding requests.

This specific issue is likely resolvable with the existing template (and likely this PR; it’s been a while), but I have other issues to resolve and hope to resolve this and other issues with a different template.

@mrmstn
Copy link
Contributor

mrmstn commented Aug 1, 2023

Hello Everyone,

My apologies for the delayed response. I recommend checking out the latest master. The modifications in this pull request could potentially resolve the issue at hand: #16061

https://github.com/OpenAPITools/openapi-generator/pull/16061/files#diff-2d2b744ffec9aadd5415f3c590af9f84883f54cae370235d9b38d8afe5df85ebR79

In light of my current workload, I regret to say that I'm not able to personally validate whether this issue still persists in the master at the moment. I do plan to revisit this over the weekend if possible. However, if anyone has a bit of free time before then, I invite you to give it a try and share your findings. Your efforts are greatly appreciated.

@ppicom
Copy link

ppicom commented Aug 3, 2023

I'm happy to test it, I'm just having issues getting the project up and running on my machine :)

@tomjoro
Copy link
Author

tomjoro commented Aug 3, 2023

I originally reported this. We have been using that deserializer "patch/hack" I posted by adding it to the code in a post generation step :) for over a year now. It would be great to have it properly fixed.

I think you are correct in your assessment of the issue. As long as the swagger is consistent and clients actually use it correctly. I've had some clients pass date-time even though the swagger specifies date hence I had handling for that. But clients should follow the specification.

Date.from_iso8601 will not parse a date-time in Elixir.

Thanks for picking this up!

@ppicom
Copy link

ppicom commented Aug 3, 2023

Thanks a lot! I'll do my best. Any pointers on getting up and running locally? I'm getting north of 24k errors, even after pulling all dependencies 🤔

@mrmstn
Copy link
Contributor

mrmstn commented Aug 3, 2023

@ppicom, I appreciate your time for testing.

For acquiring the most recent master version, you can conveniently utilize the latest pre-packaged docker container, which already contains the update.

Here's an illustrative example of how the command might look (In case you are using podman, just replace docker with podman):

docker run \
    --rm \
    -v "$PWD:/local:Z" \
    openapitools/openapi-generator-cli:latest \
      generate \
      --generator-name elixir \
      --skip-validate-spec \
      --input-spec /local/openapi.yaml \
      --output /local/

In this command:

  • The --rm option is used to automatically clean up the container and remove the file system when the container exits.
  • The -v "$PWD:/local:Z" option maps your current working directory into the container under the /local path.
  • openapitools/openapi-generator-cli:latest is the image that gets pulled from the Docker registry.
  • generate is the operation that the OpenAPI Generator CLI will perform.
  • The --generator-name elixir argument specifies the language (in this case, Elixir) for which the client SDK will be generated.
  • The --skip-validate-spec option, which you can choose to omit, skips validation of the input specification. I use this when generating the nomad_client in Elixir (https://github.com/mrmstn/nomad_client).
  • --input-spec /local/openapi.yaml points the generator to the location of the specification file inside the container, corresponding to ./openapi.yaml in your current working directory.
  • --output /local/ is the location where the generated client will be placed. You can customize this by changing it to --output /local/sub/dir to suit your needs.

Please don't hesitate to reach out if you have any further questions or require additional clarifications.

@tomjoro
Copy link
Author

tomjoro commented Oct 12, 2023

We have tested on the latest master and This issue is resolved by #16061 . Thanks.

@tomjoro tomjoro closed this as completed Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants