-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: main
Are you sure you want to change the base?
Conversation
…Date when deserializing.
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. |
There was a problem hiding this 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.
// Include fall-back for "timeless" Date (could be from a java.sql.Date, for example) | ||
dateFormats.add(DateFormat.getDateInstance(dateStyle, Locale.US)); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
@eamonnmcmanus, do you think we should add the That pattern would then have to be enabled always, even if |
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: