swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.89k stars 6.03k forks source link

[JAVA] dateLibrary "java8-localdatetime" option is broken #6992

Open nemecec opened 6 years ago

nemecec commented 6 years ago
Description

Using dateLibrary option with java8-localdatetime value generates JSON.java with Gson type adapter for parsing LocalDate not LocalDateTime (as expected).

As a result, this option is useless for parsing date-time fields (the common scenario).

Swagger-codegen version

2.2.3

Command line used for generation

Options:

Suggestion for a fix

Add also LocalDateTime adapter. Both types should be supported (as they are supported by Swagger/OpenAPI). And LocalDateTime is the more useful type.

Workaround

In user code, create the correct type adapter:

  class LocalDateTimeTypeAdapter extends TypeAdapter<LocalDateTime> {

    private final DateTimeFormatter formatter = DateTimeFormatter.ISO_LOCAL_DATE_TIME;

    @Override
    public void write(JsonWriter out, LocalDateTime date) throws IOException {
      if (date == null) {
        out.nullValue();
      } else {
        out.value(formatter.format(date));
      }
    }

    @Override
    public LocalDateTime read(JsonReader in) throws IOException {
      switch (in.peek()) {
        case NULL:
          in.nextNull();
          return null;
        default:
          String date = in.nextString();
          return LocalDateTime.parse(date, formatter);
      }
    }
  }

And register it with Gson:

    apiClient.getJSON().setGson(
      new GsonBuilder().registerTypeAdapter(LocalDateTime.class, new LocalDateTimeTypeAdapter()).create());
wing328 commented 6 years ago

Can you please try 2.3.0 (latest master) to see if it's still an issue?

v2.3.0 SNAPSHOT can be found in the README.

nemecec commented 6 years ago

Looking at this file in master, I fail to see how this could possibly work correctly. https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/Java/libraries/okhttp-gson/JSON.mustache

In addition to (or instead of) LocalDateTypeAdapter, there should be also LocalDateTimeTypeAdapter.

nemecec commented 6 years ago

Same applies for https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/Java/libraries/retrofit2/JSON.mustache

Probably every implementation that uses Gson. Jackson is not affected, as that uses Jackson built-in module for java.time APIs.

cbornet commented 6 years ago

@nemecec note that LocalDateTime is not the type to use for swagger's date-time type. We only provided the option for backward compatibility and you should avoid using it if possible.

cbornet commented 6 years ago

Swagger's date-time follows rfc3339 format so it must have a timezone.

nemecec commented 6 years ago

Well, I have an API which is not under my control and I want to generate a client to consume that API. As such, I'm forced to use LocalDateTime, other options fail to parse the timestamp without timezone.

roberterdin commented 6 years ago

rfc3339 sais

The following profile of ISO 8601 [ISO8601] dates SHOULD be used in new protocols on the Internet.

Where SHOULD in an RFC means there may exist valid reasons in particular circumstances to ignore a particular item. While OffsetDateTime is probably the right default choice for swagger's date-time, LocalDateTime is semantically correct in some cases and should therefore be available as an option.

As an example, imagine you have a vessel travelling between different time zones (very common case in the shipping industry). If you want to express that something happens at a specific date and time depending on what time zone the vessel is in you can't use a time zone.

cbornet commented 6 years ago

In introduction :

All times expressed have a stated relationship (offset) to Coordinated Universal Time (UTC). (This is distinct from some usage in scheduling applications where a local time and location may be known, but the actual relationship to UTC may be dependent on the unknown or unknowable actions of politicians or administrators. The UTC time corresponding to 17:00 on 23rd March 2005 in New York may depend on administrative decisions about daylight savings time. This specification steers well clear of such considerations.)

Don't try to bend the thing... :wink:

As an example, imagine you have a vessel travelling between different time zones (very common case in the shipping industry). If you want to express that something happens at a specific date and time depending on what time zone the vessel is in you can't use a time zone.

There are use cases for localdatetime (generally you also need a time zone id). RFC3339 cannot be used for these. Remember it's an interoperability format !

roberterdin commented 6 years ago

You're right.. I didn't read the entire RFC, sorry for that.
The inability to express a local date-time or at least the ability to express a time so you can construct it with a date and a time field is a shortcoming of the Open API specification and not of swagger-codegen. Adding a time field is already being discussed in the repo for the Open API specification btw.

cbornet commented 6 years ago

I think we have the right to define our own type formats. It could be interesting to support a local-date-time one.

roberterdin commented 6 years ago

This should be allowed according to the specification, as far as I can tell.

Formats such as "email", "uuid", and so on, MAY be used even though undefined by this specification.

There is WIP regarding custom type formats, see this issue.

I think support for two custom formats, i.e. something like time (partial-time in the RFC) and local-date-time would be quite useful. I could probably find somebody to implement it or make some time myself.

aveuiller commented 3 years ago

Hello,

I will continue with this issue as the underlying problem is the same, tell me if you'd prefer a new issue. :)

Description

I have the same issue as the original post. However, it seems that setting Gson inside ApiClient is not possible anymore since a JSON object is instantiated on call to createDefaultAdapter.

The possibility to add a Converter.Factory on the AdapterBuilder object does not solve the issue either as it seems to not be able to override the conversion behavior (probably because of the Class type adapter on the original converter). I created a minimalist test case to showcase the issue

Swagger-codegen version 5.1.0

Suggestion for a fix

Either:

Workaround

None at the moment.

EDIT: Just saw that a partial-time data type has been implemented to fix this. As far as I can see, it's not a WIP anymore. EDIT2: Talked too soon about the above, it is treating the date as a String.