Conversation
rsteph-de
left a comment
There was a problem hiding this comment.
MCRRestAPIObjects is getting longer ...
Is there a possibility to separate the Lock-API calls into another class?
This can be done in a follow-up PR which cleans up the REST-API code.
Otherwise O.K.
I'm doing some changes right now anyway. I could also move it separate if you like. Edit: question is where too? |
This could show as "proof-of-concept" - that it is possible to have multiple RESTController classes with the same path-prefix. |
9a645b7 to
03dcc67
Compare
ce67545 to
53bfb63
Compare
mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestObjects.java
Outdated
Show resolved
Hide resolved
mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestObjects.java
Outdated
Show resolved
Hide resolved
mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestObjects.java
Outdated
Show resolved
Hide resolved
mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestV2App.java
Outdated
Show resolved
Hide resolved
mycore-restapi/src/main/java/org/mycore/restapi/v2/service/MCRRestObjectLockService.java
Show resolved
Hide resolved
2d359b8 to
bfc48fe
Compare
df88c10 to
20b0c44
Compare
| @Context | ||
| UriInfo uriInfo; | ||
|
|
||
| @Inject |
There was a problem hiding this comment.
This is the introduction of the Jersey Dependency Injection mechanismn in MyCoRe (first use).
In my opinion this is a architectural decision that adds "complexity" to the code and should be discussed first in an online meeting..
I tend to stay with "our" MCRConfiguration2.getInstance().
There was a problem hiding this comment.
I can see your point. And im ok with changing it.
But in general I prefer the default way and for me that would be using the @Inject method, cause its out of the box available without any boilerplate code (and tbh our property injection stuff comes with a lot of it...). The binding happens in MCRRestV2App. I could see using our property configuration there.
We also use the @singleton annotation from the same package. So we kind of mix the stuff already.
yagee-de
left a comment
There was a problem hiding this comment.
Good work, but a minor issue between interface, implementation and configuration. Maybe the interface is not necessary?
| bind(MCRRestObjectLockServiceImpl.class) | ||
| .to(MCRRestObjectLockService.class); |
There was a problem hiding this comment.
It's either hard coded (no interface) or configurable.
There was a problem hiding this comment.
I can make it configurable.
But an interface is in both cases good. For example if somebody decides to write tests for the MCRRestObjects class. Then you can easily mock the ObjectLockService interface.
2774390 to
46507eb
Compare
- inject lock service for better code separation - use userId as lockId for easier REST API access
58ac831 to
0306a39
Compare
Link to jira.