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

[BC break] Custom validation with context + smaller fixes #123

Merged

Conversation

annismckenzie
Copy link
Contributor

Smaller fixes

Feature enhancement

I enhanced the custom type validators so their validation functions get the object being validated passed as a second argument. It is a BC break but the changes necessary are mechanical in nature (see the updated unit test for an example on how this can be used).

This is not finished yet (working on fixing #116) but I wanted to put this out there in order to ask you about the BC break: is that even an option? I thought about doing it in a backwards-compatible way but that would involve reflection. Variadic parameters aren't an option either. The only other way would be to wrap the value to validate and the object being validated in a custom object. That would make it more future-proof and the custom type validation function signature could be changed so as to not require passing an empty interface. Still, that also breaks BC or worse, breaks all current usages (because the custom validation wrapper object is also an interface{}).


This change is Reviewable

@annismckenzie annismckenzie force-pushed the custom_validation_with_context branch from f6c30e9 to 13f9f00 Compare May 3, 2016 13:44
@annismckenzie annismckenzie changed the title [BC break, WIP] Custom validation with context + smaller fixes [BC break] Custom validation with context + smaller fixes May 3, 2016
@annismckenzie
Copy link
Contributor Author

Alright, this is now done. I took care of #118 and fixed #116. Unfortunately, the BC break was unavoidable – the only good thing is that projects updating the library after this is merged will get broken builds (bear with me here…) and the fix is mechanical in nature: just update the function signature from

  // old signature
  func(i interface{}) bool

to

  // new signature
  func(i interface{}, o interface{}) bool

and you're done.

I added a few tests that were missing but otherwise didn't change anything so as to not introduce regressions.

I have no clue why the Wercker build is failing, executing all tests locally takes about 0.5s but the Wercker build takes a minute before it fails. Maybe it's running into a timeout?

@annismckenzie
Copy link
Contributor Author

Fixing the data race introduced another BC break:

  // before
  CustomTypeTagMap["customByteArrayValidator"] = CustomTypeValidator(func(i interface{}, o interface{}) bool {
    // ...
  })

to

  // after
  CustomTypeTagMap.Set("customByteArrayValidator", CustomTypeValidator(func(i interface{}, o interface{}) bool {
    // ...
  }))

Again, deeply sorry for the inconvenience. :-/

…d (in case of a struct) is passed as a second argument to the custom type validator function, making it possible to consider the whole object.
Because I already introduced a BC break, why not add another? The last
build mysteriously failed due to a data race that happens when accessing
the custom validators. You need to use the new `Get` and `Set` methods
on the `CustomTypeTagMap` struct which protects its private validator
type map with a mutex.
@annismckenzie annismckenzie force-pushed the custom_validation_with_context branch from a9ff30b to da9d480 Compare May 17, 2016 08:08
This adds annotated usage examples for custom validators and the recent
breaking changes for registering and using them.
@asaskevich asaskevich merged commit 7664702 into asaskevich:master May 18, 2016
@annismckenzie annismckenzie deleted the custom_validation_with_context branch May 19, 2016 08:44
@ayeshapatel
Copy link

can u give example

@annismckenzie
Copy link
Contributor Author

You can find an example in the readme: https://github.com/asaskevich/govalidator#custom-validation-functions. :)

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.

3 participants