Skip to content

fix: align npm package order between full and annotation scanners#21193

Merged
mshabarov merged 6 commits intomainfrom
fix/ap/npm-package-order
Mar 31, 2025
Merged

fix: align npm package order between full and annotation scanners#21193
mshabarov merged 6 commits intomainfrom
fix/ap/npm-package-order

Conversation

@platosha
Copy link
Contributor

@platosha platosha commented Mar 28, 2025

Using HashSet for the @NpmPackage annotation values results in unreliable order. The issue is visible in PR validation ITs in Hilla, build-frontend has unexpected alpha5 version picked over before alpha4, whereas prepare-frontend is using alpha4 chosen by FQN sorted class order.

@github-actions
Copy link

github-actions bot commented Mar 28, 2025

Test Results

1 179 files  1 179 suites   1h 15m 28s ⏱️
8 111 tests 8 054 ✅ 57 💤 0 ❌
8 485 runs  8 419 ✅ 66 💤 0 ❌

Results for commit 34def7b.

♻️ This comment has been updated with latest results.

@platosha platosha force-pushed the fix/ap/npm-package-order branch from 8416f4d to 8c34434 Compare March 28, 2025 14:17
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Mar 28, 2025
taefi
taefi previously approved these changes Mar 30, 2025
@tepi
Copy link
Contributor

tepi commented Mar 31, 2025

Comment from @mcollovati on Slack:

If the issue is a different ordering between prepare-frontend and build-frontend, I'm not 100% confident the fix will solve the problem in all cases.
Which version is the first depends also on the order the classes are visited by FrontendAnnotatedClassVisitor

        for (Class<?> component : getFinder()
                .getAnnotatedClasses(NpmPackage.class.getName())) {
            npmPackageVisitor.visitClass(component.getName());
        }

Are we sure that the Set<Class<?>> setAnnotatedClasses(...) method has a stable order? Or should we do here some kind of sorting?
Also to be noted that, looking at the build logs, it looks like prepare-frontend and build-frontend are executed in different maven phases, one before compile and the other after so two different ClassFinder instance are in use
But most likely my concerns have been already verified.

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Mar 31, 2025
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Mar 31, 2025
@platosha
Copy link
Contributor Author

  • ClassFinder#getAnnotatedClasses(String) delegates to ClassFinder.getAnnotatedClasses(Class<? extends Annotation>) from default implementation
  • ClassFinder#getAnnotatedClasses(Class<? extends Annotation>) has 4 implementations:
    1. CachedClassFinder does not interfere with the order, it's only a cache for another class finder
    2. DefaultClassFinder has stable order derived from classes LinkedHashSet, which is populated from FQN-sorted classes, and also using LinkedHashSet for result:
            @Override
          public Set<Class<?>> getAnnotatedClasses(
                  Class<? extends Annotation> annotation) {
              return classes.stream().filter(
                      cl -> cl.getAnnotationsByType(annotation).length > 0)
                      .collect(Collectors.toCollection(LinkedHashSet::new));
          }
    1. DevModeInitializer is delegating to superclass DefaultClassFinder
    2. ReflectionsClassFinder is also using LinkedHashSet and FQN-sorting:
        @Override
      public Set<Class<?>> getAnnotatedClasses(
              Class<? extends Annotation> clazz) {
          Set<Class<?>> classes = new LinkedHashSet<>();
          classes.addAll(reflections.getTypesAnnotatedWith(clazz, true));
          classes.addAll(getAnnotatedByRepeatedAnnotation(clazz));
          return sortedByClassName(classes);
    
      }

This confirms what I saw in the debugger: the classes are name-sorted elsewhere, including data field in the visitor.

@mshabarov mshabarov enabled auto-merge (squash) March 31, 2025 11:28
@sonarqubecloud
Copy link

@mshabarov mshabarov merged commit 5cd2d53 into main Mar 31, 2025
25 of 26 checks passed
@mshabarov mshabarov deleted the fix/ap/npm-package-order branch March 31, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Contribution PRs coming from the community or external to the team +0.0.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants