New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/constructor level injection #126

Merged

Kuroshii merged 19 commits into develop from feature/constructor-level-injection

Jun 2, 2017

Collaborator

AFrieze commented Apr 19, 2017

Added constructor injection for consumers.

Note: I thought about pulling this functionality out into its own set of classes, or even incorporating an existing DI framework. I decided that was a bit more than I wanted to take on right now and kept code changes to a minimum. Could be considerations for the future though.

Andrew Frieze added 8 commits

April 10, 2017 09:44


Initial work

83f333b


Merge branch 'develop' into feature/constructor-level-injection

89dda6c


Added constructor injection to Consumers

4f8b725


Removed 2 unused classes

52a5a24


Added ability to inject values provided by Services

e511a88


Added check in unit test for injected map of values

4ef6ab8


Merge branch 'develop' into feature/constructor-level-injection

95b769b


Javadoc and formatting

1a31ddd

AFrieze requested review from Kuroshii and dcshock

April 19, 2017 15:09

Kuroshii reviewed

View reviewed changes

core/classpath.txt Outdated

		@@ -0,0 +1 @@
		/Applications/activator-dist-1.3.7/repository/com.google.guava/guava/18.0/bundles/guava.jar:/Users/afrieze/.ivy2/cache/com.fasterxml.jackson.core/jackson-databind/bundles/jackson-databind-2.7.3.jar:/Users/afrieze/.ivy2/cache/com.fasterxml.jackson.core/jackson-annotations/bundles/jackson-annotations-2.7.0.jar:/Users/afrieze/.ivy2/cache/com.fasterxml.jackson.core/jackson-core/bundles/jackson-core-2.7.3.jar:/Users/afrieze/.ivy2/cache/com.fasterxml.jackson.datatype/jackson-datatype-jsr310/bundles/jackson-datatype-jsr310-2.7.3.jar:/Users/afrieze/.ivy2/cache/ch.qos.logback/logback-classic/jars/logback-classic-1.0.13.jar:/Users/afrieze/.ivy2/cache/ch.qos.logback/logback-core/jars/logback-core-1.0.13.jar:/Users/afrieze/.ivy2/cache/org.slf4j/slf4j-api/jars/slf4j-api-1.7.5.jar:/Users/afrieze/.ivy2/cache/org.reflections/reflections/jars/reflections-0.9.10.jar:/Applications/activator-dist-1.3.7/repository/org.javassist/javassist/3.18.2-GA/bundles/javassist.jar:/Users/afrieze/.ivy2/cache/com.google.code.findbugs/annotations/jars/annotations-2.0.1.jar:/Applications/activator-dist-1.3.7/repository/javax.inject/javax.inject/1/jars/javax.inject.jar

Collaborator

Kuroshii Apr 20, 2017

This should be ignored or deleted, right?

Kuroshii reviewed

View reviewed changes

core/src/test/java/forklift/consumer/injection/ConsumerTest.java

+ public void jsonConstructorInjection() throws Exception {
+ ConsumerService service = new ConsumerService(ServiceBeanResolver.class);
+ Consumer test = new Consumer(ConstructorJsonConsumer.class, forklift, this.getClass().getClassLoader());
+ test.setServices(Arrays.asList(service));

Collaborator

Kuroshii Apr 20, 2017

Maybe a little cleaner as addServices

Collaborator

Kuroshii Apr 20, 2017

Minor: Also, maybe it'd be nice to test service-based injection separately from forklift-based injection

Collaborator Author

AFrieze Apr 20, 2017

I think it's worth considering a refactor where the injection code is removed from the consumer entirely(this is what I started to do initially).

The setServices method was already in place and I tried to stick with adding the new functionality and not go down a refactoring rabbit hole.

Kuroshii reviewed

View reviewed changes

core/src/test/java/forklift/consumer/injection/ConsumerTest.java

		@@ -1,15 +1,18 @@
		package forklift;
		package forklift.consumer.injection;

Collaborator

Kuroshii Apr 20, 2017

Hey, it's not all injection tests!

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java

@@ @@ -213,7 +222,7 @@ else if (queue != null) @@
  log.info("Creating thread pool of {}", multiThreaded.value());
  blockQueue = new ArrayBlockingQueue<>(multiThreaded.value() * 100 + 100);
  threadPool = new ThreadPoolExecutor(
- multiThreaded.value(), multiThreaded.value(), 5L, TimeUnit.MINUTES, blockQueue);
+  multiThreaded.value(), multiThreaded.value(), 5L, TimeUnit.MINUTES, blockQueue);

Collaborator

Kuroshii Apr 20, 2017

That indentation seems a bit excessive.

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

