[COLLECTIONS-858] Add CartesianProductIterator

[alexey-pelykh]

The CartesianProductIterator is suitable to enhance a deeply-nested for-loops over a set of collections.

Given

var a = new Arrays.asList('a1', 'a2', 'a3');
var b = new Arrays.asList('b1', 'b2', 'b3');
var c = new Arrays.asList('c1', 'c2', 'c3');
var d = new Arrays.asList('d1', 'd2', 'd3');
var e = new Arrays.asList('e1', 'e2', 'e3');
var f = new Arrays.asList('f1', 'f2', 'f3');

instead of

for (var aElement : a) {
    for (var bElement : b) {
        for (var cElement : c) {
            for (var dElement : d) {
                for (var eElement : e) {
                    for (var fElement : f) {
                        // ...
                    }
                }
            }
        }
    }
}

it would look like:

var it = new CartesianProductIterator<>(a, b, c, d, e, f);
while (it.hasNext()) {
    var elements = it.next();
}

Extremely useful for nesting levels 10+.

[aherbert]

This is looking good apart from the javadoc which will fail the build. If you use <,> chars you need a code tag.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.53%. Comparing base (229425c) to head (9b5791b).
Report is 315 commits behind head on master.

Files	Patch %	Lines
...llections4/iterators/CartesianProductIterator.java	94.28%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #509      +/-   ##
============================================
- Coverage     81.60%   81.53%   -0.08%     
- Complexity     4745     4848     +103     
============================================
  Files           295      301       +6     
  Lines         13751    14128     +377     
  Branches       2022     2078      +56     
============================================
+ Hits          11222    11519     +297     
- Misses         1929     1995      +66     
- Partials        600      614      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[garydgregory]

@alexey-pelykh
Make sure you run 'mvn' by itself before you push to run all build checks.

[alexey-pelykh]

@garydgregory sorry, my oversight - should be green now

[alexey-pelykh]

@garydgregory would it be possible to restart 23-ae? It's not exactly clear what is wrong

[aherbert]

The build died at the PMD plugin with an unsupported class major version number of 67 (for Java 23). However the build on 23 is allowed to fail as it is in the experimental group in the workflow: maven.yml

This looks good to me.

[alexey-pelykh]

What would be the next steps?

[garydgregory]

@alexey-pelykh
This PR has zero text in its description.

[alexey-pelykh]

@garydgregory that is true, any specific requirements what to write there apart of what I've already added to it?

Edit: I truly don't know what else to write, the name of the PR is self-explanatory really. It's a new iterator that implements a cartesian product, which is literally a nested for-loop yet writing a manual for loop when there's 10-25-50 depth would be troublesome.

[garydgregory]

@alexey-pelykh
I want to email the dev list and ask for reviews, and the first I do at least is read the description to know what it is I'm looking at... it's up to you to help reviewers...

[alexey-pelykh]

@garydgregory okies, done 👍

[alexey-pelykh]

Seems not much of a discussion :)

[alexey-pelykh]

👋 @garydgregory anything I can do to push this forward?

[garydgregory]

Hello @alexey-pelykh

• You are missing a unit test for Iterator.forEachRemaining()
• Also edge cases like what if the first list, a middle list or the last list is empty.
• IOW what is the expected behavior if in your example letters, numbers, or symbols is empty
• What should happen if the lists point to each other or have cycles?

[garydgregory]

Just FTR https://stackoverflow.com/questions/32131987/how-can-i-make-cartesian-product-with-java-8-streams

[alexey-pelykh]

Hi @garydgregory:

You are missing a unit test for Iterator.forEachRemaining()

What is the rationale to add that given that forEachRemaining is a hasNext()/next() wrapper and those are explicitly tested?

Also edge cases like what if the first list, a middle list or the last list is empty.

Reasonable, added.

IOW what is the expected behavior if in your example letters, numbers, or symbols is empty

Added tests to be explicit. If at least one list is empty, cartesian product returns no tuples.

