3.2.2 fix OPENJPA-2814 #111
Conversation
| @@ -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. | |||
There was a problem hiding this comment.
I believe all trailing spaces should be removed
| @@ -27,8 +27,10 @@ | |||
| * | |||
| * @author Abe White | |||
| */ | |||
| public abstract class Constraint extends ReferenceCounter { | |||
| private static final long serialVersionUID = 1L; | |||
There was a problem hiding this comment.
IMO this code
public abstract class Constraint extends ReferenceCounter {
private static final long serialVersionUID = 1L;
should be restored
| @@ -250,31 +241,48 @@ public String toString() { | |||
| return "<" + name.toLowerCase() + ">"; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
IMO previous version of hashcode+equals was more readable
also it not clear why to change the order :)
There was a problem hiding this comment.
I used an auto generated eclipse equals and hashcode. I could mimic the style of the original one indeed. The important thing for me is the fields that are included in the equality and hashcode testing to avoid the mem leak !
| @@ -40,8 +40,9 @@ | |||
| * | |||
| * @author Abe White | |||
| */ | |||
| public class ForeignKey extends Constraint { | |||
| private static final long serialVersionUID = 1L; | |||
There was a problem hiding this comment.
IMo this code:
public class ForeignKey extends Constraint {
private static final long serialVersionUID = 1L;
need to be restored
|
|
||
| Column[] cols = getPrimaryKeyColumns(); | ||
| if (cols.length == 0) { | ||
| _autoAssign = Boolean.FALSE; | ||
| return false; | ||
| } | ||
|
|
||
| for (Column column : cols) { |
There was a problem hiding this comment.
Could you please clarify why for-in loops were replaced with plain-for loops in this PR?
There was a problem hiding this comment.
Yes see my general comment, will update
| @@ -187,7 +181,6 @@ public DBIdentifier getIdentifier() { | |||
| * constraint already belongs to a table. | |||
| * @deprecated | |||
| */ | |||
| @Deprecated | |||
There was a problem hiding this comment.
Why @Deprecated were removed in this PR?
| localtable.getColumn(locCols[j].getIdentifier()))) { | ||
| fkTemp.join(localtable.getColumn(locCols[j].getIdentifier()), | ||
| pkTable.getColumn(pkCols[j].getIdentifier())); | ||
| if( ! fkTemp.containsColumn( |
There was a problem hiding this comment.
if (...) is used in the code
not if( ...)
could you please revert this change?
| @@ -920,72 +905,60 @@ private static Column[] createKeyColumns(ForeignKey fk) { | |||
| return new Column[] { fkCol, pkCol }; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
There was a problem hiding this comment.
same comment to hashcode/equals
|
@solomax Ok as a general remark, indeed I think I took the patch from a local version dating back from openjpa 3.0.0, that would explain strange for loops changes :) |
I reopened OPENJPA-2814 because the proposed fix was causing some exceptions.
My initial fix is fixing properly the memory leak and is in production since few years already so I propose to use this one.