jsdsi-devel

[Jsdsi-devel] CertStore Branch

[Sean R. <sra...@ae...>]

No sooner have I merged one branch, I've created another...

Guys,

I've created a branch in CVS called 'branch-certstore'.

This is another experimental design I'd like you all to have a look at 
refactoring the architecture of CertStore to easily allow alternate 
stores and standard interface for also storing Certificates. Fancy 
taking a look and getting back to me? (jsdsi.certstore package). The new 
implementation is for JDBC.

Regards,

Sean

-- 
Dr. Sean Radford, MBBS, MSc
sra...@ae...
http://www.aegeus-technology.com/ 

Re: [Jsdsi-devel] CertStore Branch

[Sean R. <sra...@ae...>]

Sean Radford wrote:

> No sooner have I merged one branch, I've created another...
>
> Guys,
>
> I've created a branch in CVS called 'branch-certstore'.
>
> This is another experimental design I'd like you all to have a look at 
> refactoring the architecture of CertStore to easily allow alternate 
> stores and standard interface for also storing Certificates. Fancy 
> taking a look and getting back to me? (jsdsi.certstore package). The 
> new implementation is for JDBC.
>
> Regards,
>
> Sean
>
Don't suppose any of you have had a look?

-- 
Dr. Sean Radford, MBBS, MSc
sra...@ae...
http://www.aegeus-technology.com/ 

Re: [Jsdsi-devel] CertStore Branch

[Sameer A. <aj...@gm...>]

These changes look good.  A few notes:
- Provide some package-level documentation -- what's a DAO?  (Data
Access Object)  What are the important interfaces of this package? 
(CertificateDAO)  What implementations are provided? (Jdbc)
- You should extract the Multimap-based implementation of
jsdsi.CertStore into a DAO, "LocalCertificateDAO", and make
jsdsi.CertStore a subclass of certstore.JsdsiCertStore that uses
LocalCertificateDAO. (the following bullets clarify how to go about
this)
- AbstractJsdsiCertStore should implement
init(CollectionCertStoreParameters params) by calling
this.addCertificate(c) for eact Certificate c in
params.getCollection().iterator().  addCertificate is a new abstract
method of AbstractJsdsiCertStore.
- JsdsiCertStore implements addCertificate(c) by calling dao.store(c).
- LocalCertifificateDAO implements store(c) by populating its
MultiMaps as shown in jsdsi.CertStore.init().
- AbstractJsdsiCertStore.getCertificates: change "( X instanceof Y) ==
false" to "!(X instanceof Y)"
- There's a misspelled file: jsdsi.JsdiRuntimeException (there's also
the correctly-spelled version)
- I personally don't like RuntimeExceptions -- I'd rather the compiler
tell me that there's an exception on the way, even if all I can do is
propagate it.
- I thnk JsdsiCertStoreException ought to be a checked (non-runtime) exception.
- JdbcCertificateDAO.store() is hard to follow.  Instead, define these methods:
byte[] getIssuer(Certificate)
byte[] getSubject(Certificate)
byte[] getCompatible(Certificate)
byte[] getLocalName(Certificate)
byte[] getName(Certificate)
- and populate the "params" array with calls to these methods
In JdbcCertificateDAO.retrieve(), extract the long if-else chain into
a method that does:
return retrieve.(...);
- in each if-branch.  This makes the code easier to read, since it's
clear "result" is only assigned to once in retrieve().

Overall, looks cool.  Does the prover work correctly with the
JdbcCertStore? (it should!)  Is JdbcCertStore a distributed store?

Thanks for the contribution.  Let me know if you want me to look at
revisions, and assuming all is well, we can merge it in to the
mainline.

Sameer

On Tue, 23 Nov 2004 14:32:08 +0000, Sean Radford
<sra...@ae...> wrote:
> Sean Radford wrote:
> 
> 
> 
> > No sooner have I merged one branch, I've created another...
> >
> > Guys,
> >
> > I've created a branch in CVS called 'branch-certstore'.
> >
> > This is another experimental design I'd like you all to have a look at
> > refactoring the architecture of CertStore to easily allow alternate
> > stores and standard interface for also storing Certificates. Fancy
> > taking a look and getting back to me? (jsdsi.certstore package). The
> > new implementation is for JDBC.
> >
> > Regards,
> >
> > Sean
> >
> Don't suppose any of you have had a look?
> 
> --
> Dr. Sean Radford, MBBS, MSc
> sra...@ae...
> http://www.aegeus-technology.com/
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://productguide.itmanagersjournal.com/
> 
> 
> _______________________________________________
> Jsdsi-devel mailing list
> Jsd...@li...
> https://lists.sourceforge.net/lists/listinfo/jsdsi-devel
> 
> 


