replace use of Context.getService with dependencies and injection

Development
1

Hi there,

while working on addition of missing tests one encounters a lot of Context.getXxxService like for example here:

This has as multiple side effects:

  • its a pain to write a unit tests since you need to use PowerMockRunner to mock the static Context see example of PatientServiceImplTest
  • if you write unit tests using PowerMockRunner it will not show up in coverage due to a limitation in PowerMockRunner/Jacoco see https://issues.openmrs.org/browse/TRUNK-5150
  • since its hard, devs tend to just write/add tests to the BaseContextSensitive test classes, even if you just test simple cases where no DB is needed. This makes our builds take longer than needed
  • I also find a number of services that do not declare other services they need as field, so dependencies are hidden. !!!this sometimes masks cyclic dependencies!!!

Question: Is there any reason we are doing this other than historical bad practice? Is it related to transactions, that when you call another service from a service you want it to be within the same transaction? Because this should already work.

Solution:

  • clean up our Services and declare all services that are needed as fields and let spring inject them
  • create unit test classes using BaseContextMockTest and transition tests that do not need a DB, like throw APIException if given null, … over to those, so in the end we are left with unit tests & integration tests
  • update maven poms so devs can run unit tests only to speed up development
4 Likes
2

Besides getService is synchronized thus very slow! I’d prioritize fixing the implementation to get rid of synchronization.

The pattern is often used when you want to call a service method and run all AOPs from a different method in the same service see e.g. here.

I agree it should be avoided whenever possible. However, there are some places where it’s handy, thus Spring has ApplicationContext.getBean/getBeansOfType as well…

3

@teleivo your recommendations make a lot of sense! We could evaluate them and see the side effects. Hopefully we shall not get into issues like https://issues.openmrs.org/browse/TRUNK-3806

1 Like