Remove not used DB session from certificate queries #4576
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
|
|
||
| e = list.getCertRecords(0, size - 1); | ||
| } | ||
| e = list.getCertRecords(0, size - 1); |
There was a problem hiding this comment.
Hmm.. there are two DBSSession objects here, the one created in findCertRecords() and the one created in findCertRecordsInList(). The CertRecordList is created with the second DBSSession which also provides a connection to LDAP.
IIUC the CertRecordList.getCertRecords() will only work if the LDAP connection is still active, but at this point the second DBSSession is already closed (which is supposed to release the LDAP connection), so I'm suspecting that the code is currently working because we still have the first DBSSession even though we don't use it directly, so if we remove the first DBSSession it might introduce problems in some scenarios that we don't test in CI.
I'm currently working to change the default minConns to 0. Let me test this PR against my changes, if there's no problem it's probably safe to merge.
There was a problem hiding this comment.
It would be very strange if the CertRecordList is valid because of an existing session which is unrelated with the query performed. It could be even in a separate thread. It would require more investigation to understand the connection.
However, if there is an extra tests to verify this it is better.
There was a problem hiding this comment.
I'm not too familiar with how ldapjdk or the connection factory works, but IIUC it involves cloning the LDAPConnection object, so maybe somehow they are still related. Regardless, I'm going to test this anyway. I'll let you know.
There was a problem hiding this comment.
It was a bit difficult to test because some of these methods are actually not used, and the rest can only be tested using the web UI under certain scenarios or a custom profile, but I managed to do some basic testing and it seems to be working just fine, so feel free to merge it. Thanks!
Yes, I am reviewing the DB interaction for the VLV evaluation and I have difficulties to understand what is used and what is heritage. I think we should clean this part if/when we modify the queries from VLV to other approaches. |
|
@edewata Thanks! |
The DB interaction was moved in a separate method which has its own DB session so it was not required in the original methods.