-- 
Sameer Ajmani
http://ajmani.net

Re: [Jsdsi-devel] CertStore Branch

[Sean R. <sra...@ae...>]

Sameer Ajmani wrote:

>These changes look good.  A few notes:
>- Provide some package-level documentation -- what's a DAO?  (Data
>Access Object)  What are the important interfaces of this package? 
>(CertificateDAO)  What implementations are provided? (Jdbc)
>- You should extract the Multimap-based implementation of
>jsdsi.CertStore into a DAO, "LocalCertificateDAO", and make
>jsdsi.CertStore a subclass of certstore.JsdsiCertStore that uses
>LocalCertificateDAO. (the following bullets clarify how to go about
>this)
>- AbstractJsdsiCertStore should implement
>init(CollectionCertStoreParameters params) by calling
>this.addCertificate(c) for eact Certificate c in
>params.getCollection().iterator().  addCertificate is a new abstract
>method of AbstractJsdsiCertStore.
>- JsdsiCertStore implements addCertificate(c) by calling dao.store(c).
>- LocalCertifificateDAO implements store(c) by populating its
>MultiMaps as shown in jsdsi.CertStore.init().
>- AbstractJsdsiCertStore.getCertificates: change "( X instanceof Y) ==
>false" to "!(X instanceof Y)"
>- There's a misspelled file: jsdsi.JsdiRuntimeException (there's also
>the correctly-spelled version)
>- I personally don't like RuntimeExceptions -- I'd rather the compiler
>tell me that there's an exception on the way, even if all I can do is
>propagate it.
>- I thnk JsdsiCertStoreException ought to be a checked (non-runtime) exception.
>- JdbcCertificateDAO.store() is hard to follow.  Instead, define these methods:
>byte[] getIssuer(Certificate)
>byte[] getSubject(Certificate)
>byte[] getCompatible(Certificate)
>byte[] getLocalName(Certificate)
>byte[] getName(Certificate)
>- and populate the "params" array with calls to these methods
>In JdbcCertificateDAO.retrieve(), extract the long if-else chain into
>a method that does:
>return retrieve.(...);
>- in each if-branch.  This makes the code easier to read, since it's
>clear "result" is only assigned to once in retrieve().
>
>Overall, looks cool.  Does the prover work correctly with the
>JdbcCertStore? (it should!)  Is JdbcCertStore a distributed store?
>
>Thanks for the contribution.  Let me know if you want me to look at
>revisions, and assuming all is well, we can merge it in to the
>mainline.
>
>Sameer
>
>On Tue, 23 Nov 2004 14:32:08 +0000, Sean Radford
><sra...@ae...> wrote:
>  
>
Sameer,

Cheers for your input. Hopefully, I'll be able to work on it this w/e.

Sean

-- 
Dr. Sean Radford, MBBS, MSc
sra...@ae...
http://www.aegeus-technology.com/ 

Re: [Jsdsi-devel] CertStore Branch

[Sean R. <sra...@ae...>]

Guys,

Done more work on the CertStore branch:

1. jsdsi.certstore.InMemoryCertificateDAO A MultiMap implemenation of 
jsdsi.certstore.CertificateDAO.
2. Fully implemented (with unit tests), a JDBC implementation of 
CertificatedDAO (jsdsi.certstore.jdbc.JdbcCertificateDAO)
3. A extension to CertificateDAO that I presume is needed for an 
LDAPCertificateDAO implementation in jsdsi.certstore.ldap package.
4. Moved jsdsi.MultiMap to jsdsi.util package and made it 'public'.
5. Various code tidy-up and documentation.

