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

feat(api): add custom validation framework #1756

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Jul 28, 2022

What this PR changes/adds

This PR adds a convenient way for users to register their own validation functions for API methods.

Why it does that

Extensible validation may be desired by users, especially when dealing with extensible types.

Further notes

  • Contrary to what is stated in the DR, all registration methods now accept a ValidationFunction, which is a marker for Function<Object[], Result<Void>>. This was done to improve readability.

Example Usage

public class CustomValidationExtension implements ServiceExtension {

    @Inject
    private ValidationFunctionRegistry validationFunctionRegistry;

    @Override
    public void initialize(ServiceExtensionContext context) {

        // lets validate that every Asset must have an "owner" property
        validationFunctionRegistry.registerForType(AssetEntryDto.class, objects -> {
            var assetDto = Stream.of(objects).filter(ob -> AssetEntryDto.class.isAssignableFrom(ob.getClass())).findFirst();

            if (assetDto.isEmpty()) return Result.failure("Method parameters did not contain an AssetEntryDto!");

            var dto = (AssetEntryDto) assetDto.get();

            if (!dto.getAsset().getProperties().containsKey("owner")) {
                return Result.failure("owner property not present");
            }

            return dto.getAsset().getProperties().get("owner") == null ? Result.failure("owner was null") : Result.success();
        });

        // now lets add some other validation logic, specifically for the "createAsset" method
        try {
            var method = AssetApiController.class.getDeclaredMethod("createAsset", AssetEntryDto.class);
            validationFunctionRegistry.registerForMethod(method, methodArguments -> {
                // some elaborate validation logic
                return Result.failure("createAsset: elaborate validation failed");
            });
        } catch (NoSuchMethodException e) {
            throw new RuntimeException(e);
        }
    }
}

Linked Issue(s)

Closes #1736

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@paullatzelsperger paullatzelsperger added enhancement New feature or request api Feature related to the (REST) api labels Jul 28, 2022
@paullatzelsperger paullatzelsperger added this to the Milestone 6 milestone Jul 28, 2022
@github-actions github-actions bot added this to In progress in Connector Jul 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

Merging #1756 (2ba13b4) into main (78c1496) will increase coverage by 0.33%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##             main    #1756      +/-   ##
==========================================
+ Coverage   67.38%   67.72%   +0.33%     
==========================================
  Files         781      795      +14     
  Lines       16747    16981     +234     
  Branches     1056     1079      +23     
==========================================
+ Hits        11285    11500     +215     
- Misses       5004     5018      +14     
- Partials      458      463       +5     
Impacted Files Coverage Δ
...tension/jersey/validation/ResourceInterceptor.java 91.66% <91.66%> (ø)
...aceconnector/extension/jersey/JerseyExtension.java 100.00% <100.00%> (ø)
...econnector/extension/jersey/JerseyRestService.java 95.00% <100.00%> (+0.55%) ⬆️
...n/jersey/validation/ResourceInterceptorBinder.java 100.00% <100.00%> (ø)
...jersey/validation/ResourceInterceptorProvider.java 100.00% <100.00%> (ø)
...dataspaceconnector/ids/core/util/CalendarUtil.java 0.00% <0.00%> (-25.00%) ⬇️
...dataspaceconnector/core/CoreServicesExtension.java 87.50% <0.00%> (-0.97%) ⬇️
...nder/MultipartCatalogDescriptionRequestSender.java 5.88% <0.00%> (-0.12%) ⬇️
...aceconnector/ids/core/IdsCoreServiceExtension.java 0.00% <0.00%> (ø)
...api/multipart/IdsMultipartApiServiceExtension.java 76.00% <0.00%> (ø)
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@paullatzelsperger paullatzelsperger force-pushed the feature/1736_add_custom_validation branch from ae3e7b8 to 62c4633 Compare July 28, 2022 15:29
@paullatzelsperger paullatzelsperger changed the title feat(api): adds custom validation framework feat(api): add custom validation framework Jul 29, 2022
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Looks really promising!
A test showing the a custom validation implemented would help the comprehension, I know that's still a draft, but it would be a nice to have in the final version of the PR


import java.lang.reflect.Method;

public class ValidationFunctionRegistryImpl implements ValidationFunctionRegistry {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like an avoidable indirection layer, could ResourceInterceptorProvider directly implement ValidationFunctionRegistry?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically we could do that, but the idea was to make the ResourceInterceptorProvider a generic hook point that could be used for other things too, not only validation, so it is a generic layer, that doesn't need to know about validation, whereas the ValidationFunctionRegistry "binds" it specifically to the validation use case.

Also, I liked the idea of having a clean user-facing interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, so I gave it a try, and I do actually like it. check the respective commit

Copy link
Member

Choose a reason for hiding this comment

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

@paullatzelsperger maybe the ValidationFunctionRegistry could be called InterceptorFunctionRegistry?
That would make it more generic looking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it before, wasn't sure, and considering that everything else is called "interceptor"-something and you also now mentioned it, I'm fine with it.

@paullatzelsperger
Copy link
Member Author

Looks really promising!

A test showing the a custom validation implemented would help the comprehension, I know that's still a draft, but it would be a nice to have in the final version of the PR

That's a cool idea - I'll add an integration test for that

@paullatzelsperger paullatzelsperger force-pushed the feature/1736_add_custom_validation branch 2 times, most recently from 4b835e8 to 9e3dd38 Compare July 30, 2022 13:22
@paullatzelsperger paullatzelsperger force-pushed the feature/1736_add_custom_validation branch from 9e3dd38 to 29289a5 Compare July 31, 2022 06:50
@paullatzelsperger paullatzelsperger marked this pull request as ready for review July 31, 2022 20:07
Copy link
Contributor

@florianrusch-zf florianrusch-zf left a comment

Choose a reason for hiding this comment

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

LGTM so far.

Just one additional point: We should maybe also describe at least in the documentation, that the failure message is "important" to be a good one, as it will be returned to the client in the response body.

docs/developer/custom_validation.md Outdated Show resolved Hide resolved
docs/developer/custom_validation.md Show resolved Hide resolved
docs/developer/custom_validation.md Outdated Show resolved Hide resolved
Connector automation moved this from In progress to Review in progress Aug 1, 2022
@paullatzelsperger
Copy link
Member Author

... the failure message is "important" to be a good one, as it will be returned to the client in the response body.

All failure messages should be treated as "good ones". No point in stating that explicitly.

Co-authored-by: Florian Rusch (ZF Friedrichshafen AG) <florian.rusch.external@zf.com>
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Great, LGTM 🚀

@paullatzelsperger paullatzelsperger merged commit 6c8f54f into eclipse-edc:main Aug 2, 2022
Connector automation moved this from Review in progress to Done Aug 2, 2022
diegogomez-zf pushed a commit to diegogomez-zf/DataSpaceConnector that referenced this pull request Aug 3, 2022
* feat(api): add custom validation framework

* added documentation

* updated docs

* Apply suggestions from code review

Co-authored-by: Florian Rusch (ZF Friedrichshafen AG) <florian.rusch.external@zf.com>

* renamed to InterceptorFunctionRegistry

Co-authored-by: Florian Rusch (ZF Friedrichshafen AG) <florian.rusch.external@zf.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Feature related to the (REST) api enhancement New feature or request
Projects
No open projects
Connector
  
Done
Development

Successfully merging this pull request may close these issues.

Custom validator for specific AssetEntryDto fields
5 participants