Skip to content

Comments

New Feature: Allow UDF and Method Validators to Utilize Error Metadata#48

Merged
lmajano merged 32 commits intocoldbox-modules:developmentfrom
homestar9:feature/udf-and-method-metadata
Jan 12, 2022
Merged

New Feature: Allow UDF and Method Validators to Utilize Error Metadata#48
lmajano merged 32 commits intocoldbox-modules:developmentfrom
homestar9:feature/udf-and-method-metadata

Conversation

@homestar9
Copy link
Contributor

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:

// root object constraints
this.constraints = {
    "childObject": { udf: function( value, target ) {
            // validate the child object using its own constraints
            var validationResult = validationManager.validate( value );
            return !validationResult.hasErrors();
    } };
}

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 errorMetadata struct 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:

// root object constraints
this.constraints = {
    "childObject": { udf: function( value, target, metadata ) {
            // ... perform some complex validation
            // create a custom message and store it in the metadata.
            metadata[ "customMessage" ] = "This is a custom error message from within the udf";
            return false; 
        },
        // the message replacement system will look for a matching metadata key and replace it with the message from the UDF.
        udfMessage: "{customMessage}" 
    };
}

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.

@lmajano
Copy link
Contributor

lmajano commented Mar 16, 2021

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.

@lmajano
Copy link
Contributor

lmajano commented Nov 12, 2021

Can we revive this PR @homestar9 I am so sorry, forgot about it.

@lmajano lmajano closed this Nov 12, 2021
@homestar9
Copy link
Contributor Author

homestar9 commented Nov 12, 2021

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?

@lmajano
Copy link
Contributor

lmajano commented Nov 12, 2021

it had conflicts, so I need you to fix the conflicts, and start again :)

@lmajano lmajano reopened this Nov 12, 2021
@lmajano
Copy link
Contributor

lmajano commented Nov 12, 2021

Reopened-

@homestar9
Copy link
Contributor Author

Conflicts should be resolved now.

@homestar9
Copy link
Contributor Author

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 = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@lmajano
Copy link
Contributor

lmajano commented Nov 12, 2021

@homestar9 I also will need a pr for docs on this? :)
Great job on the tests! It's great to see the fine attention to detail.

@lmajano
Copy link
Contributor

lmajano commented Nov 12, 2021

I think you need to do a pull and rebase, since the github actions has been added now.

@homestar9 homestar9 requested a review from lmajano December 18, 2021 19:04
@homestar9
Copy link
Contributor Author

All set, I rebased and pushed. I will work on the doc PR request today.

@homestar9
Copy link
Contributor Author

I'm not sure why the ACF 2021 test failed.
I also added a PR for the docs: ortus-docs/cbvalidation-docs#9 Please feel free to revise or send me feedback if you feel it could be worded better. It's my first time making a big change to the docs and I'd like to get it right. :)

@lmajano lmajano merged commit 9d7197c into coldbox-modules:development Jan 12, 2022
@lmajano
Copy link
Contributor

lmajano commented Jan 12, 2022

@homestar9 can you check the docs again please, to see we are good?

@homestar9
Copy link
Contributor Author

@lmajano yes, I believe so. You approved the PR for the doc update here.

@homestar9 homestar9 deleted the feature/udf-and-method-metadata branch January 12, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants