Skip to content

FINERACT-824: NullPointerException is thrown when submitting a loan account application with an existing externalId#683

Merged
vorburger merged 1 commit intoapache:developfrom
angelboxes:FINERACT-824
Jan 14, 2020
Merged

FINERACT-824: NullPointerException is thrown when submitting a loan account application with an existing externalId#683
vorburger merged 1 commit intoapache:developfrom
angelboxes:FINERACT-824

Conversation

@angelboxes
Copy link
Member

@angelboxes angelboxes commented Jan 10, 2020

Removed the second validation since realCause.getCause() is null if it is equal to realCause

Description

When I tried to create a new loan account with an already used external Id, a NullPointerException was thrown with no description of why the loan account creation failed. I removed a validation that returned a null value, this kind of validation was only used with integrity issues on loan accounts and added for some OpenJPA Integration Changes.

https://issues.apache.org/jira/browse/FINERACT-824

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angelboxes I don't know much about this code, but it looks like someone thought that it would be useful to also check the cause... your FINERACT-824 indicates that this can lead to NPEs, so instead of just completely removing the getCause() related check, do you think it may be a good idea to check it like this (for both):

} else if (realCause.getMessage().contains("loan_externalid_UNIQUE") || (realCause.getCause() != null && realCause.getCause().getMessage().contains("loan_externalid_UNIQUE"))) {

Just a thought, as this would perhaps seem more prudent in case that extra check for the cause was there for a good reason. If you are totally confident that this cause is never set and always null, then this is also OK for me, but I thought it would be worth to clarify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, actually that was my first approach to this issue however I saw that this is not the only place where unique values exceptions are thrown but this is the only place where such validation is done. and thought that it was one of those changes that worked when it was added but a different one made it useless and that maybe it wasn't needed,. Don't worry I will change it as you propose.

Removed the second validation since realCause.getCause() is null if it is equal to realCause
Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll merge this one when the build passed.

@vorburger vorburger merged commit efb0a55 into apache:develop Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants