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

Should not parse LocalDates from number (timestamp) -- or at least should have an option preventing #58

Closed
billoneil opened this issue Mar 7, 2018 · 11 comments
Milestone

Comments

@billoneil
Copy link

In Jackson 2.8.x I had a unit test that parsed the following and expected an exception.

{
  "date": 123,
  "message": "Happy New Year!"
}

As of 2.9 related to this commit https://github.com/FasterXML/jackson-modules-java8/pull/22/files#diff-a1552641fab3c9690f2c8b0f5ee2fe89R102 it now parses as 1970-05-04

Is there a way to turn this off if I expect my dates to come in as a different format? I don't want to parse random integers as dates when it should be invalid input.

@cowtowncoder
Copy link
Member

No mechanism to prevent that currently; integers are accepted by default for all date/time types (so change for 2.9, if it didn't work on 2.8) is more due to unifying handling than otherwise intended change.

But come to think of it now, it is not actually clear that numbers should work for LocalDate, given that it is pure date value with no canonical numeric value without timezone.
So perhaps it is something that should be blocked in 3.0. Since 2.9.x are already out, behavior can not be changed in a patch however.

As to blocking value you will need to register a custom deserializer that does this.

@billoneil
Copy link
Author

Just thought I would mention the change in behavior to see if it was intended or not. I can work around it on my end if needed. Feel free to close if you don't plan on changing the behavior.

@cowtowncoder cowtowncoder changed the title Is there a way to not parse LocalDates as days from epoch? Is there a way to not parse LocalDates as number (timestamp)? Mar 7, 2018
@cowtowncoder
Copy link
Member

@billoneil Looks like this was due to #20 actually, so it was intentional. Although perhaps bit misguided, considering the part.

However! I do have one possibly useful idea wrt 2.9.x, your specific use case: there is property lenient for @JsonFormat; and that defaults to true, but may be configured differently at 3 levels: global default, per-type (LocalDate) and per-property.
I don't think this is yet supported by this particular deserializer, but it could be; and I think it would make sense... especially if coupled with format Shape for deserialization (not just serialization).

It is some work but it seems to me like a good way to do this -- my intent going forward is to limit number of on/off feature flags that are type-specific, and @JsonFormat (along with Config Overrides that allow specifying overrides for types and global default(s)).

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label Aug 13, 2019
@cowtowncoder cowtowncoder changed the title Is there a way to not parse LocalDates as number (timestamp)? Should not parse LocalDates from number (timestamp) Aug 13, 2019
@cowtowncoder cowtowncoder changed the title Should not parse LocalDates from number (timestamp) Should not parse LocalDates from number (timestamp) -- or at least should have an option preventing Aug 13, 2019
@cowtowncoder cowtowncoder added 2.10 and removed good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project labels Aug 13, 2019
@cowtowncoder
Copy link
Member

I think that there should be a way to prevent this, but I am not sure what the correct path would be.
A new module-configuration feature is a possibility, I suppose.

@kupci
Copy link
Member

kupci commented Sep 19, 2019

I think that there should be a way to prevent this, but I am not sure what the correct path would be.
A new module-configuration feature is a possibility, I suppose.

What about the format Shape idea mentioned previously?

there is property lenient for @jsonformat; [snip] _ and I think it would make sense... especially if coupled with format Shape for deserialization (not just serialization)._

@cowtowncoder
Copy link
Member

@kupci I go back and forth here, since for many date/time values integer representations ("timestamps") seem like a legit use case.

But for specific case of LocalDate (not even LocalDateTime which... one could argue could be a timestamp), number of days is sort of esoteric.

So, yes, if you could do a patch that blocks this if strict (not lenient) that'd be good.
Actually, one twist would be that if Shape of NUMBER or NUMBER_INT is specified, it should of course be accepted (but, I suppose, STRING not?).

Now, this would be great to get in 2.10 -- time is getting bit tight, release by 25th. :)
I'll give this issue priority if need be.

@kupci
Copy link
Member

kupci commented Sep 20, 2019

It turns out this bug is already partially fixed. The leniency check is already in the LocalDateDeserializer:

        // 06-Jan-2018, tatu: Is this actually safe? Do users expect such coercion?
        if (parser.hasToken(JsonToken.VALUE_NUMBER_INT)) {
            if (!isLenient()) {
                return _failForNotLenient(parser, context, JsonToken.VALUE_STRING);
            }
            return LocalDate.ofEpochDay(parser.getLongValue());
        }

As for the Shape of NUMBER_INT requirement, that still needs to be implemented in the Deserializer. Based on the code review, it looks like that was the original intention but it somehow only got to the LocalDateSerializer. Oh, and some documentation and tests of course.

Now one question - for the Shape, I see the constructor signatures were changed to add that setting, which of course I can follow for the Deserializer, maybe just LocalDateDeserializer as it doesn't need to be any higher at this point. However, I also notice that in the Serializer hierarchy, it is accessible already, as you can see it retrieved in the JSR310FormattedSerialzerBase.createContextual method:

       JsonFormat.Value format = findFormatOverrides(prov, property, handledType());
[snip]
            JsonFormat.Shape shape = format.getShape();

So maybe that's a cleaner way to go, but I don't see that same capability in the Deserializer hierarchy.

@cowtowncoder
Copy link
Member

I agree that it would probably make sense to add a shared helper method in base class for deserializers too. I do not remember the history here, but in the past serializers only were affected (to select one of many representations), whereas deserializers were always "lenient" thereby not caring too much about intended shape. At least for datatypes; there may have been some exceptions.

So probably it just wasn't felt it was needed. But now is.

@kupci
Copy link
Member

kupci commented Sep 22, 2019

It's there (the findFormatOverrides, in StdDeserializer), not sure what I was thinking. That makes things easier. But I think I'll finish up #138 and then come back to this one, since at least it does have the leniency check.

@cowtowncoder
Copy link
Member

@kupci Sounds good. I'm happy with the progress made here -- thank you very much for your fixes.
I think I could give you commit access to this module soon: I'll send an email about that.

@cowtowncoder
Copy link
Member

Quick note: due to 2.10.0 release, now needs to go to 2.11 branch for 2.11.0, as it is behavioral change and could break legit usage: something to avoid in a patch.

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

No branches or pull requests

3 participants