I haven't changed JsdsiCertStoreException to be checked... I thought we 
had agreed back April time to use RuntimeExceptions. I've gone more and 
more off Checked Exceptions. Have a read here for other (well known 
peoples' views):

http://www.artima.com/intv/handcuffs.html

http://www.mindview.net/Etc/Discussions/CheckedExceptions


Sameer,

>- There's a misspelled file: jsdsi.JsdiRuntimeException (there's also
>the correctly-spelled version)
>  
>
There sure is, but it is deprecated for the very reason that it is spelt 
wrong.

>In JdbcCertificateDAO.retrieve(), extract the long if-else chain into
>a method that does:
>return retrieve.(...);
>- in each if-branch.  This makes the code easier to read, since it's
>clear "result" is only assigned to once in retrieve().
>  
>
Sorry, but not sure I understand what you mean by this. If you mean to 
not assign to 'result' but just return, then you loose the logging 
information.


Let me know what you think. Regards,

Sean

-- 
Dr. Sean Radford, MBBS, MSc
sra...@ae...
http://www.aegeus-technology.com/ 

Re: [Jsdsi-devel] CertStore Branch

[Sean R. <sra...@ae...>]

Sameer Ajmani wrote:

>>I haven't changed JsdsiCertStoreException to be checked... I thought we
>>had agreed back April time to use RuntimeExceptions. I've gone more and
>>more off Checked Exceptions. Have a read here for other (well known
>>peoples' views):
>>
>>http://www.artima.com/intv/handcuffs.html
>>http://www.mindview.net/Etc/Discussions/CheckedExceptions
>>    
>>
>
>These are interesting arguments, but I don't actually think JSDSI has
>the scalability and versioning problems described herein.  This is
>because by having a top-level JsdsiException and other exceptions that
>derive from this, clients can just catch that exception as they would
>a runtime exception (they could even wrap it in a runtime exception if
>they really prefer that).  So even if we have JsdsiProverException,
>JsdsiSignatureException, JsdsiCertStoreException, etc, the caller can
>just catch an dprocess JsdsiException, or pass it along, or re-throw
>something else.  But making it checked guaranteed the compiler will
>give clients of JSDSI a heads-up that there's an exception that may be
>thrown.
>
>btw, I'm a big fan of exception chaining, so I definitely think any
>Java Crypto Exceptions should be wrapped in a JsdsiException subclass;
>users who care about the distinction between NoSuchProviderException
>and NoSuchAlgorithmException can follow the causal chain to find this
>information.
>  
>
Ok, I'm for that.

Would you like me to (in the certstore branch):

1. Ditch JsdsiRuntimeException
2. Create a JsdsiException (a checked exception)
3. Refactor other code to throw a JsdsiException (or a subclass).

I would also vote to clean-up the cerstore branch, in terms of removing 
a number of the deprecated methods, then merge into HEAD and make a 
shiny new release to sf.net. Thoughts?

>  
>
>>>In JdbcCertificateDAO.retrieve(), extract the long if-else chain into
>>>a method that does:
>>>return retrieve.(...);
>>>- in each if-branch.  This makes the code easier to read, since it's
>>>clear "result" is only assigned to once in retrieve().
>>>
>>>      
>>>
>>Sorry, but not sure I understand what you mean by this. If you mean to
>>not assign to 'result' but just return, then you loose the logging
>>information.
>>    
>>
>
>Actually, I just meant to move the if-then-else to a method, so as to
>guarantee result is never accidentally left as null.  Again, this is a
>way to let the compiler help you: if the if-then-else is in a method,
>and each branch returns the result, then the compiler will complain if
>not all cases are covered.  You would still have your roriginal method
>assign the returned value to a "result" variable, log it, and return
>it.
>
>  
>
Understand. Will do.


-- 
Dr. Sean Radford, MBBS, MSc
sra...@ae...
http://www.aegeus-technology.com/ 

Re: [Jsdsi-devel] CertStore Branch

[Sameer A. <aj...@gm...>]

> Ok, I'm for that.
> 
> Would you like me to (in the certstore branch):
> 
> 1. Ditch JsdsiRuntimeException
> 2. Create a JsdsiException (a checked exception)
> 3. Refactor other code to throw a JsdsiException (or a subclass).
> 
> I would also vote to clean-up the cerstore branch, in terms of removing
> a number of the deprecated methods, then merge into HEAD and make a
> shiny new release to sf.net. Thoughts?

Yes, this sounds great!

Thanks a lot,
-- 
Sameer Ajmani
http://ajmani.net