Trunk 6426: Deprecate the use of Context.get...Service #5346
Conversation
|
Hi @rkorytkowski , |
jona42-ui
changed the title
jona42-ui
force-pushed
the
TRUNK-6426
branch
2 times, most recently
from
94e6bb9 to
ac92dfd
Compare
| public AdministrationServiceImpl(@Lazy MessageSourceService messageSourceService, | ||
| @Lazy SerializationService serializationService, | ||
| @Lazy ConceptService conceptService, | ||
| @Lazy AdministrationService self) { |
There was a problem hiding this comment.
@rkorytkowski I think any dependency that would cause BeanCurrentlyInCreationException needs @Lazy to defer resolution until after the creation cycle completes. so we could decide at what level we break one link in the circular chain.
just a spoiler :) tagging @lazy seems to be a github account somewhere : )
| @Transactional(readOnly = true) | ||
| public String getGlobalProperty(String propertyName, String defaultValue) throws APIException { | ||
| String s = Context.getAdministrationService().getGlobalProperty(propertyName); | ||
| String s = getGlobalProperty(propertyName); |
There was a problem hiding this comment.
It should be self.getGlobalProperty(propertyName). The reason is that if you just call getGlobalProperty, then no authorization or logging AOPs are applied.
If you see Context.get...Service within the same service you can assume that you need to call it via self.... to have all AOPs.
| this.serializationService = serializationService; | ||
| this.conceptService = conceptService; | ||
| this.self = self; | ||
| } |
There was a problem hiding this comment.
Let's have an extra constructor without a self parameter that assigns this.self = this; so that the class can be easily mocked and is not fully dependent on Spring.
| @Override | ||
| public void setGlobalProperty(String propertyName, String propertyValue) throws APIException { | ||
| GlobalProperty gp = Context.getAdministrationService().getGlobalPropertyObject(propertyName); | ||
| GlobalProperty gp = getGlobalPropertyObject(propertyName); |
| @Override | ||
| public void updateGlobalProperty(String propertyName, String propertyValue) throws IllegalStateException { | ||
| GlobalProperty gp = Context.getAdministrationService().getGlobalPropertyObject(propertyName); | ||
| GlobalProperty gp = getGlobalPropertyObject(propertyName); |
| for (GlobalProperty prop : props) { | ||
| if (prop.getProperty() != null && prop.getProperty().length() > 0) { | ||
| Context.getAdministrationService().saveGlobalProperty(prop); | ||
| saveGlobalProperty(prop); |
| public void purgeGlobalProperties(List<GlobalProperty> globalProperties) throws APIException { | ||
| for (GlobalProperty globalProperty : globalProperties) { | ||
| Context.getAdministrationService().purgeGlobalProperty(globalProperty); | ||
| purgeGlobalProperty(globalProperty); |
| public CohortServiceImpl(@Lazy CohortService self) { | ||
| this.self = self; | ||
| } | ||
|
|
There was a problem hiding this comment.
Move all autowired fields to the constructor.
| formValidator = new FormValidator(); | ||
| @Autowired | ||
| public FormServiceImpl(@Lazy ObsService obsService, | ||
| @Lazy AdministrationService administrationService) { |
There was a problem hiding this comment.
Are you sure it doesn't work without @Lazy?
| private final AdministrationService administrationService; | ||
|
|
||
| @Autowired | ||
| public LocationServiceImpl(@Lazy AdministrationService administrationService) { |
|
@rkorytkowski thanks i will go through and review concerns, will revert for clarity |
jona42-ui
force-pushed
the
TRUNK-6426
branch
3 times, most recently
from
2a9e47a to
f18e540
Compare
rkorytkowski
left a comment
rkorytkowski
left a comment
There was a problem hiding this comment.
You haven't updated all services to use self.ownMethod...
| public Cohort voidCohort(Cohort cohort, String reason) { | ||
| // other setters done by the save handlers | ||
| return Context.getCohortService().saveCohort(cohort); | ||
| return saveCohort(cohort); |
There was a problem hiding this comment.
Replace with self.saveCohort(cohort); Please use self whenever Context.get{SameAsCalling}Service()....
| @Override | ||
| public Diagnosis voidDiagnosis(Diagnosis diagnosis, String voidReason) { | ||
| return Context.getDiagnosisService().save(diagnosis); | ||
| return save(diagnosis); |
| formValidator = new FormValidator(); | ||
| @Autowired | ||
| public FormServiceImpl(@Lazy ObsService obsService, | ||
| @Lazy AdministrationService administrationService) { |
There was a problem hiding this comment.
Are you sure it doesn't work without @Lazy?
| public void unretireForm(Form form) throws APIException { | ||
| form.setRetired(false); | ||
| Context.getFormService().saveForm(form); | ||
| saveForm(form); |
jona42-ui
force-pushed
the
TRUNK-6426
branch
5 times, most recently
from
b3158ab to
4c32467
Compare
2 participants
Description of what I changed
deprecated Context.get…Service methods in 3.0.0 with:
@deprecated since 3.0.0, use {@link Autowired} instead
Replaced the use of Context.get…Service within core codebase.
Issue I worked on
see https://openmrs.atlassian.net/browse/TRUNK-6426
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master