You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/11/08 06:34:36 UTC
[GitHub] [fineract] xurror opened a new pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
xurror opened a new pull request #928:
URL: https://github.com/apache/fineract/pull/928
https://issues.apache.org/jira/browse/FINERACT-849
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-633181452
Ok. I'll create sub-tasks and open PRs for each.
I've already opened this https://github.com/apache/fineract/pull/932 as a first sub-task
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-633077963
@awasum, @vorburger, @Anh3h, @Sanyam96 your first impressions on the work done so will be of great help.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] github-actions[bot] closed pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #928:
URL: https://github.com/apache/fineract/pull/928
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-666083005
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429565424
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/GLAccount.java
##########
@@ -41,14 +41,14 @@
@Table(name = "acc_gl_account", uniqueConstraints = { @UniqueConstraint(columnNames = { "gl_code" }, name = "acc_gl_code") })
public class GLAccount extends AbstractPersistableCustom {
- @ManyToOne(fetch = FetchType.LAZY)
+ @ManyToOne(fetch=FetchType.LAZY)
@JoinColumn(name = "parent_id")
private GLAccount parent;
@Column(name = "hierarchy", nullable = true, length = 50)
private String hierarchy;
- @OneToMany(fetch = FetchType.LAZY)
+ @OneToMany
Review comment:
Unwanted. Will remediate to that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429564984
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/GLAccount.java
##########
@@ -73,7 +73,7 @@
@Column(name = "description", nullable = true, length = 500)
private String description;
- @ManyToOne(fetch = FetchType.LAZY)
+ @ManyToOne
Review comment:
This was unwanted. I will revert all of this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429565218
##########
File path: fineract-provider/src/main/java/org/apache/fineract/adhocquery/domain/AdHoc.java
##########
@@ -60,6 +60,10 @@
@Column(name = "IsActive", nullable = false)
private boolean isActive = false;
+ public AdHoc() {
+ super();
Review comment:
Ok. I saw an issue on that on jira. I will pick up on that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #928: Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r450017056
##########
File path: fineract-provider/config/checkstyle/spotbugs-exclude.xml
##########
@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<FindBugsFilter>
+ <Match>
+ <Bug pattern="ES_COMPARING_PARAMETER_STRING_WITH_EQ" />
Review comment:
Ping @xurror , will you like to address this feedback?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r434641988
##########
File path: fineract-provider/build.gradle
##########
@@ -504,6 +500,16 @@ task integrationTest(type:Test) {
classpath = project.sourceSets.integrationTest.runtimeClasspath
}
+tasks.withType(JavaExec) {
Review comment:
I think there's also an option to use build time weaving with EclipseLink - just like with OpenJPA. Not sure what are the pros and cons though...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429556660
##########
File path: fineract-provider/build.gradle
##########
@@ -41,9 +42,7 @@ buildscript {
'org.zeroturnaround:gradle-jrebel-plugin:1.1.10',
'org.springframework.boot:spring-boot-gradle-plugin:2.2.7.RELEASE'
// below
- classpath 'org.apache.openjpa:openjpa:3.1.1' // when upgrading, also change OpenJPA version repeated below in dependencyManagement!
- classpath 'com.radcortez.gradle:openjpa-gradle-plugin:3.1.0'
- classpath 'org.nosphere.apache:creadur-rat-gradle:0.6.0'
+ classpath 'gradle.plugin.org.nosphere.apache:creadur-rat-gradle:0.3.1'
Review comment:
don't revert the RAT plugin version here
##########
File path: fineract-provider/build.gradle
##########
@@ -21,6 +21,7 @@ Run as:
gradle clean bootRun'''
project.ext.jerseyVersion = '1.19.4'
+project.ext.springVersion = '5.2.6.RELEASE'
Review comment:
I personally feel that using such variables only adds value if versions are used more than once, to avoid repetition and problems when upgrade one and forgetting about another. For versions that are only used once, like this one for the `mavenBom`, I find this harder to read - it's simpler to just leave it specfied in-place, like all other versions are.
##########
File path: fineract-provider/build.gradle
##########
@@ -504,6 +500,16 @@ task integrationTest(type:Test) {
classpath = project.sourceSets.integrationTest.runtimeClasspath
}
+tasks.withType(JavaExec) {
Review comment:
Oh great, so instead of OpenJPA's build time enhancement, we'll have to use a Java Agent with EclipseLink?! You'll have to add this e.g. in the `Dockerfile` as well. And will you be around to explain to everyone deploying Fineract WAR into a standalone Tomcat that they need to add this? I'm sure there will regular questions about this on the mailing list in the future...
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/GLAccount.java
##########
@@ -41,14 +41,14 @@
@Table(name = "acc_gl_account", uniqueConstraints = { @UniqueConstraint(columnNames = { "gl_code" }, name = "acc_gl_code") })
public class GLAccount extends AbstractPersistableCustom {
- @ManyToOne(fetch = FetchType.LAZY)
+ @ManyToOne(fetch=FetchType.LAZY)
@JoinColumn(name = "parent_id")
private GLAccount parent;
@Column(name = "hierarchy", nullable = true, length = 50)
private String hierarchy;
- @OneToMany(fetch = FetchType.LAZY)
+ @OneToMany
Review comment:
that lazy is most likely there for a good reason, and just removing it will have significant impact on performance. Remember FINERACT-902 ? Do we HAVE to remove it to move to EclipseLink? Why?
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V316__address_module_tables_metadat.sql
##########
@@ -80,12 +80,12 @@ AUTO_INCREMENT=1
-- field configuration
CREATE TABLE `m_field_configuration` (
`id` BIGINT NOT NULL AUTO_INCREMENT,
- `entity` VARCHAR(100) NOT NULL,
- `subentity` VARCHAR(100) NOT NULL,
- `field` VARCHAR(100) NOT NULL,
+ `entity` varchar(100) NOT NULL,
Review comment:
there are A LOT of (uppercase) `VARCHAR` in the DB scripts - why do we have to change them for EclipseLink? Better avoid touching what you can if you don't have to, IMHO. If absolutely required, raise incremental small PRs. In this particular, I doubt EclipseLink really needs this.
##########
File path: fineract-provider/build.gradle
##########
@@ -110,6 +107,12 @@ dependencyManagement {
dependency 'org.dom4j:dom4j:2.1.0'
dependency 'org.awaitility:awaitility:4.0.2'
dependency 'com.github.spotbugs:spotbugs-annotations:4.0.3'
+ dependency 'org.aspectj:aspectjweaver:1.9.5'
Review comment:
if EclipseLink needs this, shouldn't it already be it dependency? Or is it an optional one?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/GLAccount.java
##########
@@ -41,14 +41,14 @@
@Table(name = "acc_gl_account", uniqueConstraints = { @UniqueConstraint(columnNames = { "gl_code" }, name = "acc_gl_code") })
public class GLAccount extends AbstractPersistableCustom {
- @ManyToOne(fetch = FetchType.LAZY)
+ @ManyToOne(fetch=FetchType.LAZY)
Review comment:
avoid pure whitespace formatting changes if you can (especially inconsistent ones), it's easier to review without such diff
##########
File path: fineract-provider/build.gradle
##########
@@ -258,7 +254,8 @@ configurations {
providedCompile
compile() {
exclude module: 'hibernate-entitymanager'
- exclude module: 'hibernate-validator'
+ exclude module: 'hibernate-validator'
+ exclude module: 'hibernate-core'
Review comment:
nit pick: not nicely indented
##########
File path: fineract-provider/src/main/java/org/apache/fineract/adhocquery/domain/AdHoc.java
##########
@@ -60,6 +60,10 @@
@Column(name = "IsActive", nullable = false)
private boolean isActive = false;
+ public AdHoc() {
+ super();
Review comment:
if EclipseLink requires these, what I would do if I were you, to avoid conflicts with other PRs such as the Checkstyle work, is to raise a small PR focused on "add missing default constructors [FINERACT-849]" just to already get this done and in; it will likely be easier for you, and make future PRs smaller and easier for us to review.
##########
File path: fineract-provider/dependencies.gradle
##########
@@ -82,12 +88,12 @@ dependencies {
exclude group: 'org.apache.geronimo.specs'
}
implementation ('org.springframework.boot:spring-boot-starter-data-jpa') {
- exclude group: 'org.hibernate'
+ exclude group: 'org.hibernate', module: 'hibernate-entitymanager'
Review comment:
huh, this is curious - doesn't `exclude group: 'org.hibernate'` already exclude all `org.hibernate` ? Why this change?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/adhocquery/domain/AdHoc.java
##########
@@ -35,7 +35,7 @@
@Table(name = "m_adhoc")
public class AdHoc extends AbstractAuditableCustom {
- @Column(name = "name", length = 100)
+ @Column(name = "name", length = 100)
Review comment:
avoid making formatting changes (this will likely be done as part of the ongoing Checkstyle work anyway)
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/GLAccount.java
##########
@@ -73,7 +73,7 @@
@Column(name = "description", nullable = true, length = 500)
private String description;
- @ManyToOne(fetch = FetchType.LAZY)
+ @ManyToOne
Review comment:
as above
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/creditbureau/domain/CreditBureau.java
##########
@@ -31,29 +30,30 @@
@Table(name = "m_creditbureau")
public class CreditBureau extends AbstractPersistableCustom {
-
+ @Column(name = "name", nullable = false)
Review comment:
I'm curious and would like to learn why this is required? the name is the name, no change. The nullable false why? Because it's NOT NULL in the DB schema? Does EclipseLink actually validate that? This, and some of the other changes you seem to have to make (below), IMHO would be a clean up change worth making in a smaller separate PR already...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-638870714
Additionally, thanks to @ptuomola I rethought my approach to weaving and figured I could do something similar to openjpa enhance. So we now have a task to weave classes, thus javaAgent is no longer necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] Anh3h commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
Anh3h commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-633174567
@xurror I think the PR is too large, making PR review daunting and would cause a lot of code conflicts. Can you break it down into smaller deliverables?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r444219362
##########
File path: fineract-provider/src/main/java/org/apache/fineract/template/domain/TemplateMapper.java
##########
@@ -23,6 +23,7 @@
import javax.persistence.Table;
import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+@org.gaul.modernizer_maven_annotations.SuppressModernizer
Review comment:
Another side effect of weaving
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-671003312
Weird story but I tried to look at the schedularjobs thing but they just started failing with the timeout as soon as I set up EclipseLink configuration without the saveAndFlush changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on a change in pull request #928: Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r450017986
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java
##########
@@ -3629,6 +3637,7 @@ public void testLoanScheduleWithInterestRecalculation_WITH_REST_WEEKLY_INTEREST_
}
+ @Disabled // FINERACT-1050
Review comment:
Fix these before we merge? There are too many to just merge this without the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] github-actions[bot] commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-698043605
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r446122936
##########
File path: fineract-provider/config/checkstyle/spotbugs-exclude.xml
##########
@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<FindBugsFilter>
+ <Match>
+ <Bug pattern="ES_COMPARING_PARAMETER_STRING_WITH_EQ" />
Review comment:
Perhaps instead of globally disabled there is a way to only have it exclude for `**.domain.**` classes? @percyashu @thesmallstar
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-641168819
I'm taking a short break on this just to clear my head off a little bit. I'll be working on other issues in the meantime.
BTW, I've also started a thread on [stackoverflow](https://stackoverflow.com/questions/62263241/spring-jpa-with-eclipselink-save-operation-does-not-persist) hoping to get some tips from eclipselink and spring JPA regulars.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror closed pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror closed pull request #928:
URL: https://github.com/apache/fineract/pull/928
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r444208329
##########
File path: fineract-provider/config/checkstyle/spotbugs-exclude.xml
##########
@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<FindBugsFilter>
+ <Match>
+ <Bug pattern="ES_COMPARING_PARAMETER_STRING_WITH_EQ" />
Review comment:
Unfortunately EclipseLink weaving makes some changes to classes which violate this particular spotbugs rule. Given that I can't fix this without disabling weaving/enhancement, I thought it best to just ignore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-638240287
I fear I've hit a stalement on this. I can't seem to make any progress. I get a consistent 500 server error. I was able to determine that data does not get persisted but no eclipselink is not throwing any error as of why it doesn't.
After streamlining and focusing my tests on a single test case(OfficeIntegrationTest), I was able to determine the server error is due to a null pointer exception at this [line](https://github.com/apache/fineract/blob/3521b0ed334831dbb4ab0db9216a493c532982db/fineract-provider/src/main/java/org/apache/fineract/organisation/office/service/OfficeWritePlatformServiceJpaRepositoryImpl.java#L102). but [line](https://github.com/apache/fineract/blob/3521b0ed334831dbb4ab0db9216a493c532982db/fineract-provider/src/main/java/org/apache/fineract/organisation/office/service/OfficeWritePlatformServiceJpaRepositoryImpl.java#L100) does not persist any data. Probably the reason for the exception.
Any suggestions to workaround this will be very much appreciated.
FYI: @vorburger @awasum @maektwain @ptuomola @Sanyam96 @Anh3h
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-650126648
Wow. First of all, this is impressive. Congratulations.
I have some concerns that the bulk replace of `save()` by `saveAndFlush()` could actually lead to worse performance.
About the disabled tests, I'm wondering whether we shouldn't fix those before merging this... one of them in particular (FINERACT-1051) seems to have something to do re. some "Pre-Closure maturity amount" being suddenly different, and nother one is a failing SQL (FINERACT-1050) and the third seems to indicate regression with the scheduler (FINERACT-1048), which with much pain we just fixed... it seems to me that if we merge this PR now, we knowingly break Fineract again, no?
If in the meantime you already wanted to get a sub-set of this PR merged just to avoid future merge conflicts, e.g. those `JpaSystemException` (which is a Spring and not an EclipseLink specific class) you had to add could make a separate standalone PR which we could already merge before the rest of this (if you like, just a thought & suggestion).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429564643
##########
File path: fineract-provider/build.gradle
##########
@@ -21,6 +21,7 @@ Run as:
gradle clean bootRun'''
project.ext.jerseyVersion = '1.19.4'
+project.ext.springVersion = '5.2.6.RELEASE'
Review comment:
Was doing some tests. Missed to revert that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-649409427
This finally passes the build too. Hoping to get your reviews soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429565361
##########
File path: fineract-provider/dependencies.gradle
##########
@@ -82,12 +88,12 @@ dependencies {
exclude group: 'org.apache.geronimo.specs'
}
implementation ('org.springframework.boot:spring-boot-starter-data-jpa') {
- exclude group: 'org.hibernate'
+ exclude group: 'org.hibernate', module: 'hibernate-entitymanager'
Review comment:
I had some errors when I excluded the whole group. Seems hibernate and eclipse have some few dependencies in common.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-638866912
Good news. The JPA is actually able to read data from the DB. Which is actually proof that the connection to the DB is fine. Left with writing. I'd investigate further to try to find the source of the problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429564740
##########
File path: fineract-provider/build.gradle
##########
@@ -110,6 +107,12 @@ dependencyManagement {
dependency 'org.dom4j:dom4j:2.1.0'
dependency 'org.awaitility:awaitility:4.0.2'
dependency 'com.github.spotbugs:spotbugs-annotations:4.0.3'
+ dependency 'org.aspectj:aspectjweaver:1.9.5'
Review comment:
It is needed to enable weaving which eclipse requires inorder to be able to support OneToOne and ManyToOne LAZY fetchtype. You can refer here: https://docs.spring.io/spring/docs/5.2.5.RELEASE/spring-framework-reference/core.html#aop-aj-ltw-aop_dot_xml
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r450149846
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java
##########
@@ -3629,6 +3637,7 @@ public void testLoanScheduleWithInterestRecalculation_WITH_REST_WEEKLY_INTEREST_
}
+ @Disabled // FINERACT-1050
Review comment:
Ok. I have no problem handling this now. Just didn't want the PR to have too many different changes for easy review. Will work on it this week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #928:
URL: https://github.com/apache/fineract/pull/928#discussion_r429564939
##########
File path: fineract-provider/build.gradle
##########
@@ -504,6 +500,16 @@ task integrationTest(type:Test) {
classpath = project.sourceSets.integrationTest.runtimeClasspath
}
+tasks.withType(JavaExec) {
Review comment:
I'll be around. We can also document it as part of fineract dependencies.
I would have ignored this without any problem but it's required to make LAZY fetchtype work with eclipselink
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror edited a comment on pull request #928: [WIP] Migrate ORM from OpenJPA to EclipseLink(Fineract 849)
Posted by GitBox <gi...@apache.org>.
xurror edited a comment on pull request #928:
URL: https://github.com/apache/fineract/pull/928#issuecomment-638240287
I fear I've hit a stalemate on this. I can't seem to make any progress. I get a consistent 500 server error. I was able to determine that data does not get persisted but no eclipselink is not throwing any error as of why it doesn't.
After streamlining and focusing my tests on a single test case(OfficeIntegrationTest), I was able to determine the server error is due to a null pointer exception at this [line](https://github.com/apache/fineract/blob/3521b0ed334831dbb4ab0db9216a493c532982db/fineract-provider/src/main/java/org/apache/fineract/organisation/office/service/OfficeWritePlatformServiceJpaRepositoryImpl.java#L102). but [line](https://github.com/apache/fineract/blob/3521b0ed334831dbb4ab0db9216a493c532982db/fineract-provider/src/main/java/org/apache/fineract/organisation/office/service/OfficeWritePlatformServiceJpaRepositoryImpl.java#L100) does not persist any data. Probably the reason for the exception.
Any suggestions to workaround this will be very much appreciated.
FYI: @vorburger @awasum @maektwain @ptuomola @Sanyam96 @Anh3h
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org