+ Constructor<?>[] constructors = msgHandler.getDeclaredConstructors();
+ for (Constructor<?> constructor : constructors) {
+ if (constructor.isAnnotationPresent(Inject.class)) {
+ this.constructor = constructor;

Collaborator

Kuroshii Apr 20, 2017

Maybe worth it to log an error or warning (realistically, logging an error is more immediately useful) when there are multiple constructors annotated with @Inject?

Collaborator Author

AFrieze Apr 20, 2017

+1

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

+ Object[] parameters = new Object[constructorAnnotations.length];
+ int index = 0;
+ for (Annotation[] parameterAnnotations : constructorAnnotations) {
+ //there must be at least one annotation that indicates what we should inject

Collaborator

Kuroshii Apr 20, 2017

Except not, right? If there's no annotation you still handle it fine by treating it like it has Inject

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java

+ // Try to resolve the class from any available BeanResolvers.
+ for (ConsumerService s : this.services) {
+ try {
+ final Object o = s.resolve(mappedClass, null);

Collaborator

Kuroshii Apr 20, 2017

So we don't want name-based resolution? I guess it's worked so far, but isn't it a bit limiting to be unable to distinguish objects of the same type?

Collaborator Author

AFrieze Apr 25, 2017

I'm avoiding making changes to the existing functionality.

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java

+ break;
+ }
+ } catch (Exception e) {
+ log.debug("", e);

Collaborator

Kuroshii Apr 20, 2017

I know this was existing code, but aren't errors like "failed to autowire dependency" pretty useful?

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

  public Class<?> getMsgHandler() {
  return msgHandler;
  }
+ /**
+ * Creates an instance of hte MessageHandler class utilized by this constructor. Constructor and Field level injection is performed using both the

Collaborator

Kuroshii Apr 20, 2017

"hte"

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

@@ @@ -242,11 +251,12 @@ public void messageLoop(ForkliftConsumerI consumer) { @@
  ForkliftMessage consumerMsg;
  while ((consumerMsg = consumer.receive(2500)) != null && running.get()) {
  try {
- final Object handler = msgHandler.newInstance();

Collaborator

Kuroshii Apr 20, 2017

Not really a fan of newlines at the start of a block

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java

  /**
  * Inject the data from a forklift message into an instance of the msgHandler class.
- * @param msg containing data
+ *
+ * @param msg containing data

Collaborator

Kuroshii Apr 20, 2017

Doesn't really improve the vague documentation to add some spaces

Collaborator Author

AFrieze Apr 25, 2017

the space is to align with the param below that isn't shown :(

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java

+ return parameters;
+ }
+ private Object getInjectableValue(Annotation decorator, String mappedName, Class<?> mappedClass, ForkliftMessage msg) throws IOException {

Collaborator

Kuroshii Apr 20, 2017

Thought for later: maybe this could be separated out better with a map of handlers or something

Collaborator Author

AFrieze Apr 20, 2017

+1

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

		@@ -362,112 +384,12 @@ public void setOutOfMessages(java.util.function.Consumer<Consumer> outOfMessages
		fields.get(clazz).forEach(f -> {

Collaborator

Kuroshii Apr 20, 2017

Can we take this opportunity to not call this variable f?

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

+ *
+ * @param msg
+ */
+ public Object getMsgHandlerInstance(ForkliftMessage msg) throws InvocationTargetException, IOException, InstantiationException, IllegalAccessException {

Collaborator

Kuroshii Apr 20, 2017

Is this method just public for internal testing, or what?

Collaborator Author

AFrieze Apr 20, 2017

Yes. Copying how the inject method and testing works.

Kuroshii reviewed

View reviewed changes

core/src/main/java/forklift/consumer/Consumer.java Outdated

+ *
+ * @param msg
+ */
+ public Object getMsgHandlerInstance(ForkliftMessage msg) throws InvocationTargetException, IOException, InstantiationException, IllegalAccessException {

Collaborator

Kuroshii Apr 20, 2017 •

edited

Loading

I can't imagine most callers to these methods having a meaningful way of handling all of these exceptions differently. Maybe just wrap them in one exception?

Probably even let it be unchecked; 'cause fail-fast

Collaborator Author

AFrieze Apr 20, 2017

Good catch. The "Inject" method throws a runtime exception so I'l do the same

Kuroshii reviewed

View reviewed changes

Collaborator

Kuroshii left a comment

Mostly just some minor nits; with a few changes should be good to go.

Andrew Frieze added 4 commits

April 20, 2017 16:19


Deleting ignored classpath file

9093eb3


Added error message on when multiple constructors are found annotated…

68693de

… with Inject


javadoc correction

bb59107


Removed newline at start of try block

f4bc978

Andrew Frieze added 3 commits

April 20, 2017 16:51


Modified the error handling of getMsgHandlerInstance

b5dc880


Removed bad comment

477741d


renamed f to field for readability

508c6e3

Kuroshii approved these changes

View reviewed changes

Collaborator

Kuroshii left a comment •

edited

Loading

LGTM

P.S. But there's still an indentation problem, please fix it

Andrew Frieze and others added 3 commits

May 16, 2017 08:13


added param javadoc

5d72585


Merge pull request #131 from dcshock/fix-ci-tests

afeaa5c

Fix Tests on Travis and Circle CI


Merge branch 'develop' into feature/constructor-level-injection

4ae22ce

Kuroshii force-pushed the develop branch from 1e5b124 to 8667a0a Compare

June 2, 2017 20:02


Merge branch 'develop' into feature/constructor-level-injection

1ad6e4f

Kuroshii force-pushed the feature/constructor-level-injection branch from cee304d to 1ad6e4f Compare

June 2, 2017 20:14

Kuroshii merged commit 4ddd0da into develop

Kuroshii deleted the feature/constructor-level-injection branch

June 9, 2017 21:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment