Skip to content
This repository has been archived by the owner on Feb 8, 2019. It is now read-only.

IOTA-41: Spring dependency injection for fey performers #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cristofolini
Copy link

Our proposed implementation consists of creating a class that extends the original FeyGenericActor (named FeyGenericSpringActor), which allows for one more argument to be passed within its constructor, which is a String containing a path for the XML file containing that actor's Spring context, which can be an absolute or relative filesystem path (relative to the current VM working directory) or obtained from the classpath (by specifying a "classpath:" prefix); other protocols and prefixes may be supported by the Spring framework. The new actor class has two attributes: an ApplicationContext and its AutowireCapableBeanFactory, which will allow us to instantiate the beans specified in the actor's context.

Other modifications to the fey engine core include the necessary changes related to parsing the orchestration JSON: adding an entry for the actor context's file path on the JSON validator and parser method, along with a conditional check to see if a path to the actor's context was specified in the JSON, so that fey will instantiate a FeyGenericSpringActor instead of a FeyGenericActor.

Lastly, we've created a FeyApplicationContext.xml, which should be the parent context to all actor contexts created, and will contain useful universal declarations. For now, all it does is enable annotation usage in all Spring actors.

@rafaelweingartner
Copy link
Member

rafaelweingartner commented Jul 10, 2017

@cristofolini can you add IOTA-41 in the PR description? Because of the missing "-" Jira is not linking this PR to the ticket.

Also, what about creating an example of Fey actors using spring?
An example can clearly show how to use this feature and also clarify that actors/agents/performs are not managed by Spring, only their dependencies are.

@cristofolini cristofolini changed the title IOTA 41: Spring dependency injection for fey performers IOTA-41: Spring dependency injection for fey performers Jul 10, 2017
@cristofolini
Copy link
Author

@rafaelweingartner I've changed the PR title to include the "-" and now the JIRA ticket is linking to it.

I think an example with Spring actors is a pretty good idea. Let me think something up.

@barbaragomes
Copy link
Contributor

@cristofolini Could you write some unit tests related to this PR?
It would be nice to have some tests that uses the Spring framework.

Copy link
Member

@rafaelweingartner rafaelweingartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a doubt on the type of autowiring it is being used, can you clarify @cristofolini?


override def onStart(): Unit = {
super.onStart()
factory.autowireBeanProperties(this, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By name here will force us to use the @qualifier, right?
What about using by type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd only need a @qualifier if the bean id in the XML and the autowired attribute's name are different.

The difference between by_name and by_type is exactly what it says on the tin. By_name will autowire beans with an id that matches the name, while by_type will look for a bean of the same type. I don't really have strong reasons to go either way. Do you think it might be better to go by_type?

If so I can change that when I get the tests in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do find the by_type better, because then we do not need to care about variable names and object names/ids.

@cristofolini
Copy link
Author

@barbaragomes I've added a couple unit tests, one to check that a context is loaded and one to check that a factory is instatiated. I want to get some feedback on two things:

  • Am I doing this right? I'm completely new to Scala and these are the first tests I ever coded (in Scala, anyway) and mostly based myself on the other tests that are implemented;

  • Anything else that needs testing? Let me know if there's anything else I should cover.

@rafaelweingartner I've changed the autowiring method to by_type.

Copy link
Member

@rafaelweingartner rafaelweingartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristofolini and @barbaragomes,
I really do not understand the test here. It may be due to the fact that Scale looks like an alien language for me, so pardon me if my doubt is silly.

The unit test I see we can write here is regarding the method “getPerformer” from “Ensemble.scala” that Lucas changed. So, if I were to code the test in Java, I would create two tests. One where the parameter “performerInfo.contextPath” is null (maybe others for when the parameter is blank or empty), and check if the code used to instantiate the agent is the code at line 237 (Props method? Object creation?!). Later, I would to a test where the parameter “performerInfo.contextPath” is not empty and check if the code at line 236 is executed. By check if the code is executed, I mean execute a mockito.verify method or a powerMockito.verify calls; or Maybe checking if the result of the method is the result we expected.

However, I do not see that in the current test code. Can you explain the idea behind the test and where the assertions are?

@cristofolini
Copy link
Author

@rafaelweingartner All it does right now is assert that an ApplicationContext and a BeanFactory are created when a proper contextPath is present.

I agree that we should have tests for when it isn't. I can see about writing those as well.

@rafaelweingartner
Copy link
Member

Just to update everyone here.

This PR passed in out simple test cases scenarios, but when we started using it in a production system, it did not work properly. There is a problem on how the class loaders of different agents are being managed. We are working to fix it and then add it here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants