Skip to content

Commit

Permalink
Actually try to inject values when determining if we can resolve a pa…
Browse files Browse the repository at this point in the history
…rameter.

Fix #2
  • Loading branch information
JeffFaer committed Jun 20, 2018
1 parent 3ebbc11 commit adf2f4e
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 4 deletions.
4 changes: 3 additions & 1 deletion disable-extension/pom.xml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<parent>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
import com.google.common.reflect.TypeToken;
import com.google.inject.AbstractModule;
import com.google.inject.BindingAnnotation;
import com.google.inject.ConfigurationException;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Key;
import com.google.inject.Module;
import com.google.inject.ProvisionException;
import java.lang.annotation.Annotation;
import java.lang.reflect.AnnotatedElement;
import java.lang.reflect.Constructor;
Expand Down Expand Up @@ -199,7 +201,16 @@ public boolean supportsParameter(ParameterContext parameterContext,
extensionContext.getTestClass(),
parameter);
Optional<Injector> optInjector = getInjectorForParameterResolution(extensionContext);
return optInjector.map(injector -> injector.getExistingBinding(key)).isPresent();
return optInjector.filter(injector -> {
try {
injector.getInstance(key);

This comment has been minimized.

Copy link
@Warfront1

Warfront1 Jun 20, 2018

Is this really the best way to determine if we can get an instance? If obtaining the instance is a heavy call this can result in overhead (in some cases alot more than you would want). IE. a DB connection or some other heavy instance. Also it isn't really intuitive that a method that is supposed to determine if you can retrieve an instance of something, actually attempts to retrieve an instance.

This comment has been minimized.

Copy link
@JeffFaer

JeffFaer Jun 20, 2018

Author Owner

AFAICT, this is the simplest way to determine if an instance is injectable. The public API exposed by Injector doesn't provide anything to determine if a given type is injectable unless you actually try to provision an instance of it.

  1. We could cache this instance and retrieve it in resolveParameter
  2. You're not supposed to do any IO in providers
  3. Expensive objects tend to be scoped, and the overhead is much smaller in that case.
return true;
} catch (ConfigurationException | ProvisionException e) {
// If we throw a ParameterResolutionException here instead of returning false, we'll block
// other ParameterResolvers from being able to work.
return false;
}
}).isPresent();
}

@Override
Expand Down Expand Up @@ -250,7 +261,8 @@ private static Key<?> getKey(Optional<Class<?>> containingElement, Parameter par
}

/**
* @throws IllegalArgumentException if the given element has more than one binding annotation.
* @throws IllegalArgumentException if the given element has more than one binding
* annotation.
*/
private static Optional<? extends Annotation> getOnlyBindingAnnotation(AnnotatedElement element) {
return Optional.ofNullable(Iterables.getOnlyElement(getBindingAnnotations(element), null));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,14 @@ void canInjectQualifiedParameters(@SomeBindingAnnotation String bound) {
assertThat(bound).isEqualTo(TestModule.BOUND);
}

@Test
void canInjectJustInTimeBindings(
@SomeBindingAnnotation String bound,
ImplicitlyInjectable justInTime) {
assertThat(justInTime).isNotNull();
assertThat(justInTime.getArg()).isEqualTo(bound);
}

@SuppressWarnings("unused")
@IncludeModule(TestModule.class)
static abstract class BaseType {
Expand Down Expand Up @@ -151,7 +159,13 @@ void moduleConstructorThrowsException(String string) {}
@Cause(type = ParameterResolutionException.class, message = "No ParameterResolver")
)
@Test
void doNotHaveBinding(NegativeExamples examples) {}
void doesNotHaveInjectConstructor(NotInjectable.Arg arg) {}

@ExpectFailure(
@Cause(type = ParameterResolutionException.class, message = "No ParameterResolver")
)
@Test
void cannotBeInjected(NotInjectable notInjectable) {}
}

static final class FooBarExtension implements ParameterResolver {
Expand Down Expand Up @@ -214,4 +228,26 @@ public BadModule2() {
@Override
protected void configure() {}
}

private static class ImplicitlyInjectable {
private String arg;

@Inject
ImplicitlyInjectable(@SomeBindingAnnotation String arg) {
this.arg = arg;
}

public String getArg() {
return arg;
}
}

private static class NotInjectable {
@Inject
NotInjectable(Arg arg) {}

private static class Arg {
private Arg(String s) {}
}
}
}

0 comments on commit adf2f4e

Please sign in to comment.