Skip to content

Allow multiple date formats for date fields #1728

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 17, 2021
Merged

Allow multiple date formats for date fields #1728

merged 1 commit into from
Mar 17, 2021

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented Mar 15, 2021

Closes #1727

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our issue tracker.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@xhaggi xhaggi requested a review from sothawo March 15, 2021 12:44
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch from 830ab7e to 9341906 Compare March 15, 2021 12:46
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch 2 times, most recently from cb7434a to b72ebd0 Compare March 15, 2021 13:01
@xhaggi xhaggi removed the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
sothawo
sothawo requested changes Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

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

long time no see.
Basically one thing: deprecate none instead of removing it, this is a breaking change

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@sothawo sothawo removed the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch from b72ebd0 to 9dabc62 Compare March 15, 2021 13:46
@xhaggi xhaggi requested a review from sothawo March 15, 2021 13:48
sothawo
sothawo requested changes Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

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

can you please adapt the documentation in https://github.com/spring-projects/spring-data-elasticsearch/blob/master/src/main/asciidoc/reference/elasticsearch-object-mapping.adoc, lines 67++ to this change?
Otherwise only cosmetic changes

@sothawo sothawo removed the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 15, 2021
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch 2 times, most recently from f4e47bb to dbc4ce9 Compare March 16, 2021 11:08
@xhaggi xhaggi requested a review from sothawo March 16, 2021 11:08
@xhaggi
Copy link
Contributor Author

xhaggi commented Mar 16, 2021

@sothawo I have addressed all of your comments and force-pushed the changes.

@sothawo sothawo removed the status: waiting-for-triage An issue we've not yet triaged label Mar 16, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 16, 2021
sothawo
sothawo requested changes Mar 16, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

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

some remarks, and: there are no tests that test that the correct mapping is built for multiple formats

...g/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java Outdated
Comment on lines 216 to 221
for (ElasticsearchDateConverter dateConverter : dateConverters) {
try {
return dateConverter.parse(s, (Class<? extends TemporalAccessor>) actualType);
} catch (Exception e) {
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

after the loop, if every registered converter fails, we should throw an error that none of the defined date formats could be used to read the value

...g/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java Outdated
Comment on lines 223 to 216
for (ElasticsearchDateConverter dateConverter : dateConverters) {
try {
return dateConverter.parse(s);
} catch (Exception e) {
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, when all converters fail, we must error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done for both at the end of the method. Why should we do it twice?

@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch from dbc4ce9 to 7bac792 Compare March 17, 2021 08:13
@xhaggi
Copy link
Contributor Author

xhaggi commented Mar 17, 2021

@sothawo I've done some more changes that in my opinion makes things a bit easier. Now it is no longer necessary to set DateFormat.custom to use Custom Date Formats. If one or more values are set in pattern they are used without checking for DateFormat.custom. This way DateFormat.custom is deprecated and can be removed in a future version, as well as none. I have adjusted the documentation so that it is clear that you must set the format property to empty {} if you want to use only custom date formats.

@xhaggi xhaggi requested a review from sothawo March 17, 2021 08:26
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch from 7bac792 to 2d907ff Compare March 17, 2021 08:33
@xhaggi xhaggi force-pushed the feature/multiple-date-formats branch from 2d907ff to dada519 Compare March 17, 2021 09:06
sothawo
sothawo approved these changes Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

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

I'll change the thrown exception and add more documentation and test in a following polishing commit.

@sothawo sothawo merged commit 31b488d into spring-projects:master Mar 17, 2021
@xhaggi
Copy link
Contributor Author

xhaggi commented Mar 18, 2021

👍 thank you

@xhaggi xhaggi deleted the feature/multiple-date-formats branch June 23, 2021 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sothawo sothawo sothawo approved these changes

Assignees
No one assigned
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow multiple date formats for date fields
3 participants
@xhaggi @sothawo @spring-projects-issues