Skip to content

Commit a74df1a

Browse files
authored
fix(geolocation): replace setClient with Lookup-based factory SPI (#24259)
## 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 - `GeolocationClient` → `public 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 - [x] `mvn test -pl :flow-server -Dtest='*Geolocation*'` — 38/38 pass locally. - [x] No `@SuppressWarnings("NullAway")` introduced; constructor assigns the `client` field directly. - [x] 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
1 parent 724cc06 commit a74df1a

5 files changed

Lines changed: 157 additions & 28 deletions

File tree

flow-server/src/main/java/com/vaadin/flow/component/geolocation/BrowserGeolocationClient.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@
3636
/**
3737
* {@link GeolocationClient} implementation backed by the real browser
3838
* Geolocation API via {@code window.Vaadin.Flow.geolocation} and DOM events.
39-
* This is the default implementation injected at facade construction time;
40-
* external browserless test drivers replace it via
41-
* {@link Geolocation#setClient(GeolocationClient)}.
39+
* This is the default implementation used when no
40+
* {@link GeolocationClientFactory} is registered through
41+
* {@link com.vaadin.flow.di.Lookup Lookup}.
4242
* <p>
4343
* <b>Framework internal.</b> Application code does not reference this class
4444
* directly.

flow-server/src/main/java/com/vaadin/flow/component/geolocation/Geolocation.java

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@
2323

2424
import com.vaadin.flow.component.Component;
2525
import com.vaadin.flow.component.UI;
26+
import com.vaadin.flow.di.Lookup;
2627
import com.vaadin.flow.function.SerializableConsumer;
28+
import com.vaadin.flow.server.VaadinContext;
29+
import com.vaadin.flow.server.VaadinService;
2730
import com.vaadin.flow.signals.Signal;
2831

2932
/**
@@ -116,14 +119,17 @@ public class Geolocation implements Serializable {
116119
* {@link UI#getGeolocation()} and should not instantiate this class
117120
* directly — attempting to create a second instance for a UI that already
118121
* has one throws.
122+
* <p>
123+
* The underlying {@link GeolocationClient} is resolved through
124+
* {@link Lookup}: if a {@link GeolocationClientFactory} is registered, its
125+
* {@link GeolocationClientFactory#create(UI)} produces the client.
126+
* Otherwise the built-in browser-backed client is used.
119127
*
120128
* @param ui
121129
* the UI this facade belongs to
122130
* @throws IllegalStateException
123131
* if the UI already has a Geolocation facade
124132
*/
125-
@SuppressWarnings("NullAway") // setClient always assigns client; the guard
126-
// throw above it exits before client is used
127133
public Geolocation(UI ui) {
128134
if (ui.getGeolocation() != null) {
129135
throw new IllegalStateException(
@@ -133,12 +139,40 @@ public Geolocation(UI ui) {
133139
this.ui = ui;
134140
this.availabilityReadOnly = ui.getInternals()
135141
.getGeolocationAvailabilitySignal().asReadonly();
142+
this.client = resolveClient(ui);
143+
wireClient(this.client);
144+
}
145+
146+
private static GeolocationClient resolveClient(UI ui) {
147+
GeolocationClientFactory factory = lookupFactory(ui);
148+
if (factory != null) {
149+
return factory.create(ui);
150+
}
136151
GeolocationAvailability seed = ui.getInternals()
137152
.getGeolocationAvailabilitySignal().peek();
138153
if (seed == null) {
139154
seed = GeolocationAvailability.UNKNOWN;
140155
}
141-
setClient(new BrowserGeolocationClient(ui, seed));
156+
return new BrowserGeolocationClient(ui, seed);
157+
}
158+
159+
private static @Nullable GeolocationClientFactory lookupFactory(UI ui) {
160+
VaadinService service = VaadinService.getCurrent();
161+
if (service == null && ui.getSession() != null) {
162+
service = ui.getSession().getService();
163+
}
164+
if (service == null) {
165+
return null;
166+
}
167+
VaadinContext context = service.getContext();
168+
if (context == null) {
169+
return null;
170+
}
171+
Lookup lookup = context.getAttribute(Lookup.class);
172+
if (lookup == null) {
173+
return null;
174+
}
175+
return lookup.lookup(GeolocationClientFactory.class);
142176
}
143177

144178
/**
@@ -296,24 +330,16 @@ public Signal<GeolocationAvailability> availabilitySignal() {
296330
return availabilityReadOnly;
297331
}
298332

299-
/**
300-
* Replaces this facade's geolocation client.
301-
*
302-
* @param client
303-
* the replacement client, never null
304-
*/
305333
void setClient(GeolocationClient client) {
306-
if (this.client != null) {
307-
this.client.close();
308-
}
334+
this.client.close();
309335
this.client = client;
310-
wireAvailability(client);
336+
wireClient(client);
311337
}
312338

313-
private void wireAvailability(GeolocationClient activeClient) {
339+
private void wireClient(GeolocationClient client) {
314340
ui.getInternals()
315-
.setGeolocationAvailability(activeClient.currentAvailability());
316-
activeClient.subscribeAvailability(
341+
.setGeolocationAvailability(client.currentAvailability());
342+
client.subscribeAvailability(
317343
next -> ui.getInternals().setGeolocationAvailability(next));
318344
}
319345
}

flow-server/src/main/java/com/vaadin/flow/component/geolocation/GeolocationClient.java

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,29 @@
3030
* position data — the browser in production, an in-memory test driver in unit
3131
* tests.
3232
* <p>
33+
* <b>Framework internal.</b> Application code does not implement this interface
34+
* directly. Replacement clients are installed at facade construction time by
35+
* registering a {@link GeolocationClientFactory} through Vaadin's
36+
* {@link com.vaadin.flow.di.Lookup Lookup}; {@link Geolocation} then hands the
37+
* resulting client every {@link #get}, {@link #startWatch} and
38+
* {@link #subscribeAvailability} call. When no factory is registered,
39+
* {@code Geolocation} uses the built-in browser-backed client.
40+
* <p>
3341
* <b>Threading:</b> all callbacks on this interface (the future returned by
3442
* {@link #get}, the {@code onUpdate} consumer passed to {@link #startWatch},
3543
* and the {@code onChange} consumer passed to {@link #subscribeAvailability})
3644
* must be invoked on the UI thread.
3745
*/
3846
@NullMarked
39-
interface GeolocationClient extends Serializable {
47+
public interface GeolocationClient extends Serializable {
4048

4149
/**
4250
* Issues a one-shot position request. The future completes once the client
4351
* has an answer (a position or an error).
52+
*
53+
* @param options
54+
* tuning options, or {@code null} for browser defaults
55+
* @return a future that completes with the outcome on the UI thread
4456
*/
4557
CompletableFuture<GeolocationOutcome> get(
4658
@Nullable GeolocationOptions options);
@@ -49,6 +61,15 @@ CompletableFuture<GeolocationOutcome> get(
4961
* Starts a watch session bound to {@code owner}. Position and error pushes
5062
* are delivered via {@code onUpdate}. The returned handle is used to stop
5163
* the watch and to query whether it is still active.
64+
*
65+
* @param owner
66+
* the component that owns this watch; detaching the component
67+
* does not auto-stop the watch — the caller is responsible
68+
* @param options
69+
* tuning options, or {@code null} for browser defaults
70+
* @param onUpdate
71+
* consumer invoked on the UI thread for every push
72+
* @return a handle for stopping the watch
5273
*/
5374
WatchHandle startWatch(Component owner,
5475
@Nullable GeolocationOptions options,
@@ -57,22 +78,28 @@ WatchHandle startWatch(Component owner,
5778
/**
5879
* Subscribes to availability changes. The returned registration removes the
5980
* subscription.
81+
*
82+
* @param onChange
83+
* consumer invoked on the UI thread for every availability
84+
* change
85+
* @return a registration that removes the subscription when called
6086
*/
6187
Registration subscribeAvailability(
6288
SerializableConsumer<GeolocationAvailability> onChange);
6389

6490
/**
6591
* Returns the most recently observed availability. Implementations must
6692
* seed an initial value at construction; the result is never null.
93+
*
94+
* @return the current availability
6795
*/
6896
GeolocationAvailability currentAvailability();
6997

7098
/**
71-
* Releases any resources held by this client. Called when the facade is
72-
* replacing one client with another (e.g. when the test controller is
73-
* installed) and on UI detach. Idempotent: calling more than once is a
74-
* no-op. After {@code close()}, the behavior of {@link #get} and
75-
* {@link #startWatch} is undefined and the facade must not call them.
99+
* Releases any resources held by this client. Called on UI detach.
100+
* Idempotent: calling more than once is a no-op. After {@code close()}, the
101+
* behavior of {@link #get} and {@link #startWatch} is undefined and the
102+
* facade must not call them.
76103
*/
77104
void close();
78105

@@ -91,6 +118,8 @@ interface WatchHandle extends Serializable {
91118
/**
92119
* Returns whether the watch is currently active (has not yet been
93120
* stopped or auto-cancelled).
121+
*
122+
* @return {@code true} if the watch is still receiving updates
94123
*/
95124
boolean isActive();
96125
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/*
2+
* Copyright 2000-2026 Vaadin Ltd.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
5+
* use this file except in compliance with the License. You may obtain a copy of
6+
* the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12+
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13+
* License for the specific language governing permissions and limitations under
14+
* the License.
15+
*/
16+
package com.vaadin.flow.component.geolocation;
17+
18+
import java.io.Serializable;
19+
20+
import org.jspecify.annotations.NullMarked;
21+
22+
import com.vaadin.flow.component.UI;
23+
import com.vaadin.flow.di.Lookup;
24+
25+
/**
26+
* <b>Framework internal.</b> Factory SPI that produces
27+
* {@link GeolocationClient} instances per {@link UI}, resolved via
28+
* {@link Lookup} when a {@link Geolocation} facade is constructed. When a
29+
* factory is registered the resulting client replaces the built-in
30+
* browser-backed client for every {@code UI} in the application; when none is,
31+
* {@code Geolocation} uses the browser-backed client.
32+
* <p>
33+
* Used by external browserless test drivers to swap the production wire client
34+
* for an in-memory driver in environments where package-private cross-JAR
35+
* access is unreliable (split-classloader topologies such as Quarkus).
36+
* Application code does not reference this interface directly. May be renamed
37+
* or removed in a future release.
38+
*/
39+
@NullMarked
40+
public interface GeolocationClientFactory extends Serializable {
41+
42+
/**
43+
* Creates a {@link GeolocationClient} for the given UI. Called once per UI,
44+
* the first time {@link UI#getGeolocation()} is invoked.
45+
*
46+
* @param ui
47+
* the UI for which the client is created
48+
* @return a client that will receive every {@code Geolocation} call for
49+
* that UI; never null
50+
*/
51+
GeolocationClient create(UI ui);
52+
}

flow-server/src/test/java/com/vaadin/flow/component/geolocation/GeolocationClientSeamTest.java

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,14 @@
2222
import org.jspecify.annotations.Nullable;
2323
import org.junit.jupiter.api.BeforeEach;
2424
import org.junit.jupiter.api.Test;
25+
import org.mockito.Mockito;
2526

2627
import com.vaadin.flow.component.Component;
2728
import com.vaadin.flow.component.Tag;
29+
import com.vaadin.flow.component.UI;
30+
import com.vaadin.flow.di.Lookup;
2831
import com.vaadin.flow.function.SerializableConsumer;
32+
import com.vaadin.flow.server.VaadinService;
2933
import com.vaadin.flow.shared.Registration;
3034
import com.vaadin.tests.util.MockUI;
3135

@@ -36,9 +40,11 @@
3640
import static org.junit.jupiter.api.Assertions.assertTrue;
3741

3842
/**
39-
* Tests the {@link Geolocation#setClient(GeolocationClient)} +
40-
* {@link GeolocationTracker#handle()} seam — the framework-internal API surface
41-
* used by external browserless test drivers.
43+
* Tests the {@link GeolocationClient} seam — both the public
44+
* {@link GeolocationClientFactory} extension point that external test drivers
45+
* and native bridges register through {@link Lookup}, and the package-private
46+
* {@link Geolocation#setClient(GeolocationClient)} replacement that
47+
* flow-server's own tests use.
4248
*/
4349
class GeolocationClientSeamTest {
4450

@@ -53,6 +59,22 @@ void setUp() {
5359
private static class TestComponent extends Component {
5460
}
5561

62+
@Test
63+
void lookupFactory_resolvedAtConstruction_clientReceivesGetCalls() {
64+
FakeClient fake = new FakeClient();
65+
VaadinService service = VaadinService.getCurrent();
66+
Lookup lookup = service.getContext().getAttribute(Lookup.class);
67+
Mockito.when(lookup.lookup(GeolocationClientFactory.class))
68+
.thenReturn(unused -> fake);
69+
70+
UI freshUi = new MockUI();
71+
freshUi.getGeolocation().get(outcome -> {
72+
});
73+
74+
assertEquals(1, fake.getCalls.size(),
75+
"factory-produced client should receive get() calls");
76+
}
77+
5678
@Test
5779
void setClient_routesGetThroughInstalledClient() {
5880
FakeClient fake = new FakeClient();

0 commit comments

Comments
 (0)