Skip to content

Commit cda8180

Browse files
ptwohigptwohig
committed
Wire GuiceOptions into GuiceSpiModule, with tests (issue #32)
GuiceOptions (package-level, @SInCE 3.9) existed but was never consumed anywhere -- GuiceSpiModule ignored it entirely. This wires it up as the opt-in escape hatch it was designed to be for third-party Element authors who hit the exact ExposedButNotBound crash reported in #32 (an exported service with no locally-discovered implementation that nothing else happens to bind). - LEGACY (default) is byte-for-byte unchanged: the existing two-pass bind/expose split, verbatim. Every element that doesn't declare @GuiceOptions sees zero behavior change. - GUICE_MODULE_ONLY defers entirely to the author's own @GuiceElementModule(s): every exported service is expose-only, ElementService#implementation() is ignored (per GuiceOptions' own documented contract), avoiding double-defined bindings when an author's module and the SDK's own annotation scanning would otherwise both try to bind the same key. - strict validates that every key deferred to a @GuiceElementModule is actually bound by one, before Guice.createInjector() ever runs -- a clear SdkException naming the exact service instead of Guice's generic [Guice/ExposedButNotBound] error. Has to happen in the constructor rather than configure(): Guice folds exceptions thrown from configure() into its own CreationException, so throwing there never reaches the caller as a clean SdkException. New GuiceSpiModuleLoadingStrategyTest exercises all three paths against real annotated fixture packages (not hand-built records), built the same way the production loader builds them. Not applied anywhere in this repo -- confirmed out of scope, this is purely third-party-facing infrastructure. This repo's actual #32 crash site (sdk-service's MatchmakingApplicationConfigurationService and 8 siblings) is a separate, unrelated fix.
1 parent 529e2ab commit cda8180

14 files changed

Lines changed: 404 additions & 17 deletions

File tree

‎sdk-spi-guice/src/main/java/dev/getelements/elements/sdk/spi/guice/GuiceSpiModule.java‎

Lines changed: 125 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,29 @@
11
package dev.getelements.elements.sdk.spi.guice;
22

3+
import com.google.inject.Binding;
34
import com.google.inject.Key;
5+
import com.google.inject.Module;
46
import com.google.inject.PrivateModule;
7+
import com.google.inject.spi.Elements;
58
import dev.getelements.elements.sdk.Element;
69
import dev.getelements.elements.sdk.ElementRegistry;
710
import dev.getelements.elements.sdk.annotation.ElementServiceImplementation.DefaultImplementation;
11+
import dev.getelements.elements.sdk.exception.SdkException;
812
import dev.getelements.elements.sdk.spi.guice.record.GuiceElementModuleRecord;
13+
import dev.getelements.elements.sdk.spi.guice.record.GuiceOptionsRecord;
914
import dev.getelements.elements.sdk.record.ElementRecord;
1015
import dev.getelements.elements.sdk.record.ElementServiceRecord;
1116
import jakarta.inject.Provider;
1217

1318
import java.util.HashSet;
19+
import java.util.List;
1420
import java.util.Set;
1521

1622
import static com.google.inject.name.Names.bindProperties;
1723
import static com.google.inject.name.Names.named;
24+
import static dev.getelements.elements.sdk.spi.guice.annotations.GuiceOptions.LoadingStrategy.GUICE_MODULE_ONLY;
1825
import static java.util.Objects.requireNonNull;
26+
import static java.util.stream.Collectors.toSet;
1927

2028
/**
2129
* Used to bind services.
@@ -26,11 +34,34 @@ public class GuiceSpiModule extends PrivateModule {
2634

2735
private final ElementRecord elementRecord;
2836

37+
private final GuiceOptionsRecord options;
38+
39+
private final List<Module> guiceElementModules;
40+
2941
public GuiceSpiModule(
3042
final ElementRegistry parent,
3143
final ElementRecord elementRecord) {
44+
3245
this.parent = requireNonNull(parent, "parent");
3346
this.elementRecord = requireNonNull(elementRecord, "elementRecord");
47+
48+
final var elementPackage = elementRecord.definition().pkg();
49+
this.options = GuiceOptionsRecord.fromPackage(elementPackage);
50+
51+
this.guiceElementModules = GuiceElementModuleRecord
52+
.fromPackage(elementPackage)
53+
.map(GuiceElementModuleRecord::newModule)
54+
.toList();
55+
56+
// Validated here, eagerly, rather than inside configure() -- Guice catches exceptions thrown from a
57+
// module's configure() and folds them into its own CreationException alongside whatever other errors
58+
// it collects (e.g. the very [Guice/ExposedButNotBound] error this is meant to preempt), so a throw from
59+
// configure() never actually reaches the caller as a clean SdkException. Throwing here, before
60+
// Guice.createInjector() is ever invoked, does.
61+
if (options.strict()) {
62+
validateDeferredKeysAreBound(elementPackage);
63+
}
64+
3465
}
3566

3667
@Override
@@ -41,33 +72,110 @@ protected void configure() {
4172
binder().requireExplicitBindings();
4273
bindProperties(binder(), attributes);
4374

44-
final var targets = new HashSet<Class<?>>();
45-
final var ownKeys = new HashSet<Key<?>>();
75+
final var targets = new HashSet<Class<?>>();
76+
final var ownKeys = new HashSet<Key<?>>();
4677

47-
elementRecord
48-
.services()
49-
.stream()
50-
.filter(esr -> DefaultImplementation.class.equals(esr.implementation().type()))
51-
.forEach(esr -> exposeService(ownKeys, esr));
78+
if (options.strategy() == GUICE_MODULE_ONLY) {
5279

53-
elementRecord
54-
.services()
55-
.stream()
56-
.filter(esr -> !DefaultImplementation.class.equals(esr.implementation().type()))
57-
.forEach(esr -> bindAndExposeService(targets, ownKeys, esr));
80+
// Defers exclusively to the installed @GuiceElementModule(s); ElementService#implementation() is
81+
// ignored entirely (per GuiceOptions' documented contract) to avoid double-defining bindings the
82+
// author's own module already supplies.
83+
elementRecord.services().forEach(esr -> exposeService(ownKeys, esr));
84+
85+
} else {
86+
87+
elementRecord
88+
.services()
89+
.stream()
90+
.filter(esr -> DefaultImplementation.class.equals(esr.implementation().type()))
91+
.forEach(esr -> exposeService(ownKeys, esr));
92+
93+
elementRecord
94+
.services()
95+
.stream()
96+
.filter(esr -> !DefaultImplementation.class.equals(esr.implementation().type()))
97+
.forEach(esr -> bindAndExposeService(targets, ownKeys, esr));
98+
99+
}
58100

59101
elementRecord
60102
.dependencies()
61103
.stream()
62104
.flatMap(dep -> dep.findDependencies(parent))
63105
.forEach(element -> bindDependentElement(ownKeys, element));
64106

65-
final var elementPackage = elementRecord.definition().pkg();
107+
guiceElementModules.forEach(this::install);
66108

67-
GuiceElementModuleRecord
68-
.fromPackage(elementPackage)
69-
.map(GuiceElementModuleRecord::newModule)
70-
.forEach(this::install);
109+
}
110+
111+
/**
112+
* Computes the set of exported keys that this module will {@code expose()} without binding itself -- i.e. the
113+
* ones relying on something else (an installed {@code @GuiceElementModule}) to have bound them.
114+
*/
115+
private Set<Key<?>> computeDeferredKeys() {
116+
117+
final var deferredKeys = new HashSet<Key<?>>();
118+
119+
if (options.strategy() == GUICE_MODULE_ONLY) {
120+
elementRecord.services().forEach(esr -> deferredKeys.addAll(exportedKeys(esr)));
121+
} else {
122+
elementRecord
123+
.services()
124+
.stream()
125+
.filter(esr -> DefaultImplementation.class.equals(esr.implementation().type()))
126+
.forEach(esr -> deferredKeys.addAll(exportedKeys(esr)));
127+
}
128+
129+
return deferredKeys;
130+
131+
}
132+
133+
/**
134+
* With {@link GuiceOptionsRecord#strict()} enabled, verifies every deferred key (see {@link #computeDeferredKeys()})
135+
* is actually bound by one of the installed {@code @GuiceElementModule}s, raising a clear, actionable error at
136+
* load time instead of letting Guice's generic {@code [Guice/ExposedButNotBound]} error surface later at
137+
* injector-creation time.
138+
*/
139+
private void validateDeferredKeysAreBound(final Package elementPackage) {
140+
141+
final var deferredKeys = computeDeferredKeys();
142+
143+
if (deferredKeys.isEmpty()) {
144+
return;
145+
}
146+
147+
final var boundKeys = Elements
148+
.getElements(guiceElementModules)
149+
.stream()
150+
.filter(element -> element instanceof Binding<?>)
151+
.map(element -> ((Binding<?>) element).getKey())
152+
.collect(toSet());
153+
154+
final var unbound = deferredKeys
155+
.stream()
156+
.filter(key -> !boundKeys.contains(key))
157+
.toList();
158+
159+
if (!unbound.isEmpty()) {
160+
throw new SdkException(
161+
"Element package '" + elementPackage.getName() + "' declares GuiceOptions(strategy=" +
162+
options.strategy() + ", strict=true), but the following exported service(s) are not bound " +
163+
"by any installed @GuiceElementModule: " + unbound + ". Provide an " +
164+
"@ElementServiceImplementation, or add a @GuiceElementModule that explicitly binds them."
165+
);
166+
}
167+
168+
}
169+
170+
private Set<Key<?>> exportedKeys(final ElementServiceRecord elementServiceRecord) {
171+
172+
final var export = elementServiceRecord.export();
173+
174+
final var keys = export.isNamed()
175+
? export.exposed().stream().map(anInterface -> Key.get(anInterface, named(export.name())))
176+
: export.exposed().stream().map(Key::get);
177+
178+
return keys.collect(toSet());
71179

72180
}
73181

‎sdk-spi-guice/src/main/java/dev/getelements/elements/sdk/spi/guice/annotations/GuiceOptions.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
package dev.getelements.elements.sdk.spi.guice.annotations;
2+
3+
import dev.getelements.elements.sdk.annotation.ElementService;
4+
5+
import java.lang.annotation.Retention;
6+
import java.lang.annotation.Target;
7+
8+
import static dev.getelements.elements.sdk.spi.guice.annotations.GuiceOptions.LoadingStrategy.LEGACY;
9+
import static java.lang.annotation.ElementType.PACKAGE;
10+
import static java.lang.annotation.RetentionPolicy.RUNTIME;
11+
12+
/**
13+
* Adds options for the Guice loader SPI. This allows an element author control over how the module's loader works
14+
* and enables/disable certain options making for easier loading.
15+
*
16+
* @since 3.9
17+
*/
18+
@Target(PACKAGE)
19+
@Retention(RUNTIME)
20+
public @interface GuiceOptions {
21+
22+
/**
23+
* Specifies the loading strategy to be used by this element.
24+
*
25+
* @return the {@link LoadingStrategy}
26+
*/
27+
LoadingStrategy strategy() default LEGACY;
28+
29+
/**
30+
* Sets strict validation rules, attempting to raise as many errors at load time as possible. Depends on the
31+
* loading strategy.
32+
*
33+
* @return true, if strict mode is enabled
34+
*/
35+
boolean strict() default false;
36+
37+
/**
38+
* Specifies how the Guice based SPI loads the Element. This here to reduce some of the friction and fagile nature
39+
* of the original legacy loading system which uses a combination of annotations to expose servies. Guice largely
40+
* replaces that, but will often times conflict with itself wasting a lot cycles in the process.
41+
*/
42+
enum LoadingStrategy {
43+
44+
/**
45+
* Legacy loader which preserves the 3.8 and prior behavior honoring all SDK annotations.
46+
*/
47+
LEGACY,
48+
49+
/**
50+
* Defers loading exclusively to all {@link GuiceElementModule}s, ignoring bindings that would otherwise be
51+
* defined in the SDK annotations to avoid doubly defining them. It is still required to export services to
52+
* other {@link dev.getelements.elements.sdk.Element}s using the {@link ElementService}. Note that with this
53+
* strategy set, {@link ElementService#implementation()} is ignored.
54+
*/
55+
GUICE_MODULE_ONLY
56+
57+
}
58+
59+
}
60+

‎sdk-spi-guice/src/main/java/dev/getelements/elements/sdk/spi/guice/record/GuiceOptionsRecord.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
package dev.getelements.elements.sdk.spi.guice.record;
2+
3+
import dev.getelements.elements.sdk.spi.guice.annotations.GuiceOptions;
4+
5+
import static dev.getelements.elements.sdk.spi.guice.annotations.GuiceOptions.LoadingStrategy.LEGACY;
6+
7+
/**
8+
* A record type for {@link GuiceOptions}, resolved from a {@link Package} with the {@link GuiceOptions} annotation
9+
* defaulted to its documented defaults when the annotation is absent.
10+
*
11+
* @param strategy the {@link GuiceOptions.LoadingStrategy}
12+
* @param strict whether strict validation is enabled
13+
*/
14+
public record GuiceOptionsRecord(GuiceOptions.LoadingStrategy strategy, boolean strict) {
15+
16+
/**
17+
* The default options used when a package bears no {@link GuiceOptions} annotation.
18+
*/
19+
public static final GuiceOptionsRecord DEFAULT = new GuiceOptionsRecord(LEGACY, false);
20+
21+
/**
22+
* Resolves the {@link GuiceOptionsRecord} for the supplied {@link Package}, falling back to {@link #DEFAULT}
23+
* if the package bears no {@link GuiceOptions} annotation.
24+
*
25+
* @param aPackage the {@link Package} which may bear the {@link GuiceOptions} annotation
26+
* @return the resolved {@link GuiceOptionsRecord}
27+
*/
28+
public static GuiceOptionsRecord fromPackage(final Package aPackage) {
29+
final var guiceOptions = aPackage.getAnnotation(GuiceOptions.class);
30+
return guiceOptions == null
31+
? DEFAULT
32+
: new GuiceOptionsRecord(guiceOptions.strategy(), guiceOptions.strict());
33+
}
34+
35+
}

‎sdk-spi-guice/src/test/java/dev/getelements/elements/sdk/spi/guice/GuiceSpiModuleLoadingStrategyTest.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
package dev.getelements.elements.sdk.spi.guice;
2+
3+
import com.google.inject.Guice;
4+
import dev.getelements.elements.sdk.exception.SdkException;
5+
import dev.getelements.elements.sdk.spi.DefaultElementLoaderFactory;
6+
import dev.getelements.elements.sdk.spi.RootElementRegistry;
7+
import dev.getelements.elements.sdk.spi.guice.fixture.guicemoduleonly.SharedTestService;
8+
import dev.getelements.elements.sdk.spi.guice.fixture.legacy.LegacyTestService;
9+
import dev.getelements.elements.sdk.spi.guice.fixture.strict.UnboundTestService;
10+
import dev.getelements.elements.sdk.util.SimpleAttributes;
11+
import org.testng.annotations.Test;
12+
13+
import java.util.Map;
14+
15+
import static org.testng.Assert.assertEquals;
16+
import static org.testng.Assert.assertTrue;
17+
import static org.testng.Assert.expectThrows;
18+
19+
/**
20+
* Proves both {@code GuiceOptions.LoadingStrategy} code paths through {@link GuiceSpiModule}: the default
21+
* {@code LEGACY} behavior is unchanged (issue #32 must not regress existing Elements), and the opt-in
22+
* {@code GUICE_MODULE_ONLY} strategy lets a third-party Element author defer entirely to their own
23+
* {@code @GuiceElementModule}, avoiding the exact {@code [Guice/ExposedButNotBound]} crash reported in #32.
24+
*/
25+
public class GuiceSpiModuleLoadingStrategyTest {
26+
27+
private final DefaultElementLoaderFactory factory = new DefaultElementLoaderFactory();
28+
29+
@Test
30+
public void legacyStrategyBindsAndExposesAnnotationDerivedImplementation() {
31+
32+
final var elementRecord = factory.getElementRecordFromPackage(
33+
new SimpleAttributes(Map.of()),
34+
LegacyTestService.class.getPackage()
35+
);
36+
37+
final var injector = Guice.createInjector(new GuiceSpiModule(new RootElementRegistry(), elementRecord));
38+
final var service = injector.getInstance(LegacyTestService.class);
39+
40+
assertEquals(service.get(), "legacy");
41+
42+
}
43+
44+
@Test
45+
public void guiceModuleOnlyStrategyDefersEntirelyToTheInstalledModule() {
46+
47+
final var elementRecord = factory.getElementRecordFromPackage(
48+
new SimpleAttributes(Map.of()),
49+
SharedTestService.class.getPackage()
50+
);
51+
52+
final var injector = Guice.createInjector(new GuiceSpiModule(new RootElementRegistry(), elementRecord));
53+
final var service = injector.getInstance(SharedTestService.class);
54+
55+
// If the annotation-derived AnnotationDerivedImpl binding had not been skipped, Guice would have thrown
56+
// a duplicate-binding CreationException before this point (two bind() calls for the same key).
57+
assertEquals(service.get(), "guice-module-derived");
58+
59+
}
60+
61+
@Test
62+
public void strictModeRaisesAClearErrorForAnUnboundExportedService() {
63+
64+
final var elementRecord = factory.getElementRecordFromPackage(
65+
new SimpleAttributes(Map.of()),
66+
UnboundTestService.class.getPackage()
67+
);
68+
69+
// Expecting SdkException (thrown by GuiceSpiModule's own strict-mode validation, during configure())
70+
// rather than Guice's generic CreationException (thrown later, at injector-creation time) proves the
71+
// new validation actually ran and pre-empted the default failure mode.
72+
final var exception = expectThrows(SdkException.class, () ->
73+
Guice.createInjector(new GuiceSpiModule(new RootElementRegistry(), elementRecord)));
74+
75+
assertTrue(exception.getMessage().contains("UnboundTestService"));
76+
77+
}
78+
79+
}

‎sdk-spi-guice/src/test/java/dev/getelements/elements/sdk/spi/guice/fixture/guicemoduleonly/AnnotationDerivedImpl.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package dev.getelements.elements.sdk.spi.guice.fixture.guicemoduleonly;
2+
3+
import dev.getelements.elements.sdk.annotation.ElementServiceExport;
4+
import dev.getelements.elements.sdk.annotation.ElementServiceImplementation;
5+
6+
/**
7+
* Annotated as though it were the implementation the legacy annotation-driven scan would auto-bind. Under
8+
* {@code GuiceOptions.LoadingStrategy.GUICE_MODULE_ONLY} this must be ignored entirely in favor of whatever
9+
* {@link GuiceModuleOnlyTestFixtureModule} binds -- if it were not ignored, installing both would conflict
10+
* (Guice does not allow the same key to be bound twice).
11+
*/
12+
@ElementServiceExport(SharedTestService.class)
13+
@ElementServiceImplementation
14+
public class AnnotationDerivedImpl implements SharedTestService {
15+
16+
@Override
17+
public String get() {
18+
return "annotation-derived";
19+
}
20+
21+
}

0 commit comments

Comments
 (0)