Ca list certs free vlv #4653
Ca list certs free vlv #4653
Conversation
Replacing the VLV search with the paged search for WEB UI search certificate page. The new search is not sorted by serial number but since the new pki version is moving to random serial number this sorting is not useful in future release.
| list.getCurrentIndex(); | ||
| logger.debug("ListCerts: totalRecordCount: " + totalRecordCount); | ||
| } | ||
| totalRecordCount = mCertDB.countCertificates(filter, -1); |
There was a problem hiding this comment.
Let's suppose there's a search filter that will match 2000 entries on the DS, but the nsslapd-sizelimit param is set to 1000.
IIUC with VLV the list size would be 2000 (since it's virtual so it can include the entire results), so the totalRecordCount would be 2000 as well. With paged results will the totalRecordCount be 1000 since it counts the actual entries returned by the DS which are limited to 1000?
There was a problem hiding this comment.
I have not done test but IIUC with paged result the limit is applied to the single page. The count is getting multiple pages so at the end the number will be 2000.
| case "end": | ||
| start = Math.max(0, totalRecordCount - maxCount); | ||
| break; |
There was a problem hiding this comment.
If the totalRecordCount is difficult to obtain accurately with paged results, we might want to remove functionalities that depend on it, e.g. navigation to the end of the list. This will affect the UI.
|
It looks like IPA doesn't actually use We will still need to remove VLV eventually, but updating |
Yes, I started because I was doing all cert related VLVs and this appeared as an easy fix (I was wrong). However, I have completed this for now. It works in both the No other services are connected with this PR so it is not a priority to review/merge. |
| mAllowedClientFilters.addElement("(\\\\(\\\\&)?(\\\\(\\\\|)?(\\\\(certStatus=(\\\\*|VALID|INVALID|EXPIRED)\\\\))*(\\\\))?(\\\\(certRecordId(<|>)=(0x)?\\\\d+\\\\))*(\\\\))?"); | ||
| mUseClientFilterRegexp = true; |
There was a problem hiding this comment.
I'm a bit concerned about using regex here since it's not easy to read and might be difficult to change in case we need to add/remove the filters. If we're going to keep the regex I think we need to include some explanation what filters will match the regex.
There was a problem hiding this comment.
OK, I'll add the comment. Since the filter now include the cert serial the previous approach becomes awkward.
There was a problem hiding this comment.
Could you also include the regex in the comment without the escape characters? That probably will make it easier to see how those filters can be captured by the regex.
|
I only have one concern about the regex, but once that is addressed I think we can merge this PR (assuming it's been sufficiently tested) since eventually we will need to drop the dependency on VLV anyway. About the performance issue I think we can address it later. That might require changes to the API and the UI/CLI too. |
fmarco76
force-pushed
the
CAListCertsFreeVLV
branch
from
aab5354 to
9306741
Compare
fmarco76
force-pushed
the
CAListCertsFreeVLV
branch
from
9306741 to
94e0a4b
Compare
|
|
I have update the comment and dome some additional tests. The interface seems to work correctly and I am merging this PR. However, this part of UI should be updated ASAP to use the REST APIs avoiding code duplication. @edewata Thanks! |
List and search certs will move from VLV to paged search.
The current implementation is not efficient because of some peculiarities of VLV which cannot be easily replicated iwith paged search, such as the entries count.