Visitable.accept visits all Visitor instances regardless of the type

[manusa]

Changes in #431 seem to be causing an issue with Visitors

With the new changes in:

sundrio/core/src/main/java/io/sundr/builder/Visitable.java

Lines 77 to 91 in 836ebea

	// Copy visitables to avoid ConcurrentModificationException when Visitors add/remove Visitables
	for (Visitable<T> visitable : new ArrayList<>((List<Visitable<T>>) entry.getValue())) {
	for (Visitor visitor : visitors) {
	if (visitor.getType() != null && visitor.getType().isAssignableFrom(visitable.getClass())) {
	visitable.accept(newPath, entry.getKey(), visitor);
	}
	}
	for (Visitor visitor : visitors) {
	if (visitor.getType() == null || !visitor.getType().isAssignableFrom(visitable.getClass())) {
	visitable.accept(newPath, entry.getKey(), visitor);
	}
	}
	}
	}

Visitors are now called regardless of the matching type.

Seems like the double for-loop is a mistake or something left from previous iterations.

/cc @shawkins @iocanel

[iocanel]

I am not sure that I completely understand what the problem is, so let me try to provide some details on how it should work (or at least how I remember it should work).

The accept method on a Visitable is meant to accept any type of Visitor regardless of type. The Visitor may or may not visit the current Visitable but is forwarded/accept to all child Visitable.
For ordering reasons the forwarding process is done in two iterations:

1st pass we forward/accept all Visitors with matching types
2st pass we forward/accept all Visitor items with non-matching types

Why ? Because we want directly matching Visitor to always be applied before any other Visitor.
Why ? Becuase a directly matching Visitor may populate additional Visitable items.

So, the highlighted double for loop seems needed to me. Also, accepting all types of Visitor seems justified to me too.
Maybe its the canVisit that is not respected and the visit method being called out of place?
What exactly is the side effect?

[manusa]

OK; I might have misdiagnosed the issue then. However, we still have a problem here. Still digging into it.

The test that revealed this (among others) was https://github.com/eclipse/jkube/blob/5da6068d7756784aef9569568ee80f98da97e296/jkube-kit/enricher/generic/src/test/java/org/eclipse/jkube/enricher/generic/TriggersAnnotationEnricherTest.java#L59

For some reason the visitor is being triggered multiple times.

[manusa]

So the following test fabric8io/kubernetes-client#5558 illustrates the problem.

In this case, the visitor is called 4 times instead of the expected 1 (or at least this is how it behaved before).

The problem might be in the getVisitableMap part instead.

There seems to be some recursion going on.

[shawkins]

@manusa this should be unrelated to the recent change - all that did was move the existing logic. At least for me if I change Visitable and Basefluent back to what they were, you still see things visited multiple times.

[manusa]

I'm not sure if you've checked fabric8io/kubernetes-client#5558 (maybe I should port it to this repo though)

It illustrates the issue, it works with 6.9.0 but fails with 6.9.1.

As you say, your changes just move around code. However, it's my impression that some of the changes regarding the use of the Map and its entries seem to be causing the visiting logic to become recursive.

[shawkins]

It illustrates the issue, it works with 6.9.0 but fails with 6.9.1.

It's something else that has changed with 6.9.1 - if you revert BaseFluent / Visitable in 6.9.1, you see 4 visitations. If you forward port the changes in 6.9.0 you see 1 visitation.

[manusa]

If you forward port the changes in 6.9.0 you see 1 visitation.

You mean just bumping the Sundrio version or something else?

[manusa]

So I've tried Fabric8's main branch with Sundrio 0.101.0 and the test passes correctly.

So it must be something changed in Sundrio between these two versions:

0.101.0...0.101.2

[shawkins]

You mean just bumping the Sundrio version or something else?

I've found the issue. There's a place in the generated fluents where the visitable tracking is not correct.

[manusa]

I'm trying to implement a test for this repo, hopefully will send a PR in a few minutes

[shawkins]

@manusa this works for me #440

[manusa]

I finally managed to create the reproducer test, nothing like a good weekend's rest :)