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

adding allOf validation and test cases #438

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ShivKJ
Copy link

@ShivKJ ShivKJ commented Feb 27, 2025

Adding validation for allOf

@ShivKJ ShivKJ mentioned this pull request Feb 27, 2025
@making
Copy link
Owner

making commented Feb 27, 2025

First of all, thank you for the proposal.

Instead of test cases being a,b,c,x,y,z blah blah, please use use cases that explain why you need this feature.
I generally don't want to add built-in constants unnecessarily. I'll merge them if I think the reason makes sense after understanding the context in which they are added.

See also #191

@@ -42,6 +42,7 @@ enum Default implements ViolationMessage {
OBJECT_EQUAL_TO("object.equalTo", "\"{0}\" must be equal to {1}"), //
OBJECT_ONE_OF("object.oneOf", "\"{0}\" must be one of the following values: {1}"), //
OBJECT_NOT_ONE_OF("object.notOneOf", "\"{0}\" must not be one of the following values: {1}"), //
OBJECT_ALL_OF("object.allOf", "\"{0}\" must be have of all of the following values: {1}, missing values: {2}"), //
Copy link
Owner

Choose a reason for hiding this comment

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

object. prefix is not correct in this case

@@ -69,6 +70,22 @@ public CollectionConstraint<T, L, E> unique() {
return this;
}

public CollectionConstraint<T, L, E> allOf(Collection<? extends E> values) {
Copy link
Owner

Choose a reason for hiding this comment

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

allOf sounds misleading. It seems that contains is more appropriate.

AssertJ's Collection assert method
https://assertj.github.io/doc/#assertj-core-group-contains

contains

Copy link
Author

@ShivKJ ShivKJ Mar 1, 2025

Choose a reason for hiding this comment

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

@making using contains can be a misnomer. It does not explain whether any or all items need to be contained. How about containsAll?

this.predicates().add(ConstraintPredicate.withViolatedValue(collection -> {
final Set<E> missingValues = new HashSet<>(values);

if (collection instanceof Set)
Copy link
Owner

Choose a reason for hiding this comment

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

Use brackets

if (collection instanceof Set)
missingValues.removeAll(collection);
else
missingValues.removeAll(new HashSet<>(collection));
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a need to wrap it in a Set?

Copy link
Author

Choose a reason for hiding this comment

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

It becomes efficient. If you check removeAll implementation in AbstractSet, items are checked in collection so using set will be efficient. See below method for removeAll in AbstractSet.

    public boolean removeAll(Collection<?> c) {
        Objects.requireNonNull(c);
        boolean modified = false;

        if (size() > c.size()) {
            for (Object e : c)
                modified |= remove(e);
        } else {
            for (Iterator<?> i = iterator(); i.hasNext(); ) {
                if (c.contains(i.next())) {
                    i.remove();
                    modified = true;
                }
            }
        }
        return modified;
    }

@making making added feedback required enhancement New feature or request labels Feb 27, 2025
@ShivKJ
Copy link
Author

ShivKJ commented Mar 1, 2025

#438 (comment)

Instead of test cases being a,b,c,x,y,z blah blah, please use use cases that explain why you need this feature.
I generally don't want to add built-in constants unnecessarily. I'll merge them if I think the reason makes sense after understanding the context in which they are added.

@making I followed the test case format already present in the same test file. The use case is simply to verify whether all items from one collection are present in an expected collection. For example, consider a person on vacation with a planned list of places to visit. There might be certain locations they definitely want to visit. This type of validation can be useful in such scenarios.

@ShivKJ ShivKJ requested a review from making March 1, 2025 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants