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

Enable ResolverStyle.STRICT for java formatters #46675

Merged
merged 26 commits into from
Oct 11, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Sep 12, 2019

Joda was using ResolverStyle.STRICT when parsing. This means that date will be validated to be a correct year, year-of-month, day-of-month
However, we also want to make it works with Year-Of-Era as Joda used to, hence custom temporalquery.localdate in DateFormatters.from
Within DateFormatters we use the correct uuuu year instead of yyyy year of era

worth noting: if yyyy(without an era) is used in code, the parsing result will be a TemporalAccessor which will fail to be converted into LocalDate. We mostly use DateFormatters.from so this takes care of this. If possible the uuuu format should be used.

@pgomulka pgomulka added the :Core/Infra/Core Core issues without another label label Sep 12, 2019
@pgomulka pgomulka self-assigned this Sep 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka pgomulka added the WIP label Sep 12, 2019
@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka
Copy link
Contributor Author

Ok to test

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

One small comment, but my larger concern is adding leniency for yyyy. I think we should fix our tests, and enforce the deprecation we have already made for yyyy, where we suggested to users changing to uuuu.


JavaDateMathParser(String format, JavaDateFormatter formatter, DateTimeFormatter roundUpFormatter) {
JavaDateMathParser(String format, JavaDateFormatter formatter, JavaDateFormatter roundupParser2) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the old name was better?

@elastic elastic deleted a comment from elasticmachine Sep 27, 2019
@pgomulka
Copy link
Contributor Author

pgomulka commented Sep 30, 2019

One small comment, but my larger concern is adding leniency for yyyy. I think we should fix our tests, and enforce the deprecation we have already made for yyyy, where we suggested to users changing to uuuu.

@rjernst I am fully supportive of this. I wonder if this should not be in a separate PR thought. there is >2000 usages of yyyy in our code base. A lot of this is in docs (which is part of the testing anyway). Shouldn't be too hard to fix, yet it would hide the purpose of this PR.
wdyt?

@pgomulka pgomulka marked this pull request as ready for review September 30, 2019 14:55
@pgomulka pgomulka added v8.0.0 and removed WIP labels Sep 30, 2019
@rjernst
Copy link
Member

rjernst commented Sep 30, 2019

Sure, a separate PR (or multiple) is fine.

@pgomulka pgomulka requested a review from rjernst October 10, 2019 05:02
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@pgomulka pgomulka merged commit da44943 into elastic:master Oct 11, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Oct 11, 2019
Joda was using ResolverStyle.STRICT when parsing. This means that date will be validated to be a correct year, year-of-month, day-of-month
However, we also want to make it works with Year-Of-Era as Joda used to, hence custom temporalquery.localdate in DateFormatters.from
Within DateFormatters we use the correct uuuu year instead of yyyy year of era

worth noting: if yyyy(without an era) is used in code, the parsing result will be a TemporalAccessor which will fail to be converted into LocalDate. We mostly use DateFormatters.from so this takes care of this. If possible the uuuu format should be used.
pgomulka added a commit that referenced this pull request Oct 11, 2019
#47913)

Joda was using ResolverStyle.STRICT when parsing. This means that date will be validated to be a correct year, year-of-month, day-of-month
However, we also want to make it works with Year-Of-Era as Joda used to, hence custom temporalquery.localdate in DateFormatters.from
Within DateFormatters we use the correct uuuu year instead of yyyy year of era

worth noting: if yyyy(without an era) is used in code, the parsing result will be a TemporalAccessor which will fail to be converted into LocalDate. We mostly use DateFormatters.from so this takes care of this. If possible the uuuu format should be used.
howardhuanghua pushed a commit to TencentCloudES/elasticsearch that referenced this pull request Oct 14, 2019
Joda was using ResolverStyle.STRICT when parsing. This means that date will be validated to be a correct year, year-of-month, day-of-month
However, we also want to make it works with Year-Of-Era as Joda used to, hence custom temporalquery.localdate in DateFormatters.from
Within DateFormatters we use the correct uuuu year instead of yyyy year of era

worth noting: if yyyy(without an era) is used in code, the parsing result will be a TemporalAccessor which will fail to be converted into LocalDate. We mostly use DateFormatters.from so this takes care of this. If possible the uuuu format should be used.
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Dec 11, 2019
A recent change (elastic#46675) introduced stricter date parsing. We should now also
catch DateTimeExceptions in DateFieldMapper and ignore those when the
`ignore_malformed` option is set. Also adding an additional test that would have
caught this.

Closes elastic#50081
cbuescher pushed a commit that referenced this pull request Dec 13, 2019
A recent change around date parsing (#46675) made it stricter, so we should now also
catch DateTimeExceptions in DateFieldMapper and ignore those when the
`ignore_malformed` option is set.

Closes #50081
cbuescher pushed a commit that referenced this pull request Dec 13, 2019
A recent change around date parsing (#46675) made it stricter, so we should now also
catch DateTimeExceptions in DateFieldMapper and ignore those when the
`ignore_malformed` option is set.

Closes #50081
cbuescher pushed a commit that referenced this pull request Dec 13, 2019
A recent change around date parsing (#46675) made it stricter, so we should now also
catch DateTimeExceptions in DateFieldMapper and ignore those when the
`ignore_malformed` option is set.

Closes #50081
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
A recent change around date parsing (elastic#46675) made it stricter, so we should now also
catch DateTimeExceptions in DateFieldMapper and ignore those when the
`ignore_malformed` option is set.

Closes elastic#50081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants