Formatters - add java8 java.time.LocalDate support for bindings

[svaens]

This introduces support of LocalDate, in the way of a formatter for that type, for form binding.

Version 2.4 of playframework claimed java 8 types support. LocalDate is one of the types not yet supported.

Pull request provided in response lightbend discussion
https://discuss.lightbend.com/t/form-binding-support-for-java-8-type-localdate/852
Tests added to existing test class: framework/src/play-java-forms/src/test/scala/play/data/format/FormattersTest.java

[svaens]

@mkurz - I think this is ready for review. I have looked through the checklist, and believe all requirements to be fulfilled. As this is my first -albeit minor - contribution, I may have left something out. Please let me know if you see something missing. Thanks.

[mkurz]

Thanks! I will have a look until tomorrow.

[svaens]

@cchantep I didn't want to make too much change on my first pull-request, but it seems to me that the code would benefit from refactoring out the various classes/annotations from the 'Formats' class into their own classes. Perhaps making a 'formats' package instead.
Do you, agree?
Should it be done in this issue (since I am touching/expanding the Formats.java file already)?
Or leave the classes in the Formats.java file as currently is the case?

[cchantep]

More refactoring in other PR I'd say

[svaens]

@cchantep
Being my first pull request to playframework, I wasn't sure who decides to merge this into the main repo, and when that might be done. I ask now (maybe too late) because I read from the "Discuss Lightbend summary" email that the first (RC3) release candidate is available. So I guess this won't be included in the 2.7.0.
Can you enlighten me please?

[mkurz]

I think we can still add this to Play 2.7.
I will have a look at it.

[mkurz]

Note from a Gitter conversation with @marcospereira:

I'm thinking if this is something we need to have on our side, or if using DefaultFormattingConversionService from Spring is good enough.
In such case, we need to change it here and have proper tests.

[mkurz]

@marcospereira Referring your suggestion:

I'm thinking if this is something we need to have on our side, or if using DefaultFormattingConversionService from Spring is good enough.
In such case, we need to change it here and have proper tests.

I tried to make use of the spring provided formatters, converters, etc. like you suggested. However that won't work for Play - because it has it's own approach for parsing and formatting.

Let's go through that step-by-step for the LocalDate example:
If we would replace FormattingConversionService with DefaultFormattingConversionService it would call addDefaultConverters which would register formatters for the date types (and others of course). Here for example it sets up the formatter for the LocalDate type. And here starts the problem: The DateTimeFormatter will be set here, at registration/creation time of the TemporalAccessorParser. Now registration setup is completed. The real problem happens later when parsing actually occurs: The DateTimeFormatter set during initialisation will be used and there is no way to use another one. (What DateTimeContextHolder does here is to set the given locale onto the DateTimeFormatter, however that doesn't really matter for us). But because in Play users are able to define a pattern in conf/messages we would need to be able to set the pattern onto that used formatter for each parsing call. And that's not possible. In our Play provided formatters we actually do create a dateformatter with the pattern read from the messages for each parsing call.

Therefore I don't think we can simply re-use the spring formatters.

(Another note: We may would run into classloader issues when directly using DefaultFormattingConversionService).

[mkurz]

BTW: I will push some updates later to that pull request when I have time.

[balane]

Any chance this will get merged sometime soon?

[marcospereira]

@svaens and @mkurz this is now rebased against the master branch. I think it is good to be merged.

In our Play provided formatters we actually do create a dateformatter with the pattern read from the messages for each parsing call.

Looks like this is something we can optimize later, but it is beyond the scope of this PR.

@svaens thanks for contributing and sorry for taking too long to move this forward.

[mkurz]

I will review when I find more time, thanks Marcos!

[an-tex]

@mkurz ping, could have just used that ;)

[mkurz]

@an-tex You can easily implement this yourself for now: https://www.playframework.com/documentation/2.8.x/JavaForms#Register-a-custom-DataBinder

[an-tex]

yep thanks I did just that ;)

  implicit object queryStringLocalDateBinder extends QueryStringBindable.Parsing[LocalDate](
    dateString => LocalDate.parse(dateString),
    _.toString,
    (key: String, e: Exception) => "Cannot parse parameter %s as java.time.LocalDate: %s".format(key, e.getMessage)
  )

thought to give you a ping nevertheless about this open pull request