New Feature: Allow UDF and Method Validators to Utilize Error Metadata#48
Conversation
Added the rejectedValue to the error object in the validationResult in order to show that value with replacements in custom messages
The valuerejectedValue key was changed to an expression that only allows simple values.
UDFValidator added rejectedValue to newError arguments
# Conflicts: # modules/cbvalidation/models/validators/UDFValidator.cfc
# Conflicts: # .cfformat.json
Updated test story to be more descriptive
|
Thanks for this PR @homestar9 ! And it has tests!!!! yESSSSS!! Reviewing it with @wpdebruin and will post our thoughts here on it. It is interesting that you bring this up because it kind opens the door for validation recursion where in ORM (quick or hibernate), your relationships can get very complex and you need a way to verify that the component and it's children pass the validations before persisting. |
|
Can we revive this PR @homestar9 I am so sorry, forgot about it. |
|
Yes, I do believe it would be a valuable addition. @lmajano I see the PR has been closed. What do you need from me in order to revive it? |
|
it had conflicts, so I need you to fix the conflicts, and start again :) |
|
Reopened- |
|
Conflicts should be resolved now. |
|
The ACF2021 test failed, but it doesn't look like a problem with the PR from what I can tell. The error log mentions The zip package is not installed so it's probably something wrong with how ACF2021 is configured. |
| any validationData, | ||
| struct rules | ||
| ){ | ||
| var errorMetadata = {}; |
There was a problem hiding this comment.
I think errorMetadata is assuming this will only be used for errors, but in reality I think this can be helpful for ANY type of metadata. It could be errors, messages, i18n, etc. So let's try to use the concept of just metadata. Thoughts?
There was a problem hiding this comment.
I always like the idea of giving users more flexibility with metadata, however, in this case, the data will always be added to the ValidationResult object's errorMetadata property - so it might be confusing to some users.
validationResult.newError( argumentCollection = args ).setErrorMetadata( errorMetadata )
Unless you think there should be another way to retrieve the metadata outside of the validation result?
|
@homestar9 I also will need a pr for docs on this? :) |
|
I think you need to do a pull and rebase, since the github actions has been added now. |
Updated test story to be more descriptive
…ta' into feature/udf-and-method-metadata
|
All set, I rebased and pushed. I will work on the doc PR request today. |
|
I'm not sure why the ACF 2021 test failed. |
|
@homestar9 can you check the docs again please, to see we are good? |
First, a little backstory as to why I feel this would be a valuable addition to CBValidation:
I've found that most complex entity objects are composed of child objects (composed properties). Validating the parent (aggregate root) object often involves ensuring the children are also valid. The best way to do this with CBValidation is to use a UDF or method validator within the parent to
validate()the child objects, and if they pass their own constraints, bubble the success up to the parent like this:However, providing meaningful error messages when performing complex validations via UDFs or methods is extremely difficult, if not impossible.
We can allow users to have more control over their error messages by leveraging the existing error metadata and the custom message replacement system already in place in CBValidation. By simply passing along the
errorMetadatastruct as an additional argument to the UDF (or method), we give developers the ability and flexibility to customize their error messages to their heart's content.Here's an example of how you might do that with a UDF validator with this new feature in place:
Hopefully, you will find this new feature a useful addition. Additionally, the feature shouldn't negatively impact anyone currently using CBValidation since we are just adding an extra argument to the UDF/method validators. In other words, this shouldn't introduce any breaking changes.
Please let me know if you do merge this change into production and I'll work on some documentation changes and examples to showcase the new feature.