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

Add fall-back support for "timeless" java.util.Date deserialization in DefaultDateTypeAdapter #2669

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

Conversation

MuffinTheMan
Copy link

Purpose

Adds support for a deserialization scenario where a "timeless" java.util.Date was serialized like "Jan 1, 1970" (Gson's default behavior) but cannot then be deserialized. It seems to make sense that Gson's deserializer should be able to handle deserializing what it serialized itself by default.

Description

This example demonstrates the issue:

@Test
void testSqlDateToUtilDate() throws SQLException {
    try (
        Connection c = ConnectionFactory.getConnection();
        Statement stmt = c.createStatement();
        ResultSet rs = stmt.executeQuery("SELECT '1970-01-01';")
    ) {
        rs.next();
        // rs::getDate() returns java.sql.Date, but that *is a* java.util.Date
        final java.util.Date date = rs.getDate(1);
        final Gson gson = new Gson();
        // This will fail without the fix, because it's serialized as "Jan 1, 1970" but unable
        // to be deserialized from that format. Since adding support for this is trivial, it
        // seems worthwhile. 👇
        assertThat(gson.fromJson(gson.toJson(date), java.util.Date.class)).isEqualTo(date);

        // On the other hand, this does work; however the declared type of `date` is `java.util.Date`,
        // so it would be helpful if it could be deserialized with that same class. The "realistic" scenario
        // is that you have a java.util.Date field on a class that is being populated from a DATE database
        // column (ie using `ResultSet::getDate()` like above); if the class is serialized/deserialized, it
        // will fail deserialization without this fix. 👇
        assertThat(gson.fromJson(gson.toJson(date), java.sql.Date.class)).isEqualTo(date);
    }
}

Copy link

google-cla bot commented Apr 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request!

At least personally I am not sure if this is a change which should be made (but I am not a direct member of this project). In general the whole human-readable date formatting in JSON is not ideal, see for example #2472.

Maybe here it would be better if you registered a custom adapter for java.util.Date and / or java.sql.Date in your code to handle this. But I am not completely sure what the best approach here would be.

Comment on lines +137 to +138
// Include fall-back for "timeless" Date (could be from a java.sql.Date, for example)
dateFormats.add(DateFormat.getDateInstance(dateStyle, Locale.US));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a bit brittle for the use case you are describing because the java.sql.Date adapter actually uses a hardcoded pattern (with default locale) and not a date style:

private final DateFormat format = new SimpleDateFormat("MMM d, yyyy");

Copy link
Author

Choose a reason for hiding this comment

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

This is certainly interesting that it's a hardcoded SimpleDateFormat in the SQL adapter.

Regarding your overall point (being uncertain a change like I'm proposing should be made at all), the logic in DefaultDateTypeAdapter::deserializeToDate() is already built to try a couple different methods of parsing, so I don't see much harm in adding one more attempt (though I can see a slippery slope rebuttal that isn't necessarily invalid). If I write a custom TypeAdapter (which is currently my workaround), I have to basically copy the exact DefaultDateTypeAdapter and make this one change, which seems a bit silly (but maybe it's still the right answer 🤷‍♂️) for what seems like a pretty valid use-case (see my example code in the PR description). My other alternative is to update classes where fields are java.util.Date to be explicitly java.sql.Date where applicable, but this isn't realistic (if we could do this easily, we'd be switching from Date to something like LocalDate anyway).

Copy link
Collaborator

@Marcono1234 Marcono1234 Apr 26, 2024

Choose a reason for hiding this comment

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

I have to basically copy the exact DefaultDateTypeAdapter and make this one change

You can avoid this by writing a TypeAdapterFactory which first tries to deserialize as java.util.Date and if that fails tries java.sql.Date instead:

DateSqlFallbackFactory (click to expand)

Note that I haven't tested this extensively, and this implementation is also quite verbose. And if parsing fails, the JSON path in the exception message will not be that helpful, saying $ (root element) instead of the actual path.

class DateSqlFallbackFactory implements TypeAdapterFactory {
  @Override
  public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
    if (type.getRawType() != java.util.Date.class) {
      return null;
    }

    TypeAdapter<JsonElement> jsonElementAdapter = gson.getAdapter(JsonElement.class);
    TypeAdapter<Date> utilDateAdapter = gson.getDelegateAdapter(this, TypeToken.get(Date.class));
    TypeAdapter<java.sql.Date> sqlDateAdapter = gson.getAdapter(java.sql.Date.class);

    @SuppressWarnings("unchecked")
    TypeAdapter<T> adapter = (TypeAdapter<T>) new TypeAdapter<Date>() {
      @Override
      public Date read(JsonReader in) throws IOException {
        // First read as JsonElement tree to be able to parse it twice (once as java.util.Date,
        // once as java.sql.Date) if necessary
        JsonElement json = jsonElementAdapter.read(in);
        try {
          return utilDateAdapter.fromJsonTree(json);
        }
        // Note: This makes assumptions about the exception type thrown by Gson's built-in
        // adapter for java.util.Date
        catch (JsonParseException e) {
          try {
            return sqlDateAdapter.fromJsonTree(json);
          } catch (Exception suppressed) {
            e.addSuppressed(suppressed);
            throw e;
          }
        }
      }

      @Override
      public void write(JsonWriter out, Date value) throws IOException {
        utilDateAdapter.write(out, value);
      }
    };
    return adapter;
  }
}

If you haven't stored the JSON data for any java.util.Date and SQL subclasses persistently yet, then maybe a reliable alternative would be to use GsonBuilder.setDateFormat(String) to specify a locale insensitive pattern (if possible).

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Apr 26, 2024

@eamonnmcmanus, do you think we should add the java.sql.Date pattern (new SimpleDateFormat("MMM d, yyyy")) as fallback for deserialization to DefaultDateTypeAdapter? Similar / instead of what is proposed currently by this pull request, see discussion above.

That pattern would then have to be enabled always, even if SqlTypesSupport.SUPPORTS_SQL_TYPES is false, in case users start relying on this pattern for deserialization even when not using java.sql.Date, to avoid failures when those users run on a JDK without SQL types.

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.

2 participants