-
Notifications
You must be signed in to change notification settings - Fork 39
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 fromdateiso8601 and todateiso8601 #322
Conversation
bc71725
to
ff38724
Compare
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.
Thank you for the pull request! I added my review comments.
public void apply(final Scope scope, final List<Expression> args, final JsonNode in, final Path ipath, final PathOutput output, final Version version) throws JsonQueryException { | ||
Preconditions.checkInputType("fromdateiso8601", in, JsonNodeType.STRING); | ||
try { | ||
long epochSeconds = ZonedDateTime.parse(in.asText()).toEpochSecond(); |
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.
We need to specify the formatter here. It seems like the default formatter, ISO_ZONED_DATE_TIME, which is used by the parse method, accepts a wider variety of date-time formats than jq's fromdateiso8601
does.
Quoting from the Javadoc:
The ISO-like date-time formatter that formats or parses a date-time with offset and zone, such as '2011-12-03T10:15:30+01:00[Europe/Paris]'.
The section in square brackets is not part of the ISO-8601 standard.
For compatibility reasons, we should not accept such inputs, especially the ones that are not even ISO8601 compliant.
AFAICT, jq's fromdateiso8601
only accepts yyyy-MM-dd'T'HH:mm:ss'Z'
and so the formatter should be something like DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'")
.
|
||
@AutoService(Function.class) | ||
@BuiltinFunction({ "todateiso8601/0" }) | ||
public class ToDateIso8601Function extends StrFTimeFunction { |
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.
Could you refactor this class so that it does not depend on StrFTimeFunction, and move todateiso8601
and fromdateiso8601
to the core (= jackson-jq module)?
A bit of a history... strftime
was put in extras, because at the time the function was added, jq didn't have strftime
builtin. strftime
in jackson-jq-extras is not same as jq's strftime
and doesn't support libc strftime format specifiers like %Y-%m-%d
(instead it uses SimpleDateFormat). It's a bit of a headache because even though it's in extras module, I will have to come up with a proper migration/deprecation plan, considering some users are already relying on it.
So given the circumstances, it'd be better for new code to avoid depending on the current strftime
.
Support for built-in
fromdateiso8601
andtodateiso8601
.todateiso8601
is an alias forstrftime
, passed with an ISO 8601 compatible formatter,"%Y-%m-%dT%H:%M:%SZ"
.Some tests verifying this on my local machine:
And then a direct implementation of
fromdateiso8601
Added to extras since those base functions were defined there, but wasn't sure if it should be in the
jackson-jq
, instead.