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:
[email protected]
