jgallimore commented on a change in pull request #422: Creates Unit test to
CmpJpaConversion
URL: https://github.com/apache/tomee/pull/422#discussion_r261120405
##########
File path:
container/openejb-core/src/main/java/org/apache/openejb/config/CmpJpaConversion.java
##########
@@ -257,15 +256,11 @@ private PersistenceUnit findCmpPersistenceUnit(final
AppModule appModule) {
}
private String getPersistenceModuleId(final AppModule appModule) {
- if (appModule.getModuleId() != null) {
- return
Optional.ofNullable(appModule.getModuleUri().toString()).orElse(appModule.getModuleId());
- }
- for (final EjbModule ejbModule : appModule.getEjbModules()) {
- if (ejbModule.getModuleId() != null) {
- return
Optional.ofNullable(ejbModule.getModuleUri().toString()).orElse(ejbModule.getModuleId());
- }
+ if (appModule.getModuleUri() != null) {
+ return appModule.getModuleUri().toString();
+ } else {
+ return appModule.getModuleId();
}
- throw new IllegalStateException("Comp must be in an ejb module, this
one has none: " + appModule);
Review comment:
Did a bit of digging.
Here's the original method, from 2014, before we started changing it
recently.
```
private String getPersistenceModuleId(final AppModule appModule) {
if (appModule.getModuleId() != null) {
return appModule.getModuleId();
}
for (final EjbModule ejbModule : appModule.getEjbModules()) {
return ejbModule.getModuleId();
}
throw new IllegalStateException("Comp must be in an ejb module, this
one has none: " + appModule);
}
```
Its been through a bit of change, so TOMEE-2330 highlighted that EclipseLink
failed with the CMP->JPA mapping, because EclipseLink is expecting a URL.
https://issues.apache.org/jira/browse/TOMEE-2330. The PR is here:
https://github.com/apache/tomee/pull/266 and included an Arquillian test.
That change, in turn, had an issue where the jarLocation didn't work as a
URL on Windows, and this change was introduced:
https://github.com/apache/tomee/commit/ef70d15575ce88fda8757611cff2355205c1f9ab#diff-1111ec9cebd0c538791adc807cf5a23b
Along the way, we've added fallbacks to module ID, and introduced some Java
8 stuff. There's additional confusion as to whether we should return the ejb
module url/id as opposed to the app module, under certain conditions.
This change is either losing some logic, or we're working on the basis that
the appmodule ID is *never* null.
There's been a few changes, so I appreciate its not fair that its your PR
that gets the extra scrutiny, but now's probably a good time to comment this
method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
