Convert ca-cert-find server command to paged search #4651
Conversation
fmarco76
force-pushed
the
CACertFindCLIFreeVLV
branch
3 times, most recently
from
11db1c9 to
4accfaf
Compare
| * Contain all records in a page for a paged search. | ||
| * | ||
| * @author Marco Fargetta {@literal <[email protected]>} | ||
| */ | ||
| public class CertRecordPage implements Iterable<CertRecord> { |
There was a problem hiding this comment.
IIUC this class seems to be analogous to CertRecordList which represents the entire results of the cert search operation (instead of just a single page implied by this class name), so would it be better to call it CertRecordPagedList instead? Later the original CertRecordList probably could be renamed into CertRecordVirtualList, and we could have a new CertRecordList as the base class for both paged & virtual list classes.
There was a problem hiding this comment.
It is OK to rename the class to make more clear its behaviour. However, the classes CertRecordList and CertRecordPage have no common methods/interfaces so there is not need for the abstraction and since we have to drop the VLV I would not spend time to re-factoring the old code.
| if (revokedBy != null) { | ||
| System.out.println(" Revoked By: " + revokedBy); | ||
| } | ||
| System.out.println(); |
There was a problem hiding this comment.
This code will add an extra blank line at the end of the output. The original code in line 135 will only add blank lines between records, but not at the end of the output. Usually we use a flag like this:
https://github.com/dogtagpki/pki/blob/master/base/tools/src/main/java/com/netscape/cmstools/ca/CACertFindCLI.java#L237-L241
| * | ||
| * @author Marco Fargetta {@literal <[email protected]>} | ||
| */ | ||
| public class DBPagedSearch<E extends IDBObj> { |
There was a problem hiding this comment.
This class seems to be analogous to DBVirtualList which represents the list of results instead of the search operation itself (as implied by the class name). Would it be better to call it something like DBPagedList or DBPagedSearchResults instead?
There was a problem hiding this comment.
Not sure on this change. I was thinking DBVirtualList takes its name from the Virtual List mechanism so I used DBPagedSearch beacuse it uses Paged Seach. Therefore the name should indicate the mechanism used to interact with the DB. Currently, they are too different to merge in a single DBList with two LDAP*List implementation.
The idea of this class (in its implementation) is not to collect the results but in performing the query so I do not like the version with Results suffix.
| * | ||
| * @author Marco Fargetta {@literal <[email protected]>} | ||
| */ | ||
| public class LDAPPagedSearch<E extends IDBObj> extends DBPagedSearch<E> { |
There was a problem hiding this comment.
Similarly, would it be better to call it something like LDAPPagedList or LDAPPagedSearchResults?
| List<CertRecord> newPage = pages.getPage(); | ||
| pageEntries = newPage.iterator(); | ||
| } catch (EBaseException e) { | ||
| logger.error("CertRecordPage: Error to get a new page", e); |
There was a problem hiding this comment.
Let's throw a RuntimeException so that we'll know if there's an error. Please also append the original exception message.
There was a problem hiding this comment.
The equivalent class for VirtualList does not throw exception if no more entries can be accessed, IIUC but return a null value. However, I agee on throwing an exception here, I think it is better in case of problem
| try { | ||
| pageEntries = pages.getPage().iterator(); | ||
| } catch (EBaseException e) { | ||
| logger.error("CertRecordPage: Error to get a new page", e); |
| throw new EBaseException(CMS.getUserMessage("CMS_BASE_CONN_FAILED", | ||
| e.toString())); |
There was a problem hiding this comment.
Let's chain the original exception object so we can get the full stack trace.
There was a problem hiding this comment.
This is the same here
.I am fixing only in this class for this PR.
| } catch (LDAPException e) { | ||
| if (e.getLDAPResultCode() == LDAPException.UNAVAILABLE) | ||
| throw new EDBNotAvailException( | ||
| CMS.getUserMessage("CMS_DBS_INTERNAL_DIR_UNAVAILABLE")); |
fmarco76
force-pushed
the
CACertFindCLIFreeVLV
branch
3 times, most recently
from
4a780a0 to
aa0dfd7
Compare
fmarco76
force-pushed
the
CACertFindCLIFreeVLV
branch
from
aa0dfd7 to
d829749
Compare
|
|
@edewata Thanks! |
No description provided.