Le 24/12/15 01:31, Zheng, Kai a écrit : > Thanks Emmanuel for the review and great comments! The questions are hard but > fortunately I'm still kept in the loop so my pleasure to address them. My > comments are embedded and marked by [Kai]. > > -----Original Message----- > From: Emmanuel Lécharny [mailto:elecha...@gmail.com] > Sent: Wednesday, December 23, 2015 8:54 PM > To: Apache Directory Developers List <d...@directory.apache.org> > Subject: [kerby] ASN1 Quick review > > Hi band, > > I'm having a quick look at the kerby-asn1 code, as I wanted to paly around > the idea of porting the LDAP codec to use this piece of nice code. AFAIU, > when you want to declare an object that can be encoded or decoded, you have > to extend the correct Asn1Object child. Looking at the Ticket object, here is > what I see : > > [Kai] Glad you want a try. This is also something I wish to help with as > discussed before, but am not able to do it immediately because of bandwidth. > Sorry for that. I certainly would and will also be able to try to provide any > help if needed. > > ... > import static org.apache.kerby.kerberos.kerb.type.ticket.Ticket.MyEnum.*; > ... > public class Ticket extends KrbAppSequenceType { > public static final int TKT_KVNO = KrbConstant.KRB_V5; > public static final int TAG = 1; > > protected enum MyEnum implements EnumType { > TKT_VNO, > REALM, > SNAME, > ENC_PART; > > @Override > public int getValue() { > return ordinal(); > } > > @Override > public String getName() { > return name(); > } > } > > static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] { > new ExplicitField(TKT_VNO, 0, Asn1Integer.class), > new ExplicitField(REALM, 1, KerberosString.class), > new ExplicitField(SNAME, 2, PrincipalName.class), > new ExplicitField(ENC_PART, 3, EncryptedData.class) > }; > > I like the idea of defining the fields this way, except that I would suggest > some slight modifications : > > - get read of the import ...Ticket.MyEnum.*; > [Kai] Hmmm, I guess you mean "get rid of", good point. It's like this > because, initially we didn't use enum, but int constants. And some weeks ago > when we want to dump user defined type objects like this with meaningful > field info, we switched to use enum because it can give a friendly name. We > were in a hurry at that time and wanted to do it as less effort as possible, > thus it's like this now: the enum constant is rather like int constant and > used as before.
Okie, so I guess you don't have any provision against the suggested modification. > > - make the enum private (there is no reason we wuld like to expose it anyway) > [Kai] Well, actually there are some reasons to make it protected and > disciplined exposed. Such user defined types can be extended and these fields > may be accessed there. Ref. child classes of ContentInfo (also some other > examples I remembered when defining protected int constants). Okie for 'protected'. > > - name it something more meaningful, like TicketField inseatd if MyEnum > [Kai] Right agree. Again it was like this because it's out as a simple > pattern in a whole project scope replacement. Okie. A rename is then possible. > > - use it with the enum name, like in new ExplicitField(TicketField.TKT_VNO, > 0, Asn1Integer.class) > [Kai] Agree. > > <side note> > I find the "import static xxx.*;" atrocious, most of the time. It makes it > barely readable : you have to go to the top of your file to actually > *know* where the constant comes from... That may seems of for small files, > but otherwise... I accept it for Asserts, because it's really clear, in a > test context - pretty much like annotations. > </side note> > [Kai] I thought you may be right, but ... > > Here is what I come with : > > public class Ticket extends KrbAppSequenceType { > public static final int TKT_KVNO = KrbConstant.KRB_V5; > public static final int TAG = 1; > > private enum TicketField implements EnumType { > TKT_VNO, > REALM, > SNAME, > ENC_PART; > > @Override > public int getValue() { > return ordinal(); > } > > @Override > public String getName() { > return name(); > } > } > > static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] { > new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class), > new ExplicitField(TicketField.REALM, 1, KerberosString.class), > new ExplicitField(TicketField.SNAME, 2, PrincipalName.class), > new ExplicitField(TicketField.ENC_PART, 3, EncryptedData.class) > }; > > (note that it's just a suggestion at this point...) > [Kai] They're very good suggestions and should become true, though a tiresome > work because there are so many user defined types now. I'm wondering if any > contributor would help with such. I can do that. This is typically the kind of (boring) task that I like to do when I have little brain cycles (like when it's late, or when I don't have energy to focus on serious code : doing boring things make me feel I'm still working and contributing to the community). > > Now, let's foduc on the fields declarations. Here, we create ExplicitFields, > passing a Xxx.class as the third parameter, and a number as the second > parameter. > > First of all, as the enum being used has an implicit ordering, can't we > simply avoid passing those numbers ? There is already an ExplicitField > constructor that takes only 2 parameters, deducing the number from teh enum... > [Kai] Yeah sure we can just use the 2 parameters constructor for this type. > You're good at finding this as the example. We relied on the enum order value > mostly but this one and possible many slipped out. Okie. Another boring task ;-) > > Second, why don't we write things like : > > > new ExplicitField(TicketField.TKT_VNO, 0, new Asn1Integer()), > > instead of passing a class ? Bottom line, the instance will be created the > same way. I don't think it will make any difference in term of performances, > and as all the object *must* extends the Asn1Object (or implement the > Asn1Type), this should be good enough. > [Kai] Well, let the framework determine when to create the field instances > would be most flexible. We may want to be lazy on creating them and create > them on demand; we may need to avoid creating them in decoding because > they're nulls; in the framework codes there're some places that uses (value > == null) to check some conditions. For Any, the actual value type can only be > set by applications in specific contexts. Makes perfect sense. Side note : I wonder if calling newInstance(class) is really slower that doing new Class at this point... It's not anymore the case, I think (it was before Java 6)