What should happen if the lists point to each other or have cycles?

If any of the passed iterables has cycle and thus is infinite, the iterator would also produce an infinite result. Here the behavior is the same as in nested for-loop. Could you please elaborate on the lists pointing to each other case as I'm not exactly sure I got you there.

[garydgregory]

Hi @garydgregory:

You are missing a unit test for Iterator.forEachRemaining()

What is the rationale to add that given that forEachRemaining is a hasNext()/next() wrapper and those are explicitly tested?

Also edge cases like what if the first list, a middle list or the last list is empty.

Reasonable, added.

IOW what is the expected behavior if in your example letters, numbers, or symbols is empty

Added tests to be explicit. If at least one list is empty, cartesian product returns no tuples.

What should happen if the lists point to each other or have cycles?

If any of the passed iterables has cycle and thus is infinite, the iterator would also produce an infinite result. Here the behavior is the same as in nested for-loop. Could you please elaborate on the lists pointing to each other case as I'm not exactly sure I got you there.

If the API allows an infinite loop, this is what will happen: Someone will send an email to our security mailing list saying they found a critical DOS vulnerability and ask for a CVE to be credited to them; time will be taken analyzing, deciding what to do and replying to the poster. This is why we've fixed these types of bugs overtime and added tests like:

• HashCodeBuilderTest.testReflectionObjectCycle()
• ToStringBuilderTest.testSelfInstanceTwoVarsReflectionObjectCycle()
• ToStringBuilderTest.testSelfInstanceVarReflectionObjectCycle()
• ToStringBuilderTest.testSimpleReflectionObjectCycle()
• StrSubstitutorTest.testCyclicReplacement()
• ToStringBuilderTest.testReflectionArrayAndObjectCycle()
• ToStringBuilderTest.testReflectionArrayArrayCycle()
• ...

And yes, there are probably more of these types of bugs lurking around, so let's not add new ones ;-)

[alexey-pelykh]

And yes, there are probably more of these types of bugs lurking around, so let's not add new ones ;-)

Absolutely not my intent! That said, I guess I'm too dumb to understand how that bug may be exploited in this case.

• If a CartesianProductIterator is passed an infinite iterable, the result is expectedly infinite - the same was as for loop over infinite iterable is going to be infinite.
• If a CartesianProductIterator is passed an iterable that is modified while iterating over the iterator, it may or may not yield into an infinite iterator.
• If a CartesianProductIterator is passed a fake iterable that always returns reference to the CartesianProductIterator it was passed to ("iterating over self") - it may be infinite.

However, the infinite loop won't happen in theCartesianProductIterator itself, but the hasNext() will always be true - so iterating over such an iterator will be infinite, alike iterating over a "random generator" iterator.

[garydgregory]

@garydgregory would it be possible to restart 23-ae? It's not exactly clear what is wrong

EA builds are marked experimental in the CI and fail when the tooling has not been adapted Java EA versions. In this case PMD needs an update to ASM to support Java 23 and 24 byte code changes. If your branch fails with the same errors as master for Java 23-EA and 24-EA, then there is nothing to do.

[garydgregory]

You are missing a unit test for Iterator.forEachRemaining()

What is the rationale to add that given that forEachRemaining is a hasNext()/next() wrapper and those are explicitly tested?

The new class implements an interface. The tests should show what are the expectations for all interface methods, the ones that are actually implemented by the new class and the behavior that is inherited from the default method.

There is no guarantee that implementing the methods that a default interface calls is actually correctly implementing the contract for the default method. Testing the default methods shows that it works as expected, regardless of whether the default method is implemented or inherited. You are also showing as a PR author that you've considered the big picture and are thorough, all the pieces work individually and together, as expected.

[alexey-pelykh]

Testing the default methods shows that it works as expected, regardless of whether the default method is implemented or inherited.

👍 test added

[garydgregory]

@alexey-pelykh

Thank you for your PR and patience. Merged! 🚀