You are viewing a plain text version of this content. The canonical link for it is here.
Posted to api@directory.apache.org by Emmanuel Lecharny <el...@gmail.com> on 2010/01/12 14:29:44 UTC
[DN] Existing API review
Hi,
yesturday, I conducted a review on the existing APIs for the DN class :
- Name/LdapName (JNDI) (I will call it JNDI)
- jLdap/LdapSdk (I will call it LSD)
- ApacheDS (ADS)
- OpenDS (ODS)
- UnbounID (UID)
There are some important differences. There are two sets of API,
depending on the existence of a constructor, or not.
- API having a constructor :
JNDI,
ADS,
UID
- API not having a constructor :
LSD,
ODS (ODS uses a static method valueof() to produce a DN)
IMHO, I really think that users prefer having a constructor over none or
a static factory. There are three rasons for that :
1) Most of LDAP users are using the JNDI API, which has a LdapNAME()
constructor
2) In Java, it's pretty natural to create objects through constructor.
3) I'm not sure that handling a cache for DN is a valuable trick, as it
leads to some contention and the need to manage this cache in a
muli-threaded environement (this has to be carefully evaluated)
I think that we should have some basic constructors, and if the cache is
proven to be valuable, then we can extend the API by adding the valueof(
... ) method.
The base constructor we can have are probably something like:
DN()
DN(String dnStr)
DN( RDN... rdns)
DN( RDN rdn, DN parent)
Thoughts ?
--
Regards,
Cordialement,
Emmanuel Lécharny
www.nextury.com
Re: [DN] Existing API review
Posted by Emmanuel Lecharny <el...@gmail.com>.
Matthew Swift a écrit :
>
>>
>> public DN(String dnStr) {
>> return valueOf( dnStr );
>> }
>
> Nothing will prevent you from writing something like that, but the
> compiler will prevent you from compiling it. Get some sleep! ;-)
Doh !!! Yeah, or maybe some stronger coffee ;)
--
Regards,
Cordialement,
Emmanuel Lécharny
www.nextury.com
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
On 14/01/10 23:20, Emmanuel Lcharny wrote:
> [...]
> But for users, DN(String) covers 99% of their usage. This is how they
> created DN with JNDI, and i'm not sure they want something very
> different. Now, internally, othing prevent you to write something like :
>
> public DN(String dnStr) {
> return valueOf( dnStr );
> }
Nothing will prevent you from writing something like that, but the
compiler will prevent you from compiling it. Get some sleep! ;-)
Matt
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Matthew Swift a écrit :
>
>
> On 13/01/10 21:55, Stefan Seelmann wrote:
>> Matthew Swift wrote:
>>> On 12/01/10 14:29, Emmanuel Lecharny wrote:
>>>
>>> Even if you decide that caching is not required then that's no
>>> reason to develop an API which prevents you from implementing one in
>>> future. Using a normal constructor prevents the use of a cache (or
>>> forces you to use the pimpl idiom). If you extend the API later by
>>> adding a valueOf method then no existing applications will be able
>>> to take advantage of the perf improvement unless they are modified
>>> to use the new static factory method.
>>
>> Matthew, you mentioned good reasons for factory methdods. I think it
>> is not a big deal to add a one or two factory methods.
>
>
> rootDN/nullDN/emptyDN is an obvious candidate
Yeah. I think that nullDN is a nonsense. rootDN is probably technically
the correct vision, but I'm not sure that users will understand the
implication of such a name. I would rather pick emptyDN just because
it's semantically not tainted (ie, rootDN <=> roodDSE DN), and an empty
DN can be the base for operations like move, applied in the middle of
the tree, not at the root (assumong you can add RDN to a DN).
>
> valueOf(String) is the other and perhaps valueOf(ByteString/byte[])
We had a long discussion with Ludovic while in Portland. We have made a
big mistake one year and a half ago trying to introduce something like a
Value<?> object (where ? = String or byte[]). I think it's a nonsense
too. valueOf(String) and valueOf(byte[]) clearly have my preference over
any other form. No need to use a ByteString, it huld be purely internal.
>
> I don't like having two ways to achieve the same goal, hence my
> dislike of the DN(String) constructor, especially when users of the
> method will not automatically inherit performance improvements in
> future releases.
But for users, DN(String) covers 99% of their usage. This is how they
created DN with JNDI, and i'm not sure they want something very
different. Now, internally, othing prevent you to write something like :
public DN(String dnStr) {
return valueOf( dnStr );
}
>
>>>
>>> Also, on the subject of AVAs - we have the AVA type as an inner
>>> class in RDN. I'm not particularly happy with this this, but less
>>> happy with it being a standalone class since AVAs are only used in
>>> RDNs and may introduce confusion elsewhere. For example, filters
>>> also use attribute value assertions but these are not the same type
>>> of object as an AVA even if they have the same name. For example,
>>> AVAs (in RDNs) do not allow attribute options or matching rules to
>>> be specified.
>>>
>>> What do you think? Inner class or standalone?
>>
>> It could be an inner class, but should be visible and constructable
>> from outside. The use case I see is to create an DN or RDN by
>> specifying the attribute types and values, without having to deal
>> with escaped characters. We need such functionality in Directory
>> Studio, when defining the RDN of an entry or when renaming an entry.
>> The user for example just types "a+b" into the value field, we
>> construct a new AVA("cn", "a+b") and the AVA implementation should
>> handle the escaping to "cn=a\+b".
>>
>>
>
> There's no need for an AVA class at all from a construction point of
> view. For example, you could have an RDN method RDN.addAVA(type,
> value). The stronger requirement for an AVA class comes when you need
> to access the AVAs in an RDN since you want to get two values at once
> - the attribute type and attribute value. It is possible to avoid
> having an AVA class altogether by having methods like:
>
> int getAVACount()
> AttributeType getAVAAttributeType(int index)
> AttributeValue getAVAAttributeValue(int index)
> void addAVA(AttributeType, AttributeValue)
>
> One less class is a good thing IMO especially when it's potentially a
> source of confusion (with filters), but I'm not a big fan of the
> indexing and the inability to easily iterate over the AVAs. I'm pretty
> undecided on this one.
I also have in mind serialization and deserialization : having the AVA
class halps a bt to track down the way it's done.
Now, when you face DN like cn=ACME+gn=whatever, manipulating AVAs is
convenient, as you can iterate though them instead of using indices.
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
On 13/01/10 21:55, Stefan Seelmann wrote:
> Matthew Swift wrote:
>> On 12/01/10 14:29, Emmanuel Lecharny wrote:
>>
>> Even if you decide that caching is not required then that's no reason
>> to develop an API which prevents you from implementing one in future.
>> Using a normal constructor prevents the use of a cache (or forces you
>> to use the pimpl idiom). If you extend the API later by adding a
>> valueOf method then no existing applications will be able to take
>> advantage of the perf improvement unless they are modified to use the
>> new static factory method.
>
> Matthew, you mentioned good reasons for factory methdods. I think it
> is not a big deal to add a one or two factory methods.
rootDN/nullDN/emptyDN is an obvious candidate
valueOf(String) is the other and perhaps valueOf(ByteString/byte[])
I don't like having two ways to achieve the same goal, hence my dislike
of the DN(String) constructor, especially when users of the method will
not automatically inherit performance improvements in future releases.
>
>>> The base constructor we can have are probably something like:
>>> DN()
>>> DN(String dnStr)
>>> DN( RDN... rdns)
>>> DN( RDN rdn, DN parent)
>
> +1, and additional
> DN(DN localName, DN parent)
Agreed.
>
>
> And let's add simple methods to get the parent of an DN. Every time I
> use JNDI I have to write a test to see the result of
> ldapName.getPrefix( ldapName.size() - 1 )
>
>> Also, I strongly believe that DNs and RDNs and AVAs should be
>> immutable objects (as well as any other low level API type). What do
>> you think?
>
> Yes, I agree.
>
>>
>> Also, on the subject of AVAs - we have the AVA type as an inner class
>> in RDN. I'm not particularly happy with this this, but less happy
>> with it being a standalone class since AVAs are only used in RDNs and
>> may introduce confusion elsewhere. For example, filters also use
>> attribute value assertions but these are not the same type of object
>> as an AVA even if they have the same name. For example, AVAs (in
>> RDNs) do not allow attribute options or matching rules to be specified.
>>
>> What do you think? Inner class or standalone?
>
> It could be an inner class, but should be visible and constructable
> from outside. The use case I see is to create an DN or RDN by
> specifying the attribute types and values, without having to deal with
> escaped characters. We need such functionality in Directory Studio,
> when defining the RDN of an entry or when renaming an entry. The user
> for example just types "a+b" into the value field, we construct a new
> AVA("cn", "a+b") and the AVA implementation should handle the escaping
> to "cn=a\+b".
>
>
There's no need for an AVA class at all from a construction point of
view. For example, you could have an RDN method RDN.addAVA(type, value).
The stronger requirement for an AVA class comes when you need to access
the AVAs in an RDN since you want to get two values at once - the
attribute type and attribute value. It is possible to avoid having an
AVA class altogether by having methods like:
int getAVACount()
AttributeType getAVAAttributeType(int index)
AttributeValue getAVAAttributeValue(int index)
void addAVA(AttributeType, AttributeValue)
One less class is a good thing IMO especially when it's potentially a
source of confusion (with filters), but I'm not a big fan of the
indexing and the inability to easily iterate over the AVAs. I'm pretty
undecided on this one.
Matt
Re: [DN] Existing API review
Posted by Stefan Seelmann <se...@apache.org>.
Matthew Swift wrote:
> On 12/01/10 14:29, Emmanuel Lecharny wrote:
>
> Even if you decide that caching is not required then that's no reason to
> develop an API which prevents you from implementing one in future. Using
> a normal constructor prevents the use of a cache (or forces you to use
> the pimpl idiom). If you extend the API later by adding a valueOf method
> then no existing applications will be able to take advantage of the perf
> improvement unless they are modified to use the new static factory method.
Matthew, you mentioned good reasons for factory methdods. I think it is
not a big deal to add a one or two factory methods.
>> The base constructor we can have are probably something like:
>> DN()
>> DN(String dnStr)
>> DN( RDN... rdns)
>> DN( RDN rdn, DN parent)
+1, and additional
DN(DN localName, DN parent)
And let's add simple methods to get the parent of an DN. Every time I
use JNDI I have to write a test to see the result of
ldapName.getPrefix( ldapName.size() - 1 )
> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
> objects (as well as any other low level API type). What do you think?
Yes, I agree.
>
> Also, on the subject of AVAs - we have the AVA type as an inner class in
> RDN. I'm not particularly happy with this this, but less happy with it
> being a standalone class since AVAs are only used in RDNs and may
> introduce confusion elsewhere. For example, filters also use attribute
> value assertions but these are not the same type of object as an AVA
> even if they have the same name. For example, AVAs (in RDNs) do not
> allow attribute options or matching rules to be specified.
>
> What do you think? Inner class or standalone?
It could be an inner class, but should be visible and constructable from
outside. The use case I see is to create an DN or RDN by specifying the
attribute types and values, without having to deal with escaped
characters. We need such functionality in Directory Studio, when
defining the RDN of an entry or when renaming an entry. The user for
example just types "a+b" into the value field, we construct a new
AVA("cn", "a+b") and the AVA implementation should handle the escaping
to "cn=a\+b".
Kind Regards,
Stefan
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Matthew Swift a écrit :
>
>
> On 14/01/10 00:43, Emmanuel Lcharny wrote:
>> [...]
>> Well, I don't really think that it's anything but implementation
>> dependent, so from the API POV, it's irrelevant. As soon as we add
>> the valueof() methods, those who want to add a cache can do it.
>>
>
> It's very relevant from an API POV.
Sorry, you don't caught what I wanted to express, or I wasn't able to
express what I had in mind.
I meant that discussing this point is irrelevant, just because having
both the constructor *and* the valueof makes sense to me now.
> Being future proof is an essential part of any API. Client
> applications will need to be modified if a valueOf constructor is
> added at a later date in order to take advantage of any potential perf
> improvements. By including the valueOf initially applications can
> choose to use the constructor knowing that they will inherit any
> future improvements such as caching.
On the same page.
>
> If for learnability reasons we decide that a DN(String) constructor is
> required then so be it, but it should include Javadoc recommending
> that users use the valueOf constructor in preference.
Well, here, I disagree. Users are smat enough to understand the
difference between both methods to create objects, and i don't thik we
have to stress it out, otherwise people will keep asking 'why are you
keeping those two guys in the API ?'
>
>>>
>>> Also, I strongly believe that DNs and RDNs and AVAs should be
>>> immutable objects (as well as any other low level API type). What do
>>> you think?
>> DN and RDN should be immutable, sure. AVA, I have some doubt.
>
> If AVA is mutable then it is impossible for DN and RDN to be immutable
> unless they do defensive copies which will be a bit annoying.
I'm probably confusing the creation of AVA and the presence of setters.
Anyway, if AVAs is not visible, it' s a non issue.
>
>
>>>
>>> Also, on the subject of AVAs - we have the AVA type as an inner
>>> class in RDN. I'm not particularly happy with this this, but less
>>> happy with it being a standalone class since AVAs are only used in
>>> RDNs and may introduce confusion elsewhere. For example, filters
>>> also use attribute value assertions but these are not the same type
>>> of object as an AVA even if they have the same name. For example,
>>> AVAs (in RDNs) do not allow attribute options or matching rules to
>>> be specified.
>> I don't really like inner classes in this case for two reasons :
>> - It will be a big class, and the RN class while be hundreds of line
>> long. Not cool
>> - If we just use an Inner class just because we want to hide it from
>> the other classes, then I think it's probably better to have it
>> package protected (ie, no qualifier for this class).
>>
>
> If it's package protected then the class will not be part of the
> public API. I'm not sure that I understand?
Well, it's to protect the class from being used (then no need to make it
immutable ;)
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
On 14/01/10 00:43, Emmanuel Lcharny wrote:
> [...]
> Well, I don't really think that it's anything but implementation
> dependent, so from the API POV, it's irrelevant. As soon as we add the
> valueof() methods, those who want to add a cache can do it.
>
It's very relevant from an API POV. Being future proof is an essential
part of any API. Client applications will need to be modified if a
valueOf constructor is added at a later date in order to take advantage
of any potential perf improvements. By including the valueOf initially
applications can choose to use the constructor knowing that they will
inherit any future improvements such as caching.
If for learnability reasons we decide that a DN(String) constructor is
required then so be it, but it should include Javadoc recommending that
users use the valueOf constructor in preference.
> I don't know why I raised this point...
>>
>>> The base constructor we can have are probably something like:
>>> DN()
>>> DN(String dnStr)
>>> DN( RDN... rdns)
>>> DN( RDN rdn, DN parent)
>>>
>>
>>
>> I like the DN( RDN rdn, DN parent) constructor - we support this via
>> an instance method called DN.child(RDN). I think I prefer the
>> constructor approach since it is not clear from our "child" method
>> whether or not it modifies this DN or creates a new DN. A
>> constructor is more obvious. You may want to have a concatenation
>> constructor DN(DN child, DN parent) for constructing DNs of entries
>> within a subtree using a relative DN (or "local name").
> Why not a DN(DN child, DN parent) constructor two. It does not hurt
> and can help.
OK.
>>
>> One thing that is a bit tricky is whether or not the API should order
>> RDN parameters in little-endian (LDAP World) order or big-endian
>> (everyone else outside of LDAP) order. I think first time users may
>> be surprised by LDAP's unnatural little endian ordering.
> I think we should keep the LDAP order when using DN( RDN...)
> constructor. For instance, if we want to create "dc=example, dc=org",
> that would be :
> DN( "dc=example", "dc=org") (here, I use the String, but you should
> read RDN)
Ok, fair enough.
>>
>> Also, I strongly believe that DNs and RDNs and AVAs should be
>> immutable objects (as well as any other low level API type). What do
>> you think?
> DN and RDN should be immutable, sure. AVA, I have some doubt.
If AVA is mutable then it is impossible for DN and RDN to be immutable
unless they do defensive copies which will be a bit annoying.
>>
>> Also, on the subject of AVAs - we have the AVA type as an inner class
>> in RDN. I'm not particularly happy with this this, but less happy
>> with it being a standalone class since AVAs are only used in RDNs and
>> may introduce confusion elsewhere. For example, filters also use
>> attribute value assertions but these are not the same type of object
>> as an AVA even if they have the same name. For example, AVAs (in
>> RDNs) do not allow attribute options or matching rules to be specified.
> I don't really like inner classes in this case for two reasons :
> - It will be a big class, and the RN class while be hundreds of line
> long. Not cool
> - If we just use an Inner class just because we want to hide it from
> the other classes, then I think it's probably better to have it
> package protected (ie, no qualifier for this class).
>
If it's package protected then the class will not be part of the public
API. I'm not sure that I understand?
I agree about inner classes. They look even uglier in the API Javadoc...
Matt
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Matthew Swift a écrit :
> Hi Emmanuel,
Hi Matthew,
comments inline
>
>
>> 2) In Java, it's pretty natural to create objects through constructor.
>
>
> This may be true for constructors which compose the constructor's
> parameters (e.g. list of RDNs or RDN+DN) into a new object (DN), but
> it is definitely not true for constructors which perform parsing or
> type conversions. There are very many examples in J2SE:
>
> * String.valueOf(...) -converting objects to Strings
>
> * Integer.valueOf(int) - converting an int to an Integer
>
> * Integer.valueOf(String) - parsing a String as an Integer
>
> * plus other primitive Objects (Boolean, Byte, Char, Float, ...)
>
>
> In fact, the use of static factory methods is a very common design
> idiom and is strongly recommended in many texts including Joshua
> Bloch's "Effective Java" (item #1). By using static factories with
> well known names (e.g. valueOf) we are able to avoid creating
> duplicate objects (item 4) if this proves useful (i.e. through the use
> of a cache) and also enforce the singleton property for the root DN
> (item #2).
In any case, I thinkw e should have the constructors. Even for Integer,
you can do new Integer(n).
>> 3) I'm not sure that handling a cache for DN is a valuable trick, as it
>> leads to some contention and the need to manage this cache in a
>> muli-threaded environement (this has to be carefully evaluated)
>>
>
> I have done already and it results in a 30-40% parsing performance
> improvement as well as a significant reduction in garbage. Having said
> that, I'm not totally convinced that this gain is really worth the
> extra complexity and old generation memory usage - especially
> considering large server based applications may have 1000s of threads
> - e.g using LDAP from within a servlet - all those thread local caches
> could use a significant amount of memory!
>
> Our implementation uses a small thread/schema local cache mapping
> String values to DNs. In most applications I think that it's
> reasonable to assume that 99% (perhaps more) DNs share the same parent
> or grand parent. When parsing a DN we first check if the string is in
> the cache and, if not, parse the RDN and recursively repeat for the
> parent DN (in the SDK a DN is implemented recursively as an RDN+DN).
Well, I don't really think that it's anything but implementation
dependent, so from the API POV, it's irrelevant. As soon as we add the
valueof() methods, those who want to add a cache can do it.
I don't know why I raised this point...
>
>> The base constructor we can have are probably something like:
>> DN()
>> DN(String dnStr)
>> DN( RDN... rdns)
>> DN( RDN rdn, DN parent)
>>
>
>
> I like the DN( RDN rdn, DN parent) constructor - we support this via
> an instance method called DN.child(RDN). I think I prefer the
> constructor approach since it is not clear from our "child" method
> whether or not it modifies this DN or creates a new DN. A constructor
> is more obvious. You may want to have a concatenation constructor
> DN(DN child, DN parent) for constructing DNs of entries within a
> subtree using a relative DN (or "local name").
Why not a DN(DN child, DN parent) constructor two. It does not hurt and
can help.
>
> One thing that is a bit tricky is whether or not the API should order
> RDN parameters in little-endian (LDAP World) order or big-endian
> (everyone else outside of LDAP) order. I think first time users may be
> surprised by LDAP's unnatural little endian ordering.
I think we should keep the LDAP order when using DN( RDN...)
constructor. For instance, if we want to create "dc=example, dc=org",
that would be :
DN( "dc=example", "dc=org") (here, I use the String, but you should read
RDN)
>
> Also, I strongly believe that DNs and RDNs and AVAs should be
> immutable objects (as well as any other low level API type). What do
> you think?
DN and RDN should be immutable, sure. AVA, I have some doubt.
>
> Also, on the subject of AVAs - we have the AVA type as an inner class
> in RDN. I'm not particularly happy with this this, but less happy with
> it being a standalone class since AVAs are only used in RDNs and may
> introduce confusion elsewhere. For example, filters also use attribute
> value assertions but these are not the same type of object as an AVA
> even if they have the same name. For example, AVAs (in RDNs) do not
> allow attribute options or matching rules to be specified.
I don't really like inner classes in this case for two reasons :
- It will be a big class, and the RN class while be hundreds of line
long. Not cool
- If we just use an Inner class just because we want to hide it from the
other classes, then I think it's probably better to have it package
protected (ie, no qualifier for this class).
This can be discussed further, as your pont about its use in other
context can be figured out.
Thanks !
Re: [DN] Existing API review
Posted by Ludovic Poitou <Lu...@Sun.COM>.
On Jan 13, 2010, at 1:03 PM, Francois wrote:
> Le 13/01/2010 12:24, Matthew Swift a écrit :
>> Hi Emmanuel,
>
> I'm just giveng my point of view about the following, I don't think I can be relevant elsewhere:
>
>> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
>> objects (as well as any other low level API type). What do you think?
>
> I will go further : DN, RDN and AVA should be immutable, but Attribute should be immutable too, and Entry should have at least an immutable DN (and facilities to copy an entry with a new DN or ParentDN at factory level).
>
> For attribute, the rationnal is that one replace attribute by a new one most of the time,
That is true for attributes that hold a single value or very few.
However, not all attributes are single valued and I would strongly discourage anyone to do this for Groups where the members attribute can contain a huge number of values (yes we have customers with over 100 000 members in a group).
> and it's much easier to deal with immutable attribute - that's one of the aspect of UnboundId SDK that I prefer.
>
> For Entry's DN, it's linked to the fact that DN are almost IDs for entries. So, the semantic of such an operation is much likely in two cases:
> - create a copy of an entry with another DN (change RDN but perhaps not parentDN)
> - move an entry (only the parent DN change).
> These two cases (I hope I don't forget other ones) are easily fulfilled througth factory-like method support.
>
>
> --
> Francois ARMAND
> http://fanf42.blogspot.com
>
---
Ludovic Poitou Sun Microsystems Inc.
OpenDS Community Manager Directory Services
http://blogs.sun.com/Ludo/ Grenoble Engineering Center - France
Join OpenDS, https://opends.dev.java.net/servlets/ProjectMembershipRequest
Sun Microsystems requires the following notice:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged information.
Any unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Matthew Swift a écrit :
>>> and Entry should have at least an immutable DN (and facilities to
>>> copy an entry with a new DN or ParentDN at factory level).
>> If the DN is immutable, entry's DN will be too.
>
>
> I think Francois is stating that Entry should have a DN getter but no
> setter. In other words the DN is set during construction and cannot be
> changed afterwards. I personally think that this restriction is a bit
> artificial.
You mean it will be a real PITA ;) Every ModDN operation applied to
entries will force us to duplicate the full entry. No way !
> A similar artificial constraint could be imposed on the objectClass
> attribute (i.e. don't allow the structural object class to be
> changed). Whilst technically correct at the LDAP protocol level, I
> don't think that these constraints should be imposed at the API level.
> For example, imagine a GUI for creating a new user entry. The DN is
> going to be constructed from (usually) the cn or uid attribute. The
> user will want to type in the cn/uid in an entry box, but then may
> realize that they made a typo and need to correct the cn/uid. We don't
> want to prevent them from doing this?
No, certainly not ! Seriously, it would be too rigid, even on the client
side.
>
>
>>>
>>> For attribute, the rationnal is that one replace attribute by a new
>>> one most of the time, and it's much easier to deal with immutable
>>> attribute - that's one of the aspect of UnboundId SDK that I prefer.
>> Inside the server, having immutable Attribute is not really a
>> problem. From the client POV, I'm not convinced...
>
> I totally agree. Client apps are very different to the server.
> Interestingly most of the pain we had with the immutable Attribute API
> in the server was in client apps such as LDIF tools and unit tests
> (most unit tests are like mini client apps).
Totally on the same page.
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
On 13/01/10 13:34, Emmanuel Lcharny wrote:
> Francois a écrit :
>> Le 13/01/2010 12:24, Matthew Swift a écrit :
>>> Hi Emmanuel,
>>
>> I'm just giveng my point of view about the following, I don't think I
>> can be relevant elsewhere:
>>
>>> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
>>> objects (as well as any other low level API type). What do you think?
>>
>> I will go further : DN, RDN and AVA should be immutable
> +1
+1 from me too.
>> but Attribute should be immutable too,
> Well, it would be a bit inconvenient when manipulating values...
It is - believe me. I had everyone complaining about it to me! :-)
>> and Entry should have at least an immutable DN (and facilities to
>> copy an entry with a new DN or ParentDN at factory level).
> If the DN is immutable, entry's DN will be too.
I think Francois is stating that Entry should have a DN getter but no
setter. In other words the DN is set during construction and cannot be
changed afterwards. I personally think that this restriction is a bit
artificial. A similar artificial constraint could be imposed on the
objectClass attribute (i.e. don't allow the structural object class to
be changed). Whilst technically correct at the LDAP protocol level, I
don't think that these constraints should be imposed at the API level.
For example, imagine a GUI for creating a new user entry. The DN is
going to be constructed from (usually) the cn or uid attribute. The user
will want to type in the cn/uid in an entry box, but then may realize
that they made a typo and need to correct the cn/uid. We don't want to
prevent them from doing this?
>>
>> For attribute, the rationnal is that one replace attribute by a new
>> one most of the time, and it's much easier to deal with immutable
>> attribute - that's one of the aspect of UnboundId SDK that I prefer.
> Inside the server, having immutable Attribute is not really a problem.
> From the client POV, I'm not convinced...
I totally agree. Client apps are very different to the server.
Interestingly most of the pain we had with the immutable Attribute API
in the server was in client apps such as LDIF tools and unit tests (most
unit tests are like mini client apps).
Matt
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Francois a écrit :
> Le 13/01/2010 12:24, Matthew Swift a écrit :
>> Hi Emmanuel,
>
> I'm just giveng my point of view about the following, I don't think I
> can be relevant elsewhere:
>
>> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
>> objects (as well as any other low level API type). What do you think?
>
> I will go further : DN, RDN and AVA should be immutable,
+1
> but Attribute should be immutable too,
Well, it would be a bit inconvenient when manipulating values...
> and Entry should have at least an immutable DN (and facilities to copy
> an entry with a new DN or ParentDN at factory level).
If the DN is immutable, entry's DN will be too.
>
> For attribute, the rationnal is that one replace attribute by a new
> one most of the time, and it's much easier to deal with immutable
> attribute - that's one of the aspect of UnboundId SDK that I prefer.
Inside the server, having immutable Attribute is not really a problem.
From the client POV, I'm not convinced...
>
> For Entry's DN, it's linked to the fact that DN are almost IDs for
> entries. So, the semantic of such an operation is much likely in two
> cases:
> - create a copy of an entry with another DN (change RDN but perhaps
> not parentDN)
> - move an entry (only the parent DN change).
> These two cases (I hope I don't forget other ones) are easily
> fulfilled througth factory-like method support.
>
>
Re: [DN] Existing API review
Posted by Emmanuel Lcharny <el...@gmail.com>.
Matthew Swift a écrit :
>
>
> On 13/01/10 13:03, Francois wrote:
>> Le 13/01/2010 12:24, Matthew Swift a écrit :
>>> Hi Emmanuel,
>>
>> I'm just giveng my point of view about the following, I don't think I
>> can be relevant elsewhere:
>>
>>> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
>>> objects (as well as any other low level API type). What do you think?
>>
>> I will go further : DN, RDN and AVA should be immutable, but
>> Attribute should be immutable too, and Entry should have at least an
>> immutable DN (and facilities to copy an entry with a new DN or
>> ParentDN at factory level).
>>
>> For attribute, the rationnal is that one replace attribute by a new
>> one most of the time, and it's much easier to deal with immutable
>> attribute - that's one of the aspect of UnboundId SDK that I prefer.
>
> I understand very well why you think Attribute could be immutable. In
> fact, I went to great lengths to convert our Attribute API in OpenDS
> server so that it was immutable in order to be able to optimize for
> the common case (single valued attributes) and to avoid defensive
> copies. The result was a 4x performance improvement in the server (!).
> However, this did introduce some pain - any code in the server which
> tried to modify an attribute became more complex. For example, I had
> to implement an AttributeBuilder class to facilitate incremental
> construction of Attributes.
>
> In fact, ironically, I heard on the grape vine that Neil Wilson was
> pretty impressed with the improvements that we got and this is why
> UnboundID's Attribute class is immutable (except that Neil, being
> notoriously bad at API design, has not made it fully immutable - look
> at all those arrays with Javadoc warnings saying that you are not
> allowed to modify the content. Very suspect!).
>
> I thought about this long and hard for the OpenDS SDK and decided that
> a client API with an immutable Attribute would not be very easy to
> use. Many client apps will want to create and modify attributes and we
> should make it as easy as possible. In addition, I think that it is
> very likely that some users of the SDK (including the server itself)
> will need to have different types of Attribute - in other words,
> they'll need to sub-class Attribute (e.g. in order to create lazy
> decoded Attributes, virtual Attributes, copy on write, synchronized,
> etc). By implication any class which is non-final is not immutable
> since a sub-class could be implemented which breaks the contract (I
> realized that I had made this mistake in the server since our
> Attribute class is in fact an interface).
>
> Therefore the approach I took in the OpenDS SDK was to copy the
> Collections API approach. Provide a high-level interface providing
> read/write operations (side note: an Attribute can be thought of as a
> Set of values) and then provide an "unmodifiable" adapter as per
> Collections.unmodifiableSet. Then we get the best of all worlds -
> including the ability to avoid defensive copies by implementing
> getters which return "unmodifiable" Attributes (the OpenDS SDK
> implementation also optimizes for the case where an attribute is
> single valued even though it is mutable).
I must say that we went through the same journey on ADS : from Immutable
Attributes to mutable ones, for almost the same reason. People have to
understand that once an Attribute comes into the server it *will* be
modified (modifiersName and timestamp, probably the addition of missing
ObjectClass, UUID and entryCSN for addition...). From the client POV,
well, as soon as you can clone the entry, I would say it's probably enough.
Re: [DN] Existing API review
Posted by Francois <fa...@gmail.com>.
Le 14/01/2010 22:12, Matthew Swift a écrit :
Matthew, thanks for your answer, response inlined :
> I understand very well why you think Attribute could be immutable. In
> fact, I went to great lengths to convert our Attribute API in OpenDS
> server so that it was immutable in order to be able to optimize for the
> common case (single valued attributes) and to avoid defensive copies.
> The result was a 4x performance improvement in the server (!). However,
> this did introduce some pain - any code in the server which tried to
> modify an attribute became more complex. For example, I had to implement
> an AttributeBuilder class to facilitate incremental construction of
> Attributes.
Well, some part may have been for simpler too. Appart from optimization
and caching in particular, the other usual suspect is multi threading
management where attributes are involved (I don't know what OpenDS is
doing about it)
> look at
> all those arrays with Javadoc warnings saying that you are not allowed
> to modify the content. Very suspect!).
:) I shot my eyes on them and said to myself they don't exist ;)
> I thought about this long and hard for the OpenDS SDK and decided that a
> client API with an immutable Attribute would not be very easy to use.
> Many client apps will want to create and modify attributes and we should
> make it as easy as possible.
I'm not totally convinced about that, because I'm only a client user,
and I do like immutable attribute :)
But perhaps it's because I'm only working on Scala, and that collection
are far more powerful and easier to deals with than in java - for
example, creating a new attribute from the value of another filtered in
some way and then transformed is a matter of one (not long) line -
really, filter, map and flatMap are missing in Java.
> In addition, I think that it is very likely
> that some users of the SDK (including the server itself) will need to
> have different types of Attribute - in other words, they'll need to
> sub-class Attribute (e.g. in order to create lazy decoded Attributes,
> virtual Attributes, copy on write, synchronized, etc). By implication
> any class which is non-final is not immutable since a sub-class could be
> implemented which breaks the contract (I realized that I had made this
> mistake in the server since our Attribute class is in fact an interface).
OK, I understand that. What I fear it is that if attribute are mutable
in their interface, they will always be think about as that, and
immutable attributes will come as a surprise with runtimeerror - it's
what I dislike in the Java collection API approach: having mutation
operator that just throws errors in place of simply not being here.
> I agree that Entry DNs should not in general be modified, but I think
> preventing clients from doing this is a bit of an artificial constraint.
> There's no hard reason I can think of why you would want to prevent
> modification.
>
> In addition, if Entry is an interface (as it is in the OpenDS SDK) then,
> like Attribute, adapters can by provided which render some Entry
> operations unsupported. For example, you could have a Collections-like
> unmodifiableEntry method or even a unrenamableEntry in order to address
> the requirement above.
Perhaps... I still don't like very much the design pattern that make
compilation ok for methods that we know will always throw "not
supported" exception.
So... Well, I really tend to prefer immutable data structures as a
client user, but I may be the only one in that case (and I don't thing
there will be a lot of users of the API that would be on my side).
Goog evening,
--
Francois ARMAND
http://fanf42.blogspot.com
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
On 13/01/10 13:03, Francois wrote:
> Le 13/01/2010 12:24, Matthew Swift a écrit :
>> Hi Emmanuel,
>
> I'm just giveng my point of view about the following, I don't think I
> can be relevant elsewhere:
>
>> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
>> objects (as well as any other low level API type). What do you think?
>
> I will go further : DN, RDN and AVA should be immutable, but Attribute
> should be immutable too, and Entry should have at least an immutable
> DN (and facilities to copy an entry with a new DN or ParentDN at
> factory level).
>
> For attribute, the rationnal is that one replace attribute by a new
> one most of the time, and it's much easier to deal with immutable
> attribute - that's one of the aspect of UnboundId SDK that I prefer.
I understand very well why you think Attribute could be immutable. In
fact, I went to great lengths to convert our Attribute API in OpenDS
server so that it was immutable in order to be able to optimize for the
common case (single valued attributes) and to avoid defensive copies.
The result was a 4x performance improvement in the server (!). However,
this did introduce some pain - any code in the server which tried to
modify an attribute became more complex. For example, I had to implement
an AttributeBuilder class to facilitate incremental construction of
Attributes.
In fact, ironically, I heard on the grape vine that Neil Wilson was
pretty impressed with the improvements that we got and this is why
UnboundID's Attribute class is immutable (except that Neil, being
notoriously bad at API design, has not made it fully immutable - look at
all those arrays with Javadoc warnings saying that you are not allowed
to modify the content. Very suspect!).
I thought about this long and hard for the OpenDS SDK and decided that a
client API with an immutable Attribute would not be very easy to use.
Many client apps will want to create and modify attributes and we should
make it as easy as possible. In addition, I think that it is very likely
that some users of the SDK (including the server itself) will need to
have different types of Attribute - in other words, they'll need to
sub-class Attribute (e.g. in order to create lazy decoded Attributes,
virtual Attributes, copy on write, synchronized, etc). By implication
any class which is non-final is not immutable since a sub-class could be
implemented which breaks the contract (I realized that I had made this
mistake in the server since our Attribute class is in fact an interface).
Therefore the approach I took in the OpenDS SDK was to copy the
Collections API approach. Provide a high-level interface providing
read/write operations (side note: an Attribute can be thought of as a
Set of values) and then provide an "unmodifiable" adapter as per
Collections.unmodifiableSet. Then we get the best of all worlds -
including the ability to avoid defensive copies by implementing getters
which return "unmodifiable" Attributes (the OpenDS SDK implementation
also optimizes for the case where an attribute is single valued even
though it is mutable).
>
> For Entry's DN, it's linked to the fact that DN are almost IDs for
> entries. So, the semantic of such an operation is much likely in two
> cases:
> - create a copy of an entry with another DN (change RDN but perhaps
> not parentDN)
> - move an entry (only the parent DN change).
> These two cases (I hope I don't forget other ones) are easily
> fulfilled througth factory-like method support.
[Disclaimer: the OpenDS SDK Entry API is not finalized yet in case
you're tempted to look at it. IMO it's way to bloated still and contains
some strange methods]
I agree that Entry DNs should not in general be modified, but I think
preventing clients from doing this is a bit of an artificial constraint.
There's no hard reason I can think of why you would want to prevent
modification.
In addition, if Entry is an interface (as it is in the OpenDS SDK) then,
like Attribute, adapters can by provided which render some Entry
operations unsupported. For example, you could have a Collections-like
unmodifiableEntry method or even a unrenamableEntry in order to address
the requirement above.
Matt
Re: [DN] Existing API review
Posted by Francois <fa...@gmail.com>.
Le 13/01/2010 12:24, Matthew Swift a écrit :
> Hi Emmanuel,
I'm just giveng my point of view about the following, I don't think I
can be relevant elsewhere:
> Also, I strongly believe that DNs and RDNs and AVAs should be immutable
> objects (as well as any other low level API type). What do you think?
I will go further : DN, RDN and AVA should be immutable, but Attribute
should be immutable too, and Entry should have at least an immutable DN
(and facilities to copy an entry with a new DN or ParentDN at factory
level).
For attribute, the rationnal is that one replace attribute by a new one
most of the time, and it's much easier to deal with immutable attribute
- that's one of the aspect of UnboundId SDK that I prefer.
For Entry's DN, it's linked to the fact that DN are almost IDs for
entries. So, the semantic of such an operation is much likely in two cases:
- create a copy of an entry with another DN (change RDN but perhaps not
parentDN)
- move an entry (only the parent DN change).
These two cases (I hope I don't forget other ones) are easily fulfilled
througth factory-like method support.
--
Francois ARMAND
http://fanf42.blogspot.com
Re: [DN] Existing API review
Posted by Matthew Swift <Ma...@Sun.COM>.
Hi Emmanuel,
Thanks for doing this (happy new year by the way!). I've put my
responses inline...
On 12/01/10 14:29, Emmanuel Lecharny wrote:
> Hi,
>
> yesturday, I conducted a review on the existing APIs for the DN class :
> - Name/LdapName (JNDI) (I will call it JNDI)
> - jLdap/LdapSdk (I will call it LSD)
> - ApacheDS (ADS)
> - OpenDS (ODS)
> - UnbounID (UID)
>
> There are some important differences. There are two sets of API,
> depending on the existence of a constructor, or not.
>
> - API having a constructor :
> JNDI,
> ADS,
> UID
>
> - API not having a constructor :
> LSD,
> ODS (ODS uses a static method valueof() to produce a DN)
>
> IMHO, I really think that users prefer having a constructor over none or
> a static factory. There are three rasons for that :
> 1) Most of LDAP users are using the JNDI API, which has a LdapNAME()
> constructor
Agreed.
> 2) In Java, it's pretty natural to create objects through constructor.
This may be true for constructors which compose the constructor's
parameters (e.g. list of RDNs or RDN+DN) into a new object (DN), but it
is definitely not true for constructors which perform parsing or type
conversions. There are very many examples in J2SE:
* String.valueOf(...) -converting objects to Strings
* Integer.valueOf(int) - converting an int to an Integer
* Integer.valueOf(String) - parsing a String as an Integer
* plus other primitive Objects (Boolean, Byte, Char, Float, ...)
In fact, the use of static factory methods is a very common design idiom
and is strongly recommended in many texts including Joshua Bloch's
"Effective Java" (item #1). By using static factories with well known
names (e.g. valueOf) we are able to avoid creating duplicate objects
(item 4) if this proves useful (i.e. through the use of a cache) and
also enforce the singleton property for the root DN (item #2).
> 3) I'm not sure that handling a cache for DN is a valuable trick, as it
> leads to some contention and the need to manage this cache in a
> muli-threaded environement (this has to be carefully evaluated)
>
I have done already and it results in a 30-40% parsing performance
improvement as well as a significant reduction in garbage. Having said
that, I'm not totally convinced that this gain is really worth the extra
complexity and old generation memory usage - especially considering
large server based applications may have 1000s of threads - e.g using
LDAP from within a servlet - all those thread local caches could use a
significant amount of memory!
Our implementation uses a small thread/schema local cache mapping String
values to DNs. In most applications I think that it's reasonable to
assume that 99% (perhaps more) DNs share the same parent or grand
parent. When parsing a DN we first check if the string is in the cache
and, if not, parse the RDN and recursively repeat for the parent DN (in
the SDK a DN is implemented recursively as an RDN+DN).
> I think that we should have some basic constructors, and if the cache is
> proven to be valuable, then we can extend the API by adding the valueof(
> ... ) method.
>
Even if you decide that caching is not required then that's no reason to
develop an API which prevents you from implementing one in future. Using
a normal constructor prevents the use of a cache (or forces you to use
the pimpl idiom). If you extend the API later by adding a valueOf method
then no existing applications will be able to take advantage of the perf
improvement unless they are modified to use the new static factory method.
Incidentally, I'm not sure how representative of a typical LDAP
application OpenDS is, but I found that the String based constructor
(valueOf) is by far the most heavily used (2000 references) compared
with <20 references for the others. The most references occur in the
unit tests which are probably the most "client like" part of the server.
> The base constructor we can have are probably something like:
> DN()
> DN(String dnStr)
> DN( RDN... rdns)
> DN( RDN rdn, DN parent)
>
> Thoughts ?
>
I like the DN( RDN rdn, DN parent) constructor - we support this via an
instance method called DN.child(RDN). I think I prefer the constructor
approach since it is not clear from our "child" method whether or not it
modifies this DN or creates a new DN. A constructor is more obvious.
You may want to have a concatenation constructor DN(DN child, DN parent)
for constructing DNs of entries within a subtree using a relative DN (or
"local name").
One thing that is a bit tricky is whether or not the API should order
RDN parameters in little-endian (LDAP World) order or big-endian
(everyone else outside of LDAP) order. I think first time users may be
surprised by LDAP's unnatural little endian ordering.
Also, I strongly believe that DNs and RDNs and AVAs should be immutable
objects (as well as any other low level API type). What do you think?
Also, on the subject of AVAs - we have the AVA type as an inner class in
RDN. I'm not particularly happy with this this, but less happy with it
being a standalone class since AVAs are only used in RDNs and may
introduce confusion elsewhere. For example, filters also use attribute
value assertions but these are not the same type of object as an AVA
even if they have the same name. For example, AVAs (in RDNs) do not
allow attribute options or matching rules to be specified.
What do you think? Inner class or standalone?
Matt