Add String() methods to DN and its subtypes

[jlhawn]

This patch adds String() string methods to each of the following types:

• DN
• RelativeDN
• AttributeTypeAndValue

So that a *DN implements the fmt.Stringer interface. These methods also
produce normalized strings: Attribute Type and Value are lowercased and joined
with a "=" character while multiple attributes of a Relative DN are sorted
lexicographically before being joined witha "+" character.

This allows one to use the string representation of a DN as a map key and
ensure that two DNs which Equal() eachother would have the same String()
value.

[liggitt]

First, thanks for the PR. A nice way to print is great. I am a little concerned these will be used to serialize DNs for inclusion in filters or actual queries, and they don't escape any values or special characters.

[jlhawn]

@liggitt I'd be glad to add escaping of the attribute type and value strings. Is there an existing escape function I can use?

[jlhawn]

My intent here is for the String() string methods to return a "canonicalized" distinguished name where the string value can safely be used in queries and have the constraint where for any two DN values A and B where A.Equals(B) is true then A.String() == B.String() is also true.

[liggitt]

it's essentially the reverse of what ParseDN does, though that is not straightforward :-/
I don't think we have an escape function that does that

[jlhawn]

@liggitt I'll refer to RFC 4514 to implement what I hope will be a simple escape function.

[jlhawn]

So far the only thing that I'm not sure about is that when an attribute value is originally parsed from BER-encoded data, I don't know whether or not to re-encode it in the same way. An example from an existing test is:

#04024869 which is parsed into Hi.

[jlhawn]

I'm also not sure if the existing (*AttributeTypeAndValue) Equal(other *AttributeTypeAndValue) bool method is correct since it always considers the value to be case-sensitive. I guess, strictly, it depends on the attribute type and whether the schema description for that type specifies that the value is case sensitive. DNs are typically always case insensitive though aren't they?

[jlhawn]

@liggitt I've updated the PR with a function which escapes the attribute value. I've also added an extra case to the TestDNEqual test function to assert what I mentioned earlier:

the constraint where for any two DN values A and B where A.Equals(B) is true then A.String() == B.String() is also true.

It also asserts the opposite where if one is false then the other must also be false. In order to make this work I had to omit the part which converts the attribute values to be lowercase. I'm still very uncomfortable about that though.

[jlhawn]

On the issue of the attribute value equality matching, the original PR which added the Equal() methods quotes RFC 4517:

Two AVAs with the same attribute type are the same if their values are equal according to the equality matching rule of the attribute type.

So it doesn't appear to be strictly correct to do exact case matching of the string values. However, if you really want case insensitive matching for the Equal() methods (as I do) then you'd need to convert the raw DN string to be all lowercase before ever calling ParseDN().

[jlhawn]

Also the CI failures look unrelated - is it the detected race condition which causes the failure?

[vetinari]

@jlhawn does https://github.com/vetinari/go-ldapdn meet your requirements?

[jlhawn]

@vetinari almost, yes! The only problem that I see is a bug in the (*RelativeDN) Equal() bool method. It considers the ordering of the attributes. Where the RDNs parsed as a=b+c=d and c=d+a=b should be equal, they aren't equal according to that package. The canonicalized strings should also have these multi-attribute RDNs sorted to ensure that if two DNs are equal then their canonicalized string values are also equal.

The String() string method also does not lowercase the value.

[vetinari]

@jlhawn fixed in https://github.com/vetinari/go-ldapdn

[johnweldon]

This looks like it is a superset of the changes in #155? @vetinari should we use this PR as a replacement for the other?

[johnweldon]

@jlhawn any chance you'd want to update this PR and mirror the changes in both the root folder and the v3 folder? I think we can get this merged in if it's updated. Thanks!

[theory]

Was this closed because it was added elsewhere?

[johnweldon]

This was closed along with a bunch of other PR's that have been stagnant for a while. If you'd like to shepherd this feature into the code we'd be happy to facilitate that.

[theory]

Yeah, it's not something I've touched in a couple years and made my own workaround in the service I built, so likely won't find the TUITs for it anytime soon, but def support the idea.

[rhafer]

This was closed along with a bunch of other PR's that have been stagnant for a while. If you'd like to shepherd this feature into the code we'd be happy to facilitate that.

@johnweldon I am somewhat interested in getting this code in and might be able to spent some time on it. Though from the looking at the PR history I am not entirely sure what is missing (apart from mirroring stuff to the v3 folder)

[johnweldon]

@rhafer - we'd welcome your contributions - as far as I recall it seems it's just the mirroring left.