Throw immediately on unexpected ClassFinder usage

[denis-anisimov]

This PR may be considered as a fix for #6163. Or may be not. There is a
way to collect all classes in the classpath via
ServletContainerInitializer but I assume it may be very inefficient.
Wold be good to check whether it's feasible to collect all classes and
make the DefautClassFinder instance work as it should work in all
possible cases instead of limiting the useceses.

Related to #6163

---

This change is 

[joheriks]

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @joheriks)

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 81 at r1 (raw file):

        private static final Set<String> APPLICABLE_CLASS_NAME = Collections
                .unmodifiableSet(calculateApplicableClassNames());

APPLICABLE_CLASS_NAMES perhaps more apt as this is a collection.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 105 at r1 (raw file):

                        + clazz + ". Implementation error: the class finder "
                        + "instance is not aware of this class. "
                        + "Fix @HandlesTypes annotation value for"

A space would be required at the end of this string.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 112 at r1 (raw file):

        private Set<String> getApplicableClassNames() {
            return APPLICABLE_CLASS_NAME;
        }

Not sure this getter is really necessary.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 119 at r1 (raw file):

            return Stream.of(handlesTypes.value()).map(Class::getName)
                    .collect(Collectors.toSet());
        }

This method seems unnecessarily verbose, and is not really "calculating" anything. Why not initialize the static set APPLICABLE_CLASS_NAMES directly where it is declared instead?

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 43 at r1 (raw file):

        classes.contains(NpmPackage.class);
        classes.contains(NpmPackage.Container.class);
        classes.contains(WebComponentExporter.class);

The preceding four lines aren't actually testing anything.

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 51 at r1 (raw file):

    public void callGetSubTypesOfByClass_expectedType_doesNotThrow() {
        DevModeClassFinder classFinder = new DevModeClassFinder(
                Collections.emptySet());

Test class could be shortened by creating the DevModeClassFinder instance in a @Before method here and in all the cases below (not blocking).

[denis-anisimov]

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @joheriks)

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 81 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

APPLICABLE_CLASS_NAMES perhaps more apt as this is a collection.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 105 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

A space would be required at the end of this string.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 112 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

Not sure this getter is really necessary.

Not in the current impl.
But I prefer always to make a getter instead of the direct field reference.
It's easier to evolve.
But in this case I don't expect any evolution.
Fixed.

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 119 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

This method seems unnecessarily verbose, and is not really "calculating" anything. Why not initialize the static set APPLICABLE_CLASS_NAMES directly where it is declared instead?

I prefer to keep declaration and initialization separately.
In fact it doesn't matter how the field is initialized. It's an implementation detail.
I think I prefer to keep it as is.
May be change the method name.
Do you have a suggestion ?

[denis-anisimov]

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @joheriks)

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 43 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

The preceding four lines aren't actually testing anything.

Why ?
They test that DevModeInitializer contains the hardcoded class types.
It may looks useless since this is declared out of the box in Java but in case the declaration is changed the test will fail.
And that's the purpose: any change in the declaration should be caught here.

[denis-anisimov]

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @joheriks)

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 51 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

Test class could be shortened by creating the DevModeClassFinder instance in a @Before method here and in all the cases below (not blocking).

Done.

[joheriks]

Reviewed 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 119 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

I prefer to keep declaration and initialization separately.
In fact it doesn't matter how the field is initialized. It's an implementation detail.
I think I prefer to keep it as is.
May be change the method name.
Do you have a suggestion ?

The comment was not primarily about the name, as the method is private. I do on the other hand prefer combining declaration and initialization, but I won't block it.

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 43 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Why ?
They test that DevModeInitializer contains the hardcoded class types.
It may looks useless since this is declared out of the box in Java but in case the declaration is changed the test will fail.
And that's the purpose: any change in the declaration should be caught here.

I see the only assertion in the test is about the size of classes, so I was expecting Assert.assertTrue(classes.contains(...)) as well. How will the test fail now if the declaration is changed?

[denis-anisimov]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @joheriks)

---

flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeClassFinderTest.java, line 43 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

I see the only assertion in the test is about the size of classes, so I was expecting Assert.assertTrue(classes.contains(...)) as well. How will the test fail now if the declaration is changed?

Oh my God.
I'm just an idiot.
Of course.

Thanks.

[denis-anisimov]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @joheriks)

---

flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java, line 119 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

The comment was not primarily about the name, as the method is private. I do on the other hand prefer combining declaration and initialization, but I won't block it.

Technically it's a matter of test.
But in fact I have quite strong reason to do this in this way:

• the field is a part of internal class design (meaning it's a part of high level contract of class implementation).
• the value of the field in fact is not a part of the class design: it's doesn't matter what the value is. The implementation will work with any value.

So I strongly prefer to differentiate a contract/design from implementation detail.
The declaration will always stay as is.
On the other hand the initialization method can be changed in any way: currently it's implemented in this way. But I may decide to change the implementation at any point.
And nothing else will be affected. Only the init method impl.

[joheriks]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained