Re: [Pki-devel] CLI for editing profiles
On 8/28/2014 12:30 AM, Fraser Tweedale wrote:
> Do any of these InputStreams and OutputStreams require close()?
> I think we need to think about refactoring the DBManager/ LDAPConnection
> code so that the LDAPConnection implements Closeable. Then we can
> manage all of these resources using try-with-resources constructions,
> and not have to worry about finally and nested finally blocks.
>
> This should probably be a separate patch, but it would be nice to do
> that refactoring first, so that we could use the new cleaner code
> structure here.
LDAPConnection is provided by ldapjdk so I don't think we can modify
that class.
We could define a "wrapper" class that implements the desired
behaviour. It could contain an LDAPConnection and the factory and
in its close() method return the connection to the factory. The
code that uses it could then just get the LDAPConnection out of the
wrapper, resulting in few changes, and ILdapConnFactory (and its
implementations) would learn a new method that returns one of these
"wrapped" connections instead of a "bare" LDAPConnection.
If this sounds like a reasonable approach, I will prototype this
design.
I think to be a proper wrapper it should wrap the LDAP operations too
(e.g. search(), add()) so we don't have to unwrap the original
LDAPConnection. I'm not sure if we should implement this in Dogtag
though. This change really belongs in the LDAPJDK itself.
> Is there any synchronization required for the commit() for
> LDAPConfigStore()?
>
The PropConfigStore load() and save() methods are synchronised. I
hope that my expectation that the LDAP server will serialize
concurrent modifications to an entry is reasonably held. Let me
know if I am wrong about that, and suggested approach(es) to
addressing it.
The code is probably fine for now. Later we probably want to improve it.
Given that the certProfileConfig attribute contains the entire profile
properties, the chance that people overwrite each other's modification
can be higher. A better solution is to separate the properties into
separate attributes and only update the attributes that have changed.
We can also use REST's ETag to make sure we're not making modifications
based on outdated data. This will only work if all changes are coming
through REST though.
Some more comments:
17. In LDAPConfigStore.save() the writer.close() might not be called if
there's an exception. I don't know how likely this may happen, but to be
safe we should put close() in a finally block, or use try-with-resources.
18. In LDAPConfigStore.commit() the nested try-catch block might not be
necessary. If the LDAPException is something other than NO_SUCH_OBJECT
it could throw an EBaseException at that point.
19. In ProfileSubsystem.init() while loading the profiles if one of the
class ID's is invalid (e.g. due to typo) the loop will break and the
subsystem initialization will fail. I'm not sure about the implication
if that happens or how likely, but we probably should just disable the
failed profiles and continue loading the rest. People will find the
error when they try to use the profile but it's not available.
20. Similar issue with CAInstallerService.importProfiles(), what happens
if one profile fails to import? This is probably less likely to happen
since it's loading the default profiles.
21. The ProfileSubsystem.profileDN() probably should be called
getProfileDN() or createProfileDN() to follow Java conventions.
22. In new code like CMSEngine.getDynSubsystemNames() we should use the
light-weight ArrayList or LinkedList instead of Vector.
23. Since WarningListener.mEnabled is new, it shouldn't use the
Hungarian notation.
--
Endi S. Dewata