-
Notifications
You must be signed in to change notification settings - Fork 135
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.
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
Code cleanup in CertRecord #4463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments below.
} catch (EBaseException e) { | ||
logger.error("CA failed constructing CRL entry: " + (crlCerts.size() + 1) + " " + e, e); | ||
throw new ECAException(CMS.getUserMessage("CMS_CA_FAILED_CONSTRUCTING_CRL", e.toString())); | ||
if (includeCert == true) { | ||
logger.info("CertRecordProcessor: Adding cert " + certID.toHexString() + " into CRL"); | ||
crlCerts.put(serialNumber, newRevokedCert); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this method is now throwing EBaseException
instead of ECAException
, I'd suggest to change the code that caches ECAException
to catch EBaseException
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, there are some code that are specifically catching ECAException
like this:
https://github.com/dogtagpki/pki/blob/master/base/ca/src/main/java/com/netscape/ca/AuthorityMonitor.java#L248
https://github.com/dogtagpki/pki/blob/master/base/ca/src/main/java/com/netscape/ca/ServiceCert4Crl.java#L92
Since the CertRecordProcessor
is being changed to throw EBaseException
(or any of its subclasses) but not ECAException
specifically, there is a risk that the above catch
clauses will not catch anything anymore. My suggestion is to change the above catch
clauses to catch EBaseException
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the
CertRecordProcessor
is being changed to throwEBaseException
(or any of its subclasses) but notECAException
specifically,
Actually, the method process()
of CertRecordProcessor
has not been modified. It was throwing EBaseException
(https://github.com/dogtagpki/pki/pull/4463/files#diff-41d5d2b8254bd95f284c584826e8f836528342c9aea2dfdf32f7bfe5e4f0fdb4R210) so the calling code has to manage that exception. In the linked example the exception is generated by different methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method signature of CertRecordProcessor.process()
did not change, but the original try-catch
converted EBaseException
into ECAException
. For example, suppose the original code threw an ELdapException
:
public void process() throws EBaseException {
try {
// throw ELdapException
} catch (EBaseException e) {
throw new ECAException();
}
}
The caller would catch the ECAException
and do something based on that exception:
try {
// call process() directly/indirectly
} catch (ECAException e) {
// do something
}
In the new code the try-catch
has been removed, so the process()
will throw the original ELdapException
instead of ECAException
:
public void process() throws EBaseException {
// throw ELdapException
}
If we don't change the caller, it will not catch ELdapException
since it's not a subclass of ECAException
so the behavior will change:
try {
// call process() directly/indirectly
} catch (ECAException e) {
// will not do anything
}
To maintain the original behavior the caller needs to be changed to catch EBaseException
instead:
try {
// call process() directly/indirectly
} catch (EBaseException e) {
// do something
}
If we're sure that the caller does not actually call process()
directly/indirectly we don't have to change it, but I suppose it's quite difficult to determine, so it's probably safer to change it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated CertRecordProcessor.process()
:
public void process() throws ECAException {
// throw ECAException
}
...so now the method throws the same exception that it did before, with the caveat that it now does so for any Exception
rather than EBaseException
, but I think that is fine - sound OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't change the caller, it will not catch
ELdapException
since it's not a subclass ofECAException
The caller has to manage this exception because if it is not managed or thrown from the caller you have a compilation error. However, it could be possible you have different handling for some subclasses and this should be verified.
However, the code is now different so there is no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just realized that CertRecord.getRevocationDate()
is also being changed to not declare EDBException
anymore (because it can't happen), so the try-catch
in CertRecordProcessor.process()
is no longer needed (because there is nothing to catch). This means CertRecordProcessor.process()
actually never throws the ECAException
, or any other EBaseException
, so we can actually remove the exception declaration from CertRecordProcessor.process()
too. It also means that the code that catches ECAException
elsewhere will not be affected by this change.
throw new EBaseException(CMS.getUserMessage("CMS_BASE_INVALID_ATTRIBUTE", name)); | ||
throw new EBaseException(CMS.getUserMessage(CMS_BASE_INVALID_ATTRIBUTE, name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that Sonar complained about the duplicate literals. There are around ~1500 calls to CMS.getUserMessage()
like this, so if we fix it by creating local constants we might end up with a lot of duplicate constants scattered in many places. I think it would be better to consolidate the constants for log messages into a single class, but I'm not sure exactly how that would work since this might be related to localization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had a similar thought, but wasn't sure what to do about it. I'll leave as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment. The previous version is actually good (i.e. without the try-catch
), so feel free to revert the latest changes. You can remove the exception declaration from CertRecordProcessor.process()
too. Sorry for the trouble, thanks!
No worries, thanks for taking the time to look at it! If I had managed to retain the list of changes this probably would have been avoided, sorry about that. |
Kudos, SonarCloud Quality Gate passed! |
I wrote down exactly what I did as usual in the commit message but Eclipse died with an out of memory exception as I committed so that record is now gone, so apologies for that.