fix: Warn for duplicate npm classes (#13291) (#13309) (#13310)

If encountering multiple NpmPackage
annotations with different versions log
a warning and inform of choice taken.

Fixes #13285

Co-authored-by: caalador <mikael.grankvist@vaadin.com>

flow-migration/src/main/java/com/vaadin/flow/server/scanner/ReflectionsClassFinder.java

@@ -22,6 +22,7 @@
 import java.net.URLClassLoader;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Set;
 
 import org.reflections.Reflections;
@@ -60,7 +61,7 @@ public ReflectionsClassFinder(URL... urls) {
     @Override
     public Set<Class<?>> getAnnotatedClasses(
             Class<? extends Annotation> clazz) {
-        Set<Class<?>> classes = new HashSet<>();
+        Set<Class<?>> classes = new LinkedHashSet<>();
         classes.addAll(reflections.getTypesAnnotatedWith(clazz, true));
         classes.addAll(getAnnotatedByRepeatedAnnotation(clazz));
         return classes;

flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeTasks.java

@@ -443,7 +443,7 @@ private NodeTasks(Builder builder) {
 
             frontendDependencies = new FrontendDependenciesScanner.FrontendDependenciesScannerFactory()
                     .createScanner(!builder.useByteCodeScanner, classFinder,
-                            builder.generateEmbeddableWebComponents);
+                            builder.generateEmbeddableWebComponents, false);
 
             if (builder.generateEmbeddableWebComponents) {
                 FrontendWebComponentGenerator generator = new FrontendWebComponentGenerator(
@@ -517,7 +517,7 @@ private FrontendDependenciesScanner getFallbackScanner(Builder builder,
         if (builder.useByteCodeScanner) {
             return new FrontendDependenciesScanner.FrontendDependenciesScannerFactory()
                     .createScanner(true, finder,
-                            builder.generateEmbeddableWebComponents);
+                            builder.generateEmbeddableWebComponents, true);
         } else {
             return null;
         }

flow-server/src/main/java/com/vaadin/flow/server/frontend/scanner/ClassFinder.java

@@ -18,8 +18,10 @@
 import java.io.Serializable;
 import java.lang.annotation.Annotation;
 import java.net.URL;
+import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -40,7 +42,7 @@ public interface ClassFinder extends Serializable {
      * list of classes.
      */
     class DefaultClassFinder implements ClassFinder {
-        private final Set<Class<?>> classes;
+        private final LinkedHashSet<Class<?>> classes;
 
         private final transient ClassLoader classLoader;
 
@@ -51,7 +53,15 @@ class DefaultClassFinder implements ClassFinder {
          *            The classes.
          */
         public DefaultClassFinder(Set<Class<?>> classes) {
-            this.classes = classes;
+            /*
+             * Order the classes to get deterministic behavior. It does not
+             * really matter HOW the classes are ordered as long as they are
+             * always in the same order
+             */
+            List<Class<?>> classList = new ArrayList<>(classes);
+            classList.sort(
+                    (cls1, cls2) -> cls1.getName().compareTo(cls2.getName()));
+            this.classes = new LinkedHashSet<>(classList);
             // take classLoader from the first class in the set, unless empty
             classLoader = classes.isEmpty() ? getClass().getClassLoader()
                     : classes.iterator().next().getClassLoader();
@@ -70,7 +80,7 @@ public DefaultClassFinder(Set<Class<?>> classes) {
         public DefaultClassFinder(ClassLoader classLoader,
                 Class<?>... classes) {
             this.classLoader = classLoader;
-            this.classes = new HashSet<>();
+            this.classes = new LinkedHashSet<>();
             for (Class<?> clazz : classes) {
                 this.classes.add(clazz);
             }
@@ -81,7 +91,7 @@ public Set<Class<?>> getAnnotatedClasses(
                 Class<? extends Annotation> annotation) {
             return classes.stream().filter(
                     cl -> cl.getAnnotationsByType(annotation).length > 0)
-                    .collect(Collectors.toSet());
+                    .collect(Collectors.toCollection(LinkedHashSet::new));
         }
 
         @Override
@@ -102,7 +112,8 @@ public <T> Set<Class<? extends T>> getSubTypesOf(Class<T> type) {
             return this.classes.stream()
                     .filter(cl -> GenericTypeReflector.isSuperType(type, cl)
                             && !type.equals(cl))
-                    .map(cl -> (Class<T>) cl).collect(Collectors.toSet());
+                    .map(cl -> (Class<T>) cl)
+                    .collect(Collectors.toCollection(LinkedHashSet::new));
         }
     }
 

flow-server/src/main/java/com/vaadin/flow/server/frontend/scanner/FrontendDependenciesScanner.java

@@ -59,10 +59,35 @@ class FrontendDependenciesScannerFactory {
         public FrontendDependenciesScanner createScanner(
                 boolean allDependenciesScan, ClassFinder finder,
                 boolean generateEmbeddableWebComponents) {
+            return createScanner(allDependenciesScan, finder,
+                    generateEmbeddableWebComponents, false);
+        }
+
+        /**
+         * Produces scanner implementation based on {@code allDependenciesScan}
+         * value.
+         * <p>
+         *
+         * @param allDependenciesScan
+         *            if {@code true} then full classpath scanning strategy is
+         *            used, otherwise byte scanning strategy is produced
+         * @param finder
+         *            a class finder
+         * @param generateEmbeddableWebComponents
+         *            checks {@code WebComponentExporter} classes for
+         *            dependencies if {@code true}, doesn't check otherwise
+         * @param fallback
+         *            whether FullDependenciesScanner is used as fallback
+         * @return a scanner implementation strategy
+         */
+        public FrontendDependenciesScanner createScanner(
+                boolean allDependenciesScan, ClassFinder finder,
+                boolean generateEmbeddableWebComponents,
+                boolean fallback) {
             if (allDependenciesScan) {
                 // this dep scanner can't distinguish embeddable web component
                 // frontend related annotations
-                return new FullDependenciesScanner(finder);
+                return new FullDependenciesScanner(finder, fallback);
             } else {
                 return new FrontendDependencies(finder,
                         generateEmbeddableWebComponents);

flow-server/src/main/java/com/vaadin/flow/server/frontend/scanner/FullDependenciesScanner.java

@@ -57,6 +57,7 @@ class FullDependenciesScanner extends AbstractDependenciesScanner {
     private static final String COULD_NOT_LOAD_ERROR_MSG = "Could not load annotation class ";
 
     private static final String VALUE = "value";
+    private static final String VERSION = "version";
 
     private ThemeDefinition themeDefinition;
     private AbstractTheme themeInstance;
@@ -70,15 +71,19 @@ class FullDependenciesScanner extends AbstractDependenciesScanner {
 
     private final SerializableBiFunction<Class<?>, Class<? extends Annotation>, List<? extends Annotation>> annotationFinder;
 
+    private final boolean fallback;
+
     /**
      * Creates a new scanner instance which discovers all dependencies in the
      * classpath.
      *
      * @param finder
      *            a class finder
+     * @param fallback
+     *            whether FullDependenciesScanner is used as fallback
      */
-    FullDependenciesScanner(ClassFinder finder) {
-        this(finder, AnnotationReader::getAnnotationsFor);
+    FullDependenciesScanner(ClassFinder finder, boolean fallback) {
+        this(finder, AnnotationReader::getAnnotationsFor, fallback);
     }
 
     /**
@@ -89,10 +94,16 @@ class FullDependenciesScanner extends AbstractDependenciesScanner {
      *            a class finder
      * @param annotationFinder
      *            a strategy to discover class annotations
+     * @param fallback
+     *            whether dependency scanner is used as fallback
      */
     FullDependenciesScanner(ClassFinder finder,
-            SerializableBiFunction<Class<?>, Class<? extends Annotation>, List<? extends Annotation>> annotationFinder) {
+            SerializableBiFunction<Class<?>, Class<? extends Annotation>, List<? extends Annotation>> annotationFinder,
+            boolean fallback) {
         super(finder);
+
+        this.fallback = fallback;
+
         long start = System.currentTimeMillis();
         this.annotationFinder = annotationFinder;
         try {
@@ -191,12 +202,28 @@ private Map<String, String> discoverPackages() {
                 classes.add(clazz.getName());
                 List<? extends Annotation> packageAnnotations = annotationFinder
                         .apply(clazz, loadedAnnotation);
-                packageAnnotations.forEach(pckg -> {
-                    String value = invokeAnnotationMethodAsString(pckg, VALUE);
-                    String vers = invokeAnnotationMethodAsString(pckg,
-                            "version");
-                    logs.add(value + " " + vers + " " + clazz.getName());
-                    result.put(value, vers);
+                packageAnnotations.forEach(annotation -> {
+                    String value = invokeAnnotationMethodAsString(annotation,
+                            VALUE);
+                    String version = invokeAnnotationMethodAsString(annotation,
+                            VERSION);
+                    logs.add(value + " " + version + " " + clazz.getName());
+                    if (result.containsKey(value)
+                            && !result.get(value).equals(version)) {
+                        if (!fallback) {
+                            // Only log warning if full scanner is not used as
+                            // fallback scanner. For fallback the bytecode
+                            // scanner will have informed about multiple
+                            // versions
+                            String foundVersions = "[" + result.get(value)
+                                    + ", " + version + "]";
+                            getLogger().warn(
+                                    "Multiple npm versions for {} found:  {}. First version found '{}' will be considered.",
+                                    value, foundVersions, result.get(value));
+                        }
+                    } else {
+                        result.put(value, version);
+                    }
                 });
             }
             debug("npm dependencies", logs);

flow-server/src/test/java/com/vaadin/flow/server/frontend/scanner/FullDependenciesScannerTest.java

@@ -317,7 +317,7 @@ public void getModules_explcitTheme_returnAllModulesExcludingNotUsedTheme_getCla
                     }
                     Assert.fail();
                     return null;
-                });
+                }, false);
 
         List<String> modules = scanner.getModules();
         Assert.assertEquals(24, modules.size());
@@ -390,7 +390,7 @@ private FullDependenciesScanner setUpAnnotationScanner(
                 .thenReturn(getAnnotatedClasses(annotationType));
 
         return new FullDependenciesScanner(finder,
-                (type, annotation) -> findAnnotations(type, annotationType));
+                (type, annotation) -> findAnnotations(type, annotationType), false);
     }
 
     private FullDependenciesScanner setUpThemeScanner(
@@ -410,7 +410,7 @@ private FullDependenciesScanner setUpThemeScanner(
         Mockito.when(finder.getAnnotatedClasses(fakeNoThemeClass))
                 .thenReturn(noThemeClasses);
 
-        return new FullDependenciesScanner(finder, annotationFinder) {
+        return new FullDependenciesScanner(finder, annotationFinder, false) {
             @Override
             protected Class<? extends AbstractTheme> getLumoTheme() {
                 return FakeLumoTheme.class;