solomax commented on code in PR #111:
URL: https://github.com/apache/openjpa/pull/111#discussion_r1220924707
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Constraint.java:
##########
@@ -14,7 +14,7 @@
* "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.
Review Comment:
I believe all trailing spaces should be removed
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java:
##########
@@ -920,72 +905,60 @@ private static Column[] createKeyColumns(ForeignKey fk) {
return new Column[] { fkCol, pkCol };
}
- @Override
- public boolean equals(Object o) {
Review Comment:
same comment to hashcode/equals
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Constraint.java:
##########
@@ -27,8 +27,10 @@
*
* @author Abe White
*/
-public abstract class Constraint extends ReferenceCounter {
- private static final long serialVersionUID = 1L;
Review Comment:
IMO this code
```
public abstract class Constraint extends ReferenceCounter {
private static final long serialVersionUID = 1L;
```
should be restored
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Constraint.java:
##########
@@ -250,31 +241,48 @@ public String toString() {
return "<" + name.toLowerCase() + ">";
}
- @Override
Review Comment:
IMO previous version of hashcode+equals was more readable
also it not clear why to change the order :)
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java:
##########
@@ -183,45 +182,45 @@ public boolean isLogical() {
*/
public boolean isPrimaryKeyAutoAssigned() {
if (_autoAssign != null)
- return _autoAssign;
+ return _autoAssign.booleanValue();
return isPrimaryKeyAutoAssigned(new ArrayList(3));
}
/**
- * Helper to calculate whether this foreign key depends on auto-assigned
+ * Helper to calculate whether this foreign key depends on auto-assigned
* columns. Recurses appropriately if the primary key columns this key
* joins to are themselves members of a foreign key that is dependent on
- * auto-assigned columns. Caches calculated auto-assign value as a side
+ * auto-assigned columns. Caches calculated auto-assign value as a side
* effect.
*
* @param seen track seen foreign keys to prevent infinite recursion in
* the case of foreign key cycles
*/
private boolean isPrimaryKeyAutoAssigned(List seen) {
- if (_autoAssign != null)
- return _autoAssign;
+ if (_autoAssign != null)
+ return _autoAssign.booleanValue();
Column[] cols = getPrimaryKeyColumns();
if (cols.length == 0) {
_autoAssign = Boolean.FALSE;
return false;
}
- for (Column column : cols) {
Review Comment:
Could you please clarify why `for-in` loops were replaced with `plain-for`
loops in this PR?
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java:
##########
@@ -40,8 +40,9 @@
*
* @author Abe White
*/
-public class ForeignKey extends Constraint {
- private static final long serialVersionUID = 1L;
Review Comment:
IMo this code:
```
public class ForeignKey extends Constraint {
private static final long serialVersionUID = 1L;
```
need to be restored
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Constraint.java:
##########
@@ -187,7 +181,6 @@ public DBIdentifier getIdentifier() {
* constraint already belongs to a table.
* @deprecated
*/
- @Deprecated
Review Comment:
Why `@Deprecated` were removed in this PR?
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java:
##########
@@ -810,58 +795,58 @@ public DBIdentifier loadIdentifierFromDB(DBDictionary
dbdict, Connection conn) {
DBIdentifier retVal = DBIdentifier.NULL;
try{
Schema schema = getTable().getSchema();
- ForeignKey[] fks = dbdict.getImportedKeys(conn.getMetaData(),
- DBIdentifier.newCatalog(conn.getCatalog()),
schema.getIdentifier(),
+ ForeignKey[] fks = dbdict.getImportedKeys(conn.getMetaData(),
+ DBIdentifier.newCatalog(conn.getCatalog()),
schema.getIdentifier(),
getTable().getIdentifier(), conn, false);
- for (ForeignKey fk : fks) {
- Table localtable = schema.getTable(fk.getTableIdentifier());
+ for ( int i=0; i< fks.length; i++) {
+ Table localtable =
schema.getTable(fks[i].getTableIdentifier());
Table pkTable = schema.getTable(
- fk.getPrimaryKeyTableIdentifier());
+ fks[i].getPrimaryKeyTableIdentifier());
boolean addFK = false;
ForeignKey fkTemp = localtable.getForeignKey(
- fk.getIdentifier());
- if (fkTemp == null) {
- addFK = true;
+ fks[i].getIdentifier());
+ if( fkTemp == null) {
+ addFK=true;
fkTemp = localtable.addForeignKey(
- fk.getIdentifier());
- fkTemp.setDeferred(fk.isDeferred());
- fkTemp.setDeleteAction(fk.getDeleteAction());
- }
- if (fk.getColumns() == null || fk.getColumns().length == 0) {
- // Singular column foreign key
- if (!fkTemp.containsColumn(
- localtable.getColumn(fk.getColumnIdentifier())))
-
fkTemp.join(localtable.getColumn(fk.getColumnIdentifier()),
-
pkTable.getColumn(fk.getPrimaryKeyColumnIdentifier()));
+ fks[i].getIdentifier());
+ fkTemp.setDeferred(fks[i].isDeferred());
+ fkTemp.setDeleteAction(fks[i].getDeleteAction());
}
- else {
+ if (fks[i].getColumns() == null || fks[i].getColumns().length
== 0) {
+ // Singular column foreign key
+ if( ! fkTemp.containsColumn(
+ localtable.getColumn(fks[i].getColumnIdentifier())))
+
fkTemp.join(localtable.getColumn(fks[i].getColumnIdentifier()),
+
pkTable.getColumn(fks[i].getPrimaryKeyColumnIdentifier()));
+ } else {
// Add the multi-column foreign key, joining local and pk
columns in
// the temporary key
- Column[] locCols = fk.getColumns();
- Column[] pkCols = fk.getPrimaryKeyColumns();
+ Column[] locCols = fks[i].getColumns();
+ Column[] pkCols = fks[i].getPrimaryKeyColumns();
// Column counts must match
if (locCols != null && pkCols != null &&
- locCols.length != pkCols.length) {
+ locCols.length != pkCols.length) {
Log log = dbdict.getLog();
if (log.isTraceEnabled()) {
log.trace(_loc.get("fk-column-mismatch"));
}
}
for (int j = 0; j < locCols.length; j++) {
- if (!fkTemp.containsColumn(
-
localtable.getColumn(locCols[j].getIdentifier()))) {
-
fkTemp.join(localtable.getColumn(locCols[j].getIdentifier()),
-
pkTable.getColumn(pkCols[j].getIdentifier()));
+ if( ! fkTemp.containsColumn(
Review Comment:
`if (...)` is used in the code
not `if( ...)`
could you please revert this change?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
