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

Validatable.validatesDateOf() replacing exception thrown for invalid dates. #718

Closed
wants to merge 1 commit into from

Conversation

hotaru355
Copy link
Contributor

@hotaru355 hotaru355 commented Sep 14, 2015

Please review this PR that fixes issue #592, but also takes a broader approach on how loopback handles invalid dates.

  • Current behavior - Making a POST request or instantiating a model with the following data throws an exception:
{
  "createdAt": "2015-13-13"
}

I would like to propose changing this behavior since it allows any client to crash the server with invalid data. Unfortunately, trying to catch the error on the server is not possible by adding custom validation methods, since parsing the data into a model (and throwing the exception) happens before any validation method is called. The only feasible solution I see for this problem is to implement a before save hook on every model that contains a date field or maybe implement some middleware to handle date field errors (? not tested).

  • Proposed behavior - Providing a validatesDateOf() method which is added to any date field by default could be the cleaner solution. Making a request to the REST API with an invalid date field triggers a proper validation error as a client would expect it. Instantiating a model with an invalid date would not throw an exception, but calling isValid() on it would return false, without having to add the validation method explicitly. Removing the default validation method can easily be done if so desired.

…emove throwing exception when parsing invalid date into model
@raymondfeng
Copy link
Contributor

@hotaru355 Thanks for the patch. Enforcing date value check as part of the validation is nice. What I'm wondering is what expectation a developer has against the following code:

p1 = new Person({name: 'John', dob: 'X'});

Should we throw from the constructor as dob property has an invalid value?

@dhmlau
Copy link
Member

dhmlau commented Apr 7, 2017

@hotaru355 Thanks for the PR. Sorry that it's been a while we last looked at it. It's good to include your changes. Could you please rebase? Thanks!

@slnode
Copy link

slnode commented Apr 7, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link

slnode commented Apr 7, 2017

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Apr 7, 2017

Can one of the admins verify this patch?

@dhmlau dhmlau added blocked and removed in-progress labels Apr 7, 2017
@dhmlau
Copy link
Member

dhmlau commented Apr 10, 2017

@hotaru355, I actually rebase it for you. Could you please grant me access to your fork? Thanks!
https://github.com/blog/2247-improving-collaboration-with-forks

@dhmlau
Copy link
Member

dhmlau commented Apr 17, 2017

@hotaru355, your changes look good to us after rebasing and changing some test cases. Could you please grant me access to your fork (see instructions in my previous post)? Please respond by this Friday (April 21), otherwise we'd have to unfortunately close the PR. Thanks!

@dhmlau
Copy link
Member

dhmlau commented Apr 21, 2017

I've created another PR for the changes suggested here and fixed the test cases in #1339.
Closing this.

@dhmlau dhmlau closed this Apr 21, 2017
@dhmlau dhmlau added this to the Sprint 34 - Apex milestone Apr 21, 2017
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.