Skip to content

HDDS-5396. Fix negligence issue conditional expressions in MockCRLStore.java #2380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jojochuang merged 2 commits into from
Jun 30, 2021
Merged

HDDS-5396. Fix negligence issue conditional expressions in MockCRLStore.java #2380

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60fb40b
HDDS-5396. Fix potential IndexOutOfBoundsException in MockCRLStore.java
sky76093016 Jun 30, 2021
3350565
Modify if condition
sky76093016 Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/update/server/MockCRLStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public Optional<Long> revokeCert(List<BigInteger> certs,
List<CRLInfo> crlInfos =
scmCertStore.getCrls(ImmutableList.of(crlId.get()));

if (crlInfos.isEmpty()) {
if (crlInfos == null || crlInfos.isEmpty()) {

@ayushtkn ayushtkn Jun 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When crlInfos can be null?
The value is fetched from. scmCertStore.getCrls(ImmutableList.of(crlId.get())); and this doesn't look like returning a null in any case. It is in general returning an empty list, if it can not find anything?

Secondly, If I understand the code correct, now if crlInfos is null, we will log the line below, so, will this crlInfos.get(0) not throw a NPE since crlInfos is null?

Thirdly, How this null check, preventing an IndexOutOfBoundsException

@sky76093016 sky76093016 Jun 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When crlInfos is empty, crlInfos.get(0) will have something feels unreasonable. After I communicated with @xiaoyuyao , it was because of the original negligence, it should be if (!crlInfos.isEmpty()).

log.debug("CRL[0]: {}", crlInfos.get(0));
}
return crlId;
Expand Down