Repository navigation
Make VaadinScanPackages serializable and merge scan packages correctly - #25062
Hariharan02112004 wants to merge 1 commit into
Conversation
|
|
|
Updated description: DescriptionThis change extracts scan package handling into VaadinScanPackages and makes it Serializable. Previously, scan package merging logic was handled inside VaadinScanPackagesRegistrar. The merge logic is now handled by VaadinScanPackages. FixesFixes #25022 |
mcollovati
added
the
Contribution
mshabarov
added
Intent Approved
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class VaadinScanPackages implements Serializable { |
There was a problem hiding this comment.
This can be turned into a Java record.
There was a problem hiding this comment.
If this class becomes public, it requires proper Javadoc both at class, constructor and method level.
Javadoc should also state that it is an internal API.
- For internal use only. May be renamed or removed in a future release.
We should consider if having it public makes sense (e.g. knowing user-defined packages are useful for some additional processor other than bootstrap) or if it can be package-private.
|
|
||
| public VaadinScanPackages(String[] scanPackages) { | ||
| assert scanPackages != null; | ||
| this.scanPackages = new ArrayList<>(Arrays.asList(scanPackages)); |
There was a problem hiding this comment.
this.scanPackages could be an unmodifiable list. Consider using List.of(scanPackages)
There was a problem hiding this comment.
I also wonder if scanPackages can be a Set (or LinkedHashSet if order matters); duplicated packages are irrelevant.
| return scanPackages; | ||
| } | ||
|
|
||
| public static String[] merge(String[] existingPackages, |
There was a problem hiding this comment.
I would not add this method in this class. It's totally unrelated to class, it just concat two arrays.
I guess it can be moved to the caller class as a private helper method.
| } | ||
|
|
||
| @Test | ||
| void vaadinScanPackagesIsSerializable() throws Exception { |
There was a problem hiding this comment.
This should be already covered by SpringClassesSerializableTest. We don't need to test serialization internals if we do not add any custom logic.
|
Thank you for this contribution, @Hariharan02112004! We continued the work in #26482, which includes your changes and the fixes from the review, with you as co-author. Closing this PR in favour of that one. |
github-project-automation
Bot
moved this from 🔎Iteration reviews
to Done
in Vaadin Flow | Hilla | Kits ongoing work
…26482) Fixes vaadin#25022 **new feature** · vaadin-spring · Spring apps with `@EnableVaadin` on more than one configuration class Applications can now put `@EnableVaadin` with packages on several configuration classes, for example one per module, and Vaadin scans the packages of all of them. Before, a Spring Boot application failed at startup with `BeanDefinitionOverrideException`, and plain Spring kept only the packages of the last annotation. **Risks:** -⚠️ Behaviour change: plain Spring applications with several `@EnableVaadin` annotations now scan all their packages, not only the packages of the last annotation processed - ✅ No public API, security, memory, serialization, threading, performance or migration impact - Changed `VaadinScanPackagesRegistrar` to add the packages of each `@EnableVaadin` to the bean definition that is already registered, instead of registering a second one - The packages keep their order and duplicates are removed - Turned `VaadinScanPackages` into a package-private record that holds an unmodifiable list - Documented in `EnableVaadin` that packages from several annotations are merged - Added `VaadinScanPackagesRegistrarTest`, which runs with bean definition overriding disabled, as in Spring Boot - Based on vaadin#25062 by @Hariharan02112004 --------- Co-authored-by: Hariharan G <167274701+Hariharan02112004@users.noreply.github.com>
Description
This change extracts scan package handling into
VaadinScanPackagesand makes the class serializable.Previously, scan package merging logic was handled inside
VaadinScanPackagesRegistrar. The merge logic is now moved intoVaadinScanPackagesso that scan package handling remains in one place.This also fixes serialization support required for
VaadinScanPackages.Fixes
Fixes #25022
Type of change
Checklist