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

HV-897 and HV-981 #402

Closed
wants to merge 4 commits into from
Closed

Conversation

hferentschik
Copy link
Contributor

Here come my take at the TimeProvider.

gunnarmorling and others added 4 commits April 21, 2015 16:25
* Avoiding some raw-type warnings
* Adding missing @SInCE tags
* Making jboss-logging-annotations referenceable in JavaDocs
- Using TimeProvider contract for validation of @past and @future constraints
- Allowing to set a specific time provider per validator
- Allowing to set time provider via validation.xml
- Updating reference guide
…lendar to long (epoch time in milliseconds)

- Marking TimeProvider as experimental
@gunnarmorling
Copy link
Member

You know my stance on this: the proposed change and esp. the removal done for HV-981 restricts the usefulness of the API IMHO for no good reason.

I simply don't see why it should not be supported to check whether e.g. LocalDate.of( 2015, 12, 03 ) is in the future or not. Note that by omitting the timezone information from the TimeProvider contract you also prevent users from providing custom constraints for this case. So at least I'd expose the TZ on that contract to allow for that.

Anyways, I have had my say, and I don't think we will come to an agreement. Feel free to move forward and apply the change. I just might ocassionally yell "Told you so" if users complain ;)

@hferentschik
Copy link
Contributor Author

I just might ocassionally yell "Told you so" if users complain ;)

I guess. On the other hand, I think this is also the most conservative approach. The TimePrivider contract can easily be extended and new constraint validator classes can be added. The other way around is harder. This way we can wait for feedback from users and potential BV spec discussions.

@vguna
Copy link

vguna commented Aug 23, 2016

I came across this while searching for the reason why LocalDate isn't working with @Past. My usecase is a birthdate. Why isn't it possible to check whether a birthdate is in the past? I'm with @gunnarmorling regarding this.

@gunnarmorling
Copy link
Member

@vguna Thanks for reporting back! In your case, how would you obtain the LocalDate representing "now" for doing the comparison?

@vguna
Copy link

vguna commented Aug 25, 2016

Call me naive but LocalDate.now() seems a good choice :)?

@gunnarmorling
Copy link
Member

gunnarmorling commented Aug 25, 2016 via email

@vguna
Copy link

vguna commented Aug 25, 2016

But that isn't different from Date or Calendar is it? They also use the date-time from the JVM. What else should/can they use? I think I'm not getting it :). Do you have a concrete example where this won't fit?

@yrodiere
Copy link
Member

By the way, using non-customizable date sources significantly impairs the ability to test application code, as you generally don't want your tests to rely on their time of execution.
That's one reason using new Date() is not really an awesome practice and some advice to define a dedicated service for getting the current date, so that they can easily be mocked.
It might be something to keep in mind when designing the actual validator implementations...

@vguna
Copy link

vguna commented Aug 25, 2016

If I write a testcase, that checks that birthdate is in the @Past, I would simply create a birthdate that is now()-n days to be valid. After 15 years of software development I never came across the need to mock JVM's or an application's "now" :). I don't think that's really a problem here...

@gunnarmorling
Copy link
Member

But that isn't different from Date or Calendar is it? They also use the date-time from the JVM. What else should/can they use?

Yes, that's what's currently provided by the BV spec, but we are thinking to make this more flexible for BV 2.0. As a matter of fact, HV already has a specific SPI for this, TimeProvider.

Do you have a concrete example where this won't fit?

Yes, see above:

use the local date for the currently logged in user in a server app with users in many countries

Others are batch jobs which may run with a "logical date" different from what's "now" on the JVM. E.g. re-running yesterday's invoicing batch etc. etc. Testing as said by @yrodiere is another one.

I never came across the need to mock JVM's or an application's "now"

Lucky you then :) Had it more than once. But I take it using the JVM date and TZ would be fine for your use case.

@gunnarmorling
Copy link
Member

some advice to define a dedicated service for getting the current date

Yes, we already do that in HV via TimeProvider :)

@vguna
Copy link

vguna commented Aug 25, 2016

Ah I see. Yeah, seems lucky me ;). Thanks for the clarification.

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

Successfully merging this pull request may close these issues.

4 participants