Repository navigation
chore: import vaadin-cdi as a Flow module and run its tests in validation - #25525
Conversation
Change-Id: If457da33cf5489c9b5cd552827fb60166599cd72
Change-Id: I326af890640a1b3e5fc8d96a2b3407b19785fc95
Using a nested VaadinCDIServlet is allowed. Change-Id: I17261000feef668c7bd48b521eca3e3cf0a531f7
Change-Id: Ice24d701df7d3e0765c1493e1e75af69e7f54666
Change-Id: I17dfa70e98e64ea99bf897c48bdae7aad97a0f9d
Change-Id: Ic8199751125db735968297ae45b9a181c28d0313
Change-Id: Ic4fea717afa7389a17f82c2adf664f62f8198627
Change-Id: I37da96e890363029eddeaf76575100f237db2e21
Change-Id: Ic316f0fcfbbeebf96f5b33c6f34dc9e6fbe48d73
Change-Id: I04e4171168c021da9e690f553b1ff9c70412fdf9
Change-Id: I48f4623c9379c3a2bef11eda5601ace5458de644
Change-Id: Ibbcd930a52675ea1983ab38c027692a6f6758144
Change-Id: I3044360d37d69916e8a8c861539293681d9a5520
Change-Id: I5831d94061fa9070e5c740d742d2df0e905c3288
Change-Id: Id596cd1f38df95796905b057fd3d2818f84427d0
Change-Id: If5c8b606365247a74637174e1c49d49fc9ed510f
Change-Id: Ib5ca7086840fdd6320550522a6cfee504883a861
Change-Id: If4cac244c171bfb292c29f68dabafa54587edc55
Change-Id: Ifb600c961b6d6d95cbdb72cf3282f01a259640d8
Add checks for DenyAll to tighten security and have full support for the javax.security annotations. Change-Id: I20627f4d0bec88a34524e484ffe56ea7076efdda
* Travis build for validation and master snapshots
* Fix path issue in deploy script
* Make sure all tests are run with TomEE 7 Rename tests and update dependencies to make all tests execute. Use a recent Arquillian version and TomEE 7.
Ignore Shiro tests that are causing problems in validation builds with pre-releases - see #186.
The liberty leg failed the DependencyConvergence rule the Flow parent enforces and the CDI parent did not: the Liberty adapter pulls the Arquillian core in at 1.7.0.Final, diverging from what arquillian-junit-container brings, across nine artifacts. Importing arquillian-bom in place of the single managed entry pins all of them. Nearest-wins already resolved them to arquillian.version, so this records the version that was in effect rather than changing it, and it holds for every container profile instead of only the ones that happen not to declare an adapter of their own. Convergence now passes for tomee, wildfly, payara, liberty and tomcat-weld alike.
Declaring maven-antrun-plugin also activated the execution the flow-tests parent manages, which deletes an exploded WAR's WEB-INF/lib. This module is a jar and ShrinkWrap assembles its archives in process, so that directory never exists and the execution printed an Ant BuildException and a stack trace on every run - non-fatal, but indistinguishable from a real failure in the log. Bound to phase none here. The dependencyConvergence rule lives in the enforcer default-cli execution, so no lifecycle phase evaluates it and nothing in CI would have caught the Arquillian divergence the previous commit fixed, or a regression of it. The CDI job now names the goal explicitly, which evaluates it once per container profile - the profiles are what bring the conflicting adapters. Confirmed it fails again if the BOM import is removed.
|
@claude do you see any blockers in this move - any concerns about CI, release, structure problems, test run and similar? |
|
Claude finished @mshabarov's task in 2m 19s —— View job Review: blockers & concerns for the CDI importI read the CI wiring ( 🔴 Likely merge blockers (checks, not code)
🟠 Release / versioning — worth confirming before merge
🟡 CI cost / robustness — by design, but note the trade‑offs
🟢 Things I checked that are fine
One small nit (non‑blocking)
Bottom lineNo structural blocker to the move itself — the build/reactor/CI plumbing is sound. The two things that will actually gate the merge are external to the code: the Sonar quality gate (coverage + reliability on new code) and the CLA. The one decision I'd want made explicitly is the release coordination (old Analysis based on the branch as of |
The bump to 25.4 reached vaadin-cdi but not its test module, which was still declaring flow-tests 25.3-SNAPSHOT as its parent. A clean checkout has no such parent to resolve, so the module would have failed the build rather than joining the reactor at the current version.
Local tooling rewrote the lockfile while building and it was picked up by the previous commit. Nothing in this branch touches flow-client, so this puts the file back to what main has.
Seven of the eight bugs the analysis reports on this import are real and fixable without changing behaviour: - CdiVaadinServlet cleared its servlet-name ThreadLocal with set(null), which leaves an entry attached to the container's pooled thread. Uses remove() instead; getCurrentServletName() still answers null. - RouteScopedContext dereferenced the UI's navigation data without checking for it. A bean that names its owner resolves it from the qualifier, so it reaches that check even before any navigation has happened, and failed with an NPE instead of the IllegalStateException naming the owner and the bean. Covered by a new test, which fails with the NPE when the guard is removed. - BeanManagerProvider held its ClassLoader map in a volatile field that is never reassigned; the map is already concurrent, so it is final now. Its getParentBeanManagerInfo also recursed when getBeanManagerInfo returned null, which it cannot - it creates and stores an entry when one is missing - so that branch was dead. - ClassUtils.extractPossiblyGenericMethod dereferenced a class its own extractMethod explicitly tolerates as null. The eighth, DependentProvider writing its CreationalContext, stays: the declared type is not Serializable but the instances containers supply are, and dropping the write would leave a deserialized provider unable to destroy its @dependent instance. Ignored for that one rule in that one file. Coverage is scoped to exclude com.vaadin.cdi.util, the DeltaSpike-derived plumbing carried over with the import. What exercises it is the Arquillian suite, and the analysis is fed unit-test coverage only, so those lines read as untested no matter how much of them the containers run. The rest of the module is Vaadin's own code and stays measured.
The guard in navigationChainHasOwner is evaluated before the && createIfNotExist short-circuit, so it changed two paths and only the create one was pinned. The other is the one that matters in practice: AbstractContext.get(Contextual) looks storage up with createIfNotExist false, which is how the container resolves an IF_EXISTS observer, and firing such an event before any navigation used to fail with an NPE. Both tests now fail with that NPE if the guard is neutralised. Also corrects the javadoc on getParentBeanManagerInfo, which still described the recursion that was removed and claimed a null return for a ClassLoader hierarchy without a BeanManagerInfo - it creates one, and answers null only for a ClassLoader with no parent. And states the null precondition once at the top of extractPossiblyGenericMethod instead of nesting it inside the fallback branch, matching the shape isProxyableClass already uses.
The job was written against an older base, so its Node and pnpm pins had drifted behind the ones every other job in the file uses. A bump of weld.version in the root pom reaches the tomcat-weld profile through weld-servlet-shaded, so the root pom now counts as a CDI source and runs the gated containers. tomee is ungated and never diffs, so it no longer clones the full history. cleanup-artifacts already reaches cdi-tests through test-results.
vaadin-cdi kept its own release scheme under the same GAV -- 46 releases from 1.0.0.alpha1 to 16.1.2 -- before it started releasing with Flow's version number. On the shared axis those versions land in the gap between Flow 9.2 and 23.0, which breaks the "present" streak the tool derives @SInCE from and rewrites the tags across most of the framework. An isolated index-dir keeps them off it, the same way vaadin-spring is handled.
…output
build-helper:parse-version sets properties whose names end in a question
mark, which is a legal Maven property name but not a legal XML element
name, so help:effective-pom writes tags the strict POM reader ShrinkWrap
uses rejects. Not producing them is cheaper than matching them with a
regular expression that has to guess how Maven serialises an element.
Nothing in this module reads ${parsedVersion.*}: osgi.bundle.version is
its only consumer in the repository and bnd reads that from a bnd.bnd,
which this module does not have. bnd still derives Bundle-Version from
the project version.
The remaining replaceregexp strips the duplicate XML declaration, and
xmlvalidate still fails the build if the output changes shape again --
an illegal element name from another plugin included.
Without a scope the adapter is a compile dependency, so it reaches the compile classpath and the archive ShrinkWrap assembles, neither of which has any use for it. Verified with the liberty profile: 57 tests, all passing. The concurrency API in the tomcat-weld profile has no scope on purpose, which reads the same and is not. Documented, because removing it breaks the module: Tomcat with Weld supplies no Jakarta Concurrency, so the provided scope declared for the rest of the containers has to be promoted to compile there.
The module map pointed at a future test-cdi module as the home for CDI tests. That module is here now under a different name, so both the decision list and the target table point at it. Running the suite locally needs JDK 21, the version validation uses. On JDK 24 and later java.security.Policy.setPolicy throws UnsupportedOperationException, and TomEE calls it while installing its JACC policy provider, so the container never starts and all 57 tests fail with a message that says nothing about CDI.
|
@claude re-check previous comments - what is not yet fixed? |
|
Claude finished @mshabarov's task in 1m 16s —— View job Re-check of earlier comments — what's fixed vs. still openI went back through Artur's review and my 2026‑09‑11 analysis and checked each item against the branch as it stands now ( ✅ Fixed since the earlier comments
All of the code-level items from both threads are in. CI on the latest commit: Formatter ✅, Validation still in progress, no failures. 🟠 Still open — external / not closable by this PR
🟡 Accepted trade-offs (not "unfixed" — decisions to confirm, not change)
Bottom lineEverything that was actionable in the code — Artur's version bump (plus the sibling POM the author caught), the Sonar gate, and all four reliability fixes — is fixed and verified on the branch. The only genuinely outstanding items are the two external ones: the CLA signatures and the release/BOM coordination. Neither can be closed from within this PR. |
|
|
Looks good to me, need to change from squash to merge with a short window. |
|
Thanks @mshabarov — agreed, and there are two separate settings in the way, so flagging both before the window opens.
What is at stake concretely: the import is a merge commit ( No code change for this one; it is purely a settings action and I have no admin rights here. Everything else on the branch is green. |
Brings vaadin/quarkus main into this repository the same way vaadin/cdi was imported in vaadin#25525: a merge of the upstream history so `git log` and `git blame` keep working for the Quarkus extension sources. The extension lands at /vaadin-quarkus, keeping upstream's runtime and deployment split, and the integration tests at /flow-tests/vaadin-quarkus-tests. The relocation happens in this merge commit without touching file contents, so rename detection carries the history across it. The upstream root pom, formatter configuration and validation workflow are replaced by Flow's in the following commits. The ecosystem CI workflow and its test script are kept and adapted there.



Summary
The
vaadin-cdiintegration used to live in its own repository. This moves it into Flow as a normal module, with its full git history, so it builds and releases together with Flow and shares Flow's version number. Its Arquillian test suite moves toflow-tests/vaadin-cdi-testsand now runs in CI.What changed
Behavior change (only for apps using
com.vaadin:vaadin-cdi): two things move for them, even though no Java signature changed.16.1.2.RouteScopedContextnow reports a missing route scope withIllegalStateException(naming the owner and the bean) where it used to fail with a bareNullPointerException. This happens when a bean that names its owner with@RouteScopeOwneris resolved before any navigation on the UI.For everyone else this is additive. No existing Flow source file is touched. The changes outside the two new modules are build and CI wiring only.
Imported as-is:
vaadin-cdi/— 36 main sources incom.vaadin.cdi,.annotation,.contextand.util, plus 65 unit tests in 11 classes (89 executions, because one abstract base class runs against six different scope bindings). Registered in the rootpom.xml, inflow-bom, and in the module tables inCLAUDE.mdandguidelines/repository.md.flow-tests/vaadin-cdi-tests/— the Arquillian suite (17 test classes, 58 tests) plus its test application. Added toflow-tests/pom.xml.Small fixes to the imported code, all behavior-preserving except the one called out above:
CdiVaadinServletclears its servlet-nameThreadLocalwithremove()instead ofset(null), so no entry stays attached to a pooled container thread.getCurrentServletName()still answersnull.RouteScopedContext.navigationChainHasOwnerchecks for missing navigation data instead of dereferencing it.BeanManagerProviderholds itsClassLoadermap in afinalfield (the map is already concurrent, and the field is never reassigned) and drops a dead recursion branch ingetParentBeanManagerInfo.ClassUtils.extractPossiblyGenericMethodtolerates thenullclass that its ownextractMethodalready documents as allowed.DependentProviderkeeps writing itsCreationalContext. The declared type is notSerializable, but the instances containers supply are, and dropping the write would leave a deserialized provider unable to destroy its@Dependentinstance. Sonar'sjava:S2118is ignored for that one file.Build and CI:
cdi-testsjob invalidation.yml, one leg per container:tomee,wildfly,payara,liberty,tomcat-weld.tomeeruns on every change as a smoke test; the other four only when the CDI sources or the rootpom.xmlchange (aweld.versionbump there reaches thetomcat-weldprofile).cleanup-artifactsnow depends on the job and fails the build if it fails.computeMatrix.jsexcludesflow-tests/vaadin-cdi-testsfrom the generic IT matrix, because the tests need a real application server.update-since-tags.ymlgivesvaadin-cdiits own@sinceindex directory, the same wayvaadin-springhas one. Its 46 old releases would otherwise land in the gap between Flow 9.2 and 23.0 and rewrite@sincetags across most of the framework.pom.xmladds managedweld.version/weld.junit.versionand theweld-se-coreandweld-junit5entries the module's unit tests need.build-helper:parse-versionis switched off, because it sets property names ending in?, whichhelp:effective-pomthen writes as illegal XML element names that the strict POM reader ShrinkWrap uses rejects. Nothing in the module reads${parsedVersion.*}. Areplaceregexpstill strips the duplicate XML declaration, andxmlvalidatefails the build if the output changes shape again. The Open Liberty Arquillian adapter is scoped totest.src/main/java/com/vaadin/cdi/util/**. That package is DeltaSpike-derived plumbing that only the container suite exercises, and Sonar is fed unit-test coverage only, so it reads as untested no matter how much of it runs.Running the suite locally needs JDK 21, the version validation uses. On JDK 24 and later
java.security.Policy.setPolicythrows, and TomEE calls it while installing its JACC policy provider, so the container never starts. Both READMEs say so.API Changes
All new to this repository. The code is carried over from the
vaadin-cdirepository unchanged, so nothing here is new to an application that already depends oncom.vaadin:vaadin-cdi. No signature was changed or removed by the import. 37 public types added: 9 incom.vaadin.cdi, 9 incom.vaadin.cdi.annotation, 9 incom.vaadin.cdi.context, 10 incom.vaadin.cdi.util. Nothing is deprecated.com.vaadin.cdi.AbstractCdiInstantiator
com.vaadin.cdi.CdiInstantiator
com.vaadin.cdi.CdiInstantiatorFactory
com.vaadin.cdi.CdiServletDeployer
com.vaadin.cdi.CdiVaadinServlet
com.vaadin.cdi.CdiVaadinServletService
com.vaadin.cdi.CdiVaadinServletService.CdiVaadinServiceDelegate
com.vaadin.cdi.UIDetachEvent
com.vaadin.cdi.VaadinExtension
com.vaadin.cdi.annotation.CdiComponent
com.vaadin.cdi.annotation.UIScoped
com.vaadin.cdi.annotation.NormalUIScoped
com.vaadin.cdi.annotation.RouteScoped
com.vaadin.cdi.annotation.NormalRouteScoped
com.vaadin.cdi.annotation.RouteScopeOwner
com.vaadin.cdi.annotation.VaadinServiceScoped
com.vaadin.cdi.annotation.VaadinSessionScoped
com.vaadin.cdi.annotation.VaadinServiceEnabled
com.vaadin.cdi.context.ContextWrapper
com.vaadin.cdi.context.UIScopedContext
com.vaadin.cdi.context.RouteScopedContext
com.vaadin.cdi.context.VaadinServiceScopedContext
com.vaadin.cdi.context.VaadinSessionScopedContext
com.vaadin.cdi.util.AbstractContext
com.vaadin.cdi.util.AnyLiteral
com.vaadin.cdi.util.BeanManagerProvider
com.vaadin.cdi.util.BeanProvider
com.vaadin.cdi.util.ClassUtils
com.vaadin.cdi.util.ContextUtils
com.vaadin.cdi.util.ContextualInstanceInfo
com.vaadin.cdi.util.ContextualStorage
com.vaadin.cdi.util.DependentProvider
com.vaadin.cdi.util.ProxyUtils
Test summary
@RouteScopeOwnerbefore any navigation on the UI throwsIllegalStateException, notNullPointerExceptionReception.IF_EXISTSbefore any navigation notifies nobody and does not throw&& createIfNotExistshort-circuit, so it also changed the no-create lookup path — the one containers actually takecom.vaadin.cdi.utilat all, and the only check that the import still works per containermvn verifyoverflow-testsfails for everyone; no other job covers this pathCdiVaadinServletleaves noThreadLocalentry afterinitandservicereturn, andgetCurrentServletName()still answersnullCdiVaadinServletTestnever touches the servlet nameClassUtils.extractPossiblyGenericMethod(null, sourceMethod)returnsnullinstead of throwingextractMethoddocumentsnullas allowed, so a caller relying on that gets an NPE if the guard regressesTests added or changed on this branch:
RouteContextualStorageManagerTest.get_noNavigationDataYet_ownedBean_scopeDoesNotExist_Throws→ 1RouteContextualStorageManagerTest.customEvent_noNavigationDataYet_conditionalBean_doesNotThrow→ 2Both fail with the original
NullPointerExceptionif the guard is removed. Test 2 asserts by not throwing — there is no explicitAssertionscall, because the point is that theIF_EXISTSlookup completes and notifies nobody. These two methods are the only new tests in this PR; the other 63 declared unit tests and all 58 Arquillian tests are imported unchanged, including ones whose commits look recent because the import grafted the upstream history.Covered by CI steps rather than JUnit methods:
validation.yml→cdi-tests→ "Run CDI ITs" (enforcer:enforce verify, per container profile) → 3, 5validation.yml→cdi-tests→ "Verify the no-container build skips its tests" → 4xmlvalidatein the test module'ssanitize-effective-pomexecution fails the build ifhelp:effective-pomoutput stops being parseable by ShrinkWrap, instead of failing inside every test's deployment methodDeliberately left untested: the remaining two fixes are behavior-neutral and have nothing to assert — making
BeanManagerProvider's never-reassigned map fieldfinal, and deleting a recursion branch ingetParentBeanManagerInfothat could not be reached. The DeltaSpike-derivedcom.vaadin.cdi.utilpackage has no unit tests by design; only the container suite exercises it, which is why it is excluded from coverage measurement. One inherited weak spot worth knowing about, but out of scope here:CdiVaadinServletServiceTest.init_SystemMessagesProviderMissing_defaultConfiguredasserts aServiceException, which makes the default-provider assertion inside it unreachable.