Skip to content

fix(geolocation): replace setClient with Lookup-based factory SPI#24259

Merged
heruan merged 2 commits intomainfrom
fix/geolocation-client-lookup-spi
May 5, 2026
Merged

fix(geolocation): replace setClient with Lookup-based factory SPI#24259
heruan merged 2 commits intomainfrom
fix/geolocation-client-lookup-spi

Conversation

@heruan
Copy link
Copy Markdown
Member

@heruan heruan commented May 5, 2026

Summary

  • Promotes GeolocationClient to a public SPI and adds a GeolocationClientFactory extension point resolved through com.vaadin.flow.di.Lookup.
  • Geolocation queries the factory at construction time and falls back to the built-in browser-backed client when none is registered.
  • setClient is retained as a package-private in-JAR seam for flow-server's own tests; external test drivers and native bridges register a factory via META-INF/services.

Why

Follow-up to #24211. The setClient seam (originally package-private) breaks under split-classloader topologies (Quarkus, OSGi, JPMS): the JVM scopes runtime packages by classloader, so a class in flow-server.jar and one in an external JAR with the same package name end up in different runtime packages when their JARs load through different classloaders. Cross-JAR package-private access then throws IllegalAccessError at class definition time. Reproduced locally and in CI on browserless-test PR #51:

java.lang.IllegalAccessError: class com.vaadin.flow.component.geolocation.BrowserlessGeolocationClient
  cannot access its superinterface com.vaadin.flow.component.geolocation.GeolocationClient
  (BrowserlessGeolocationClient is in unnamed module of loader QuarkusClassLoader @34eb5d01;
   GeolocationClient            is in unnamed module of loader QuarkusClassLoader @272778ae)

Lookup is classloader-aware and explicitly designed for this kind of pluggable SPI — it's the canonical Flow pattern (InstantiatorFactory, RoutePathProvider, ResourceProvider, StaticFileHandlerFactory, BrowserLiveReloadAccessor).

Changes

  • GeolocationClientpublic interface with proper SPI Javadoc (no longer "framework internal").
  • GeolocationClientFactory (new) → public interface, resolved via Lookup; documents the use cases (in-memory test drivers; native bridges in Cordova/Electron/Capacitor shells).
  • Geolocation constructor calls resolveClient(ui) → looks up a factory via VaadinService.getCurrent()Lookup.lookup(GeolocationClientFactory.class); falls back to new BrowserGeolocationClient(ui, seed) when no factory is registered.
  • setClient stays package-private — flow-server's own tests use it; external code goes through Lookup.
  • BrowserGeolocationClient Javadoc updated to mention it's the no-factory default.
  • GeolocationClientSeamTest now exercises both the public Lookup-factory path and the in-package setClient seam.

No META-INF/services file is shipped from flow-server: production has no factory and uses the fallback. Implementations live in external JARs (e.g. browserless-test-shared).

Test plan

  • mvn test -pl :flow-server -Dtest='*Geolocation*' — 38/38 pass locally.
  • No @SuppressWarnings("NullAway") introduced; constructor assigns the client field directly.
  • Local end-to-end verification with browserless-test patched to register a factory: Quarkus suite that previously threw IllegalAccessError now passes (junit6 21/21, quarkus 11/11, including QuarkusBrowserlessBaseClassTest and QuarkusUnitSecurityTest that were failing).
  • Reviewer to confirm SPI shape (factory vs. direct service) is acceptable for Flow's public API surface.

cc @mcollovati

Promote GeolocationClient to a public SPI and add a
GeolocationClientFactory extension point resolved through
com.vaadin.flow.di.Lookup. Geolocation queries the factory at
construction time and falls back to the built-in browser-backed client
when none is registered.

Replaces the package-private Geolocation#setClient seam introduced in
1e432d2. Package-private cross-JAR access is unreliable under split-
classloader topologies (Quarkus, OSGi, JPMS): the JVM scopes runtime
packages by classloader, so a class in flow-server.jar and one in an
external JAR with the same package string land in different runtime
packages once their JARs load through different classloaders. Lookup is
classloader-aware and explicitly designed for this kind of pluggable
SPI.

setClient stays package-private as an in-JAR seam for flow-server's own
tests; external test drivers and native bridges (Cordova, Electron,
Capacitor) register a GeolocationClientFactory via META-INF/services.

Fixes #24211 follow-up (browserless-test Quarkus IllegalAccessError).
heruan added a commit to vaadin/browserless-test that referenced this pull request May 5, 2026
Stop calling the package-private Geolocation#setClient seam from
GeolocationSimulator.forUI; instead ship a BrowserlessGeolocationClientFactory
implementation of Flow's new GeolocationClientFactory SPI, registered via
META-INF/services. Flow's Lookup discovers it during bootstrap, so the
in-memory client is installed automatically the first time UI's constructor
creates the Geolocation facade — for any UI subclass, including custom ones.

The factory also publishes the GeolocationSimulator on the UI via
ComponentUtil.setData; GeolocationSimulator.forUI is now a pure lookup that
throws if the simulator is missing.

The eager GeolocationSimulator.forUI(this) call in MockedUI.<init> is
removed: the factory has already installed the simulator during super()
before the init block runs.

Fixes the Quarkus IllegalAccessError caused by package-private cross-JAR
access under split-classloader topologies. Requires the matching Flow
change in vaadin/flow#24259.
@github-actions github-actions Bot added the +0.1.0 label May 5, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Test Results

 1 396 files  ±0   1 396 suites  ±0   1h 15m 56s ⏱️ -40s
10 089 tests +1  10 019 ✅ +1  70 💤 ±0  0 ❌ ±0 
10 564 runs  +1  10 485 ✅ +1  79 💤 ±0  0 ❌ ±0 

Results for commit 5be9d25. ± Comparison against base commit 9ac549e.

♻️ This comment has been updated with latest results.

Comment thread flow-server/src/main/java/com/vaadin/flow/component/geolocation/Geolocation.java Outdated
Address review feedback on #24259:

- Mark GeolocationClientFactory and the SPI Javadoc on
  BrowserGeolocationClient and GeolocationClient as "Framework internal".
- Replace META-INF/services-specific wording with a generic reference to
  Lookup, since vaadin-spring and vaadin-quarkus install their own Lookup
  implementations that do not use service files.
- Drop the speculative "native bridges in Cordova/Electron/Capacitor"
  use case from GeolocationClientFactory; document only the actual driver
  (browserless test drivers under split-classloader topologies).

Also fix an NPE in Geolocation.lookupFactory observed in CI: some test
scaffolding (Mockito-mocked VaadinService) returns null from getContext().
Treat a null context as "no factory registered" and fall back to the
built-in browser-backed client.
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 5, 2026

@github-actions github-actions Bot added +1.0.0 and removed +0.1.0 labels May 5, 2026
@heruan heruan marked this pull request as ready for review May 5, 2026 09:01
@heruan heruan enabled auto-merge May 5, 2026 09:02
@heruan heruan added this pull request to the merge queue May 5, 2026
Merged via the queue into main with commit a74df1a May 5, 2026
31 checks passed
@heruan heruan deleted the fix/geolocation-client-lookup-spi branch May 5, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants