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

POC: Implement an Object validator #24

Merged
merged 1 commit into from Aug 2, 2016

Conversation

yanfali
Copy link
Contributor

@yanfali yanfali commented Jul 15, 2016

Issue #23 Proof of concept

Create an "object" Validator which leverages leverages Schema validation to validate a sub document using Schema itself.

Goal is to allow you to creation arrays of objects which have their fields validated correctly.

This needs more tests and work, but I wanted to get your feedback @rs on this general approach, or if you have thoughts on a better way to do this.

@yanfali
Copy link
Contributor Author

yanfali commented Jul 15, 2016

Travis failed, but it looks like it's errors in the JWT implementation and not related to this PR.

@yanfali
Copy link
Contributor Author

yanfali commented Jul 18, 2016

@rs feedback on the direction of this PR would be appreciated. I'm holding off on JSON Schema stuff because I could use something like this to implement "items" of arrays. Thanks

@rs
Copy link
Owner

rs commented Jul 21, 2016

I’m on vacation with limited access to internet. I will look at it asap.

@yanfali
Copy link
Contributor Author

yanfali commented Jul 21, 2016

Thanks @rs I assumed you were on vacation so was being patient.

@yanfali yanfali closed this Jul 21, 2016
@yanfali yanfali reopened this Jul 21, 2016
@rs
Copy link
Owner

rs commented Jul 27, 2016

JWT usage is fixed in master. Please rebase.

@yanfali yanfali force-pushed the yanfali-add-object-validator branch from c2ee188 to 1f2820a Compare July 28, 2016 22:31
@yanfali
Copy link
Contributor Author

yanfali commented Jul 28, 2016

Weird, now it's triggering --- FAIL: TestRoutePathParentsExists (0.00s) errors :(

@yanfali yanfali force-pushed the yanfali-add-object-validator branch from 1f2820a to 500a9a5 Compare August 1, 2016 16:35
@yanfali
Copy link
Contributor Author

yanfali commented Aug 1, 2016

rebased. Thanks

@rs
Copy link
Owner

rs commented Aug 1, 2016

By reading the code, I'm not sure to see how the errors would be formatted with sub-fields. Don't you think it could be a good idea to add some tests on the format of the error in different context?

@yanfali
Copy link
Contributor Author

yanfali commented Aug 1, 2016

Good point. I will do that. Thanks for the feedback.

@yanfali yanfali force-pushed the yanfali-add-object-validator branch from 500a9a5 to 9053b79 Compare August 2, 2016 01:59
@yanfali
Copy link
Contributor Author

yanfali commented Aug 2, 2016

OK, I've added 3 more tests. Because the error messages are a map, go makes the ordering that comes back non-deterministic, so I'm using assert.Contains to test for multiple values detected as bad. I made an array test but am only testing 1 bad value in the array because Array will stop executing after the first value that does not validate.

I also improved the error message a little to make it more readable. 1 thing I noticed which I may send a PR in for, is that Bool has inconsistent casing for it's error message. Everything else reports lower case except bool.go

Thanks.

@rs
Copy link
Owner

rs commented Aug 2, 2016

Boolean does always take a capital as it is coming from a proper noun.

http://english.stackexchange.com/questions/4481/should-the-word-boolean-be-capitalized

if !ok {
return nil, errors.New("not a dict")
}
fmt.Printf("%+v", value)
Copy link
Owner

Choose a reason for hiding this comment

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

Is it a debug leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep thinko. I'll remove it.

@rs
Copy link
Owner

rs commented Aug 2, 2016

What about something like that to ease testing: https://play.golang.org/p/Dn5htsZKSv

@yanfali
Copy link
Contributor Author

yanfali commented Aug 2, 2016

This looks good. I was avoiding making the Error() function more complex, but if you're ok with a sort and the extra garbage I'll use your implementation. Thanks!

@yanfali yanfali force-pushed the yanfali-add-object-validator branch 2 times, most recently from 7538bd2 to 1c489a1 Compare August 2, 2016 16:47
@yanfali
Copy link
Contributor Author

yanfali commented Aug 2, 2016

OK, rebased, removed thinko and refactored ErrorMap and tests to be like much nicer playground example. Thanks for all the feedback!

@yanfali yanfali force-pushed the yanfali-add-object-validator branch from 1c489a1 to c9306dc Compare August 2, 2016 16:55
@yanfali
Copy link
Contributor Author

yanfali commented Aug 2, 2016

OK, I spotted an unnecessary allocation and got rid of it in the error path.

@yanfali yanfali force-pushed the yanfali-add-object-validator branch from c9306dc to ba19506 Compare August 2, 2016 17:07
- leverages Schema validation to validate a sub document using Schema
- allows you to create arrays of objects which have their fields
  validated correctly.
- add more tests for output of error map
- ensure ErrorMap.Error() output is deterministic
- simplify ErrorMap implementation (rs)
@yanfali yanfali force-pushed the yanfali-add-object-validator branch from ba19506 to 88aab5e Compare August 2, 2016 17:09
@yanfali
Copy link
Contributor Author

yanfali commented Aug 2, 2016

Fixed an ordering issue in arguments to assert.Equal

@rs
Copy link
Owner

rs commented Aug 2, 2016

Thx @yanfali you're awesome

@rs rs merged commit ba0a0ab into rs:master Aug 2, 2016
@yanfali yanfali deleted the yanfali-add-object-validator branch August 2, 2016 17:20
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.

None yet

2 participants