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

anyOf and allOf variants for multiple ThrowingConsumer parameters #2764

Closed
onacit opened this issue Sep 5, 2022 · 8 comments · Fixed by #3219
Closed

anyOf and allOf variants for multiple ThrowingConsumer parameters #2764

onacit opened this issue Sep 5, 2022 · 8 comments · Fixed by #3219
Assignees
Labels
type: new feature A new feature
Milestone

Comments

@onacit
Copy link
Contributor

onacit commented Sep 5, 2022

Feature summary

I need to assert with satisfiesAnyOf for each element in an iterable.

This may be a collection version of #1304 .

List<Some> some = getSome();

// Now each some.other should be asserted by any of multiple conditions.

some.stream.map(Some::getOther).forEach(o -> {
    assertThat(o).satisfiesAnyOf(
        o1 -> assertThat(o1).isNull(),
        o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x);     
    );
});

assertThat(some)
    .extracting(Some::getOther)
    .allSatisfy(o -> {
        assertThat(o).satisfiesAnyOf(
            o1 -> assertThat(o1).isNull(),
            o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x);     
        );
    });

Example

List<Some> some = getSome();
assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfyAnyOf( // or eachSatisfiesAnyOf
        o -> {},
        o -> {}
    );        
@joel-costigliola
Copy link
Member

allSatisfyAnyOf could be a good addition to keep the assertion light, @scordio thoughts?

@scordio
Copy link
Member

scordio commented Sep 5, 2022

If I understood it correctly, we want to transform this:

assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfy(o -> {
        assertThat(o).satisfiesAnyOf(
            o1 -> assertThat(o1).isNull(),
            o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x)
        )
    });

into this:

assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfyAnyOf(
        o -> assertThat(o).isNull(),
        o -> assertThat(o).isNotNull().extracting(Other::getId).isEqualTo(x);     
    );

which is indeed easier to read.

However, this would open the door to other combinations:

  • allSatisfyAllOf
  • anySatisfyAllOf
  • anySatisfyAnyOf

Adding anyMatch / allMatch combinations as well would mean four more methods.

Are we willing to add all of these?

With some formatting adjustments, the original example could look like this:

assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfy(o -> assertThat(o).satisfiesAnyOf(
        o1 -> assertThat(o1).isNull(),
        o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x)
    ));

which I personally consider a good compromise.

@joel-costigliola
Copy link
Member

Fair point, we don't want to have so many combinations

@scordio scordio closed this as not planned Won't fix, can't repro, duplicate, stale Sep 6, 2022
@scordio scordio added the status: declined A suggestion or change that we don't feel we should currently apply label Sep 6, 2022
@onacit
Copy link
Contributor Author

onacit commented Nov 10, 2022

If I may, I want to contribute to the project by sharing my solution.

I have an abstract base class for my entity classes.

public abstract class _Mapped<T extends _Mapped<T, U>, U> {

    @Transient
    protected @Nullable U _getId() {
        return ...;
    }
}

An here comes one of entities.

@Entity
@Table(name = ...)
public class Some extends _Mapped<Some, Long> {

    public void setOther(final Other other) {
        this.other = other;
        setOtherId(
                Optional.ofNullable(this.other)
                        .map(Other::getId)
                        .orElse(null)
        );
    }

    @Column(name = COLUMN_NAME_OTHER_ID, nullable = false)
    private Long otherId;

    @Valid
    @NotFound(action = NotFoundAction.IGNORE) // -> EAGER!
    @ManyToOne(optional = /*false*/true, fetch = FetchType.LAZY)
    @JoinColumn(name = COLUMN_NAME_OTEHR_ID, nullable = false, insertable = false, updatable = false,
                foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT))
    @EqualsAndHashCode.Exclude
    @ToString.Exclude
    private Other other; // either null or has its `id` equal to `otherId`
}

Now, with a list of selected Some instances, I want to assert that each element's other attribute is either null or its id attribute is equal to sibling otherId.

As an already applicable solution, I can do this.

List<Some> list = selectSome();
list.forEach(s -> {
    assertThat(s.getOther()).satisfiesAnyOf(
        o -> assertThat(o).isNull(),
        o -> assertThat(o).isNotNull().extracting(Other::getId).isEqualTo(s.getOtherId());
    );
});

Nothing's wrong, as already mentioned, yet the Condition can help.

public final class _MappedConditions {

    public static <T extends _Mapped<T, U>, U> Condition<T> id(final U id) {
        return new Condition<>(
                t -> {
                    assert t != null;
                    return Objects.equals(t._getId(), id);
                },
                String.format("should has id of %1$s", id)
        );
    }
}

Now the expression may look like this.

        list.forEach(s -> {
            assertThat(s.getOther()).satisfiesAnyOf(
                    o -> assertThat(o).isNull(),
                    o -> assertThat(o).isNotNull().has(id(r.getOtherId()))
            );
        });

And another Condition combining the previous condition may do more.

    public static <T extends _Mapped<T, U>, U> Condition<T> nullOrHasId(final U id) {
        final Condition<T> hasId = id(id);
        return new Condition<>(
                t -> t == null || hasId.matches(t),
                "should be null or " + hasId.description().value()
        );
    }

Which leads the statements look like this.

        list.forEach(s -> {
            assertThat(s.getOther()).is(nullOrHasId(r.getOtherId()));
        });
        // or
        assertThat(list).allSatisfy(s -> {
            assertThat(s.getOther()).is(nullOrHasId(r.getOtherId()));
        });

@scordio
Copy link
Member

scordio commented May 26, 2023

Reopening to discuss a potential improvement for this use case.

We could transform my example:

assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfy(o -> assertThat(o).satisfiesAnyOf(
        o1 -> assertThat(o1).isNull(),
        o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x)
    ));

into:

assertThat(some)
    .isNotEmpty()
    .extracting(Some::getOther)
    .allSatisfy(anyOf( // anyOf(ThrowingConsumer...) in Assertions?
        o1 -> assertThat(o1).isNull(),
        o1 -> assertThat(o1).isNotNull().extracting(Other::getId).isEqualTo(x)
    ));

where anyOf is defined as:

public static <T> ThrowingConsumer<T> anyOf(ThrowingConsumer<? super T>... assertions) {
  return e -> new ObjectAssert<>(e).satisfiesAnyOf(assertions);
}

Also, an allOf with similar pattern could be defined.

We already have other anyOf and allOf methods in Assertions but they are related to Conditions.

@joel-costigliola @onacit thoughts?

@scordio scordio reopened this May 26, 2023
@scordio scordio added status: team discussion An issue we'd like to discuss as a team to make progress and removed status: declined A suggestion or change that we don't feel we should currently apply labels May 26, 2023
@joel-costigliola
Copy link
Member

LGTM

@onacit
Copy link
Contributor Author

onacit commented May 28, 2023

@scordio What an elegant syntax/idiom you suggested! Thanks!

@scordio
Copy link
Member

scordio commented Jul 2, 2023

There is one aspect where I see a potential issue.

If we would put anyOf / allOff in Assertions, from a signature perspective there is nothing connecting the new methods to allSatisfy assertions:

public static <T> ThrowingConsumer<T> anyOf(ThrowingConsumer<? super T>... assertions) {
  ...
}

This means they would "steal" the ThrowingConsumer vararg parameter usage. This might still be ok, though.

@scordio scordio added this to the 3.25.0 milestone Oct 12, 2023
@scordio scordio self-assigned this Oct 12, 2023
@scordio scordio changed the title Add IterableAssert#allSatisfyAnyOf anyOfand allOf variants for multiple ThrowingConsumers Oct 12, 2023
@scordio scordio changed the title anyOfand allOf variants for multiple ThrowingConsumers anyOfand allOf variants for multiple ThrowingConsumer parameters Oct 12, 2023
@scordio scordio changed the title anyOfand allOf variants for multiple ThrowingConsumer parameters anyOf and allOf variants for multiple ThrowingConsumer parameters Oct 12, 2023
@scordio scordio added type: new feature A new feature and removed status: team discussion An issue we'd like to discuss as a team to make progress labels Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: new feature A new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants