My own version looks something like this:

    String filter = FilterBuilder.and( 
            FilterBuilder.equals( MAIL_AT, "*@example.com" ), 
            FilterBuilder.notEqualTo( CN_AT, "test" ) ).build();

Totally agree. Let's add that in the next iteration.

Added to revision 1654581

I checked the code, there are at missing parts :

• the ASF headers (they are mandatory, and we can't cut a release without them)
• the classes header : we need the apache directory email address to be added in the header
• The javadoc : we are poor on Javadoc, due to the urge of developping and because we are a bit lazzy, but we try to get this fixed

Currently, the FilterBuilder class is the only one that has everything but the ASF header, and I must admit it's quite well documented. It would be cool if the other classes have a bit of doco, too. Not necessarily that extensive, but still. For test, we don't really care about Javadoc, except for specific tests.

Regarding the Operator enum, we do have an enum in the ldap-model module that could have been used, or at least improved : AssertionType. I think there is some room for improvement in this enum. I also think that there is no need to declare two inner Operator - in UnaryFilter and setOfFiltersFilter -.

Last, not least, the Filter.build(...) is clearly missing a lot of things : the attribute value is to be escaped wrt RFC 4515 (assertionvalue) :

      assertionvalue = valueencoding
      ; The <valueencoding> rule is used to encode an <AssertionValue>
      ; from Section 4.1.6 of [RFC4511].
      valueencoding  = 0*(normal / escaped)
      normal         = UTF1SUBSET / UTFMB
      escaped        = ESC HEX HEX
      UTF1SUBSET     = %x01-27 / %x2B-5B / %x5D-7F
                          ; UTF1SUBSET excludes 0x00 (NUL), LPAREN,
                          ; RPAREN, ASTERISK, and ESC.
      EXCLAMATION    = %x21 ; exclamation mark ("!")
      AMPERSAND      = %x26 ; ampersand (or AND symbol) ("&")
      ASTERISK       = %x2A ; asterisk ("*")
      COLON          = %x3A ; colon (":")
      VERTBAR        = %x7C ; vertical bar (or pipe) ("|")
      TILDE          = %x7E ; tilde ("~")

If we don't do that, we are very likely to have failing requests. I think we have the code that does the escaping in the ldap-model module (Filter)

FTR, I have added the missing AL 2.0 and class headers (a placeholder)

Emmanuel Lecharny

Currently, the FilterBuilder class is the only one that has everything but the ASF header, and I must admit it's quite well documented. It would be cool if the other classes have a bit of doco, too. Not necessarily that extensive, but still. For test, we don't really care about Javadoc, except for specific tests.

Thank you for adding the headers, I completely forgot that. The reason I only javadoc'd FilterBuilder is because it is the only public interface to the FilterBuilder (a facade). All the other classes are package only classes and as such, internal only. Do we still javadoc those?

Regarding the Operator enum, we do have an enum in the ldap-model module that could have been used, or at least improved : AssertionType. I think there is some room for improvement in this enum. I also think that there is no need to declare two inner Operator - in UnaryFilter and setOfFiltersFilter -.

I am not sure I understand this comment. I initially had a single enum, but split it up to provide compile time assurance that the wrong enum value is impossible to supply (cannot give an Operator.NOT to an AttributeValueAssertion constructor). However, as I ended up leaving the constructors private, this probably is an unnecessary protection. I could improve the AssertionType enum you mentioned, but it has some strange values: EXTENSIBLE, SCOPE, ASSERTION, UNDEFINED, OBJECTCLASS. The don't actually have operators, so it would not be possibly to have the operator method in there. Any suggestion on this? Could we remove those 5 values (i assume you put them there for a reason so probably not)? I can look to see where they are used if you think i should.

Last, not least, the Filter.build(...) is clearly missing a lot of things : the attribute value is to be escaped wrt RFC 4515 (assertionvalue) :

I thought about adding escaping, but at the same time I thought it may be confusing to the user. Anybody familiar with LDAP searches knows that * is a glob so I assume they would want to do:

    equals( "givenName", "emman*" );

or something similar. If we internally escape that during the build() method, it would break their search. I understand that they should be using a substring (which I left out because I assumed they would just put in their own * on an equals). Anyway, I saw that you added a substring and, given that, I could certainly add the proper escapes to the build method. Do you think doing escaping would confuse others? It is certainly more correct, and safer, but I could see people missing this quirk. Last, I noticed in your implementation of substring has the signiture:

    public static FilterBuilder substring( String attribute, String initial, String end, String... any )

Which has middle after the end. I understand this was done to leverage the varargs, but using it is very weird. What do you think of breaking the substring function up into:

    /** results in [part_0](*[part_n]) */
    public static FilterBuilder substring( String attribute,  String... parts)
    /** results in [part_0](*[part_n])* */
    public static FilterBuilder startsWith( String attribute, String... parts)
    /** results in *[part_0](*[part_n]) */
    public static FilterBuilder endsWith( String attribute, String... parts)
    /** results in *[part_0](*[part_n])* */
    public static FilterBuilder contains( String attribute, String... parts)

This way the parts always show up in order.

What do you think?

Regarding the Javadoc :

• for private methods, I usually don't add a lot of doco. I certainly don't document the arguments, return and exception, although I sometime do it when it helps someone who didn't wrote the code, if it's complex. I generally drop a single line of text explaining what the method dos.
• for classes (even private) : same thing, but I try to add a doco no matter what.
• for tests : do what you prefer. I don't add doco most of the time, excpet what I'm in a good mood.

I totally missed the fact that most of the classes are package protected. Again, here, I do add things like /* No qualifier */ in front of every class/method/field that is package protected, for clarification.

Anyway, a minimum doco for those classes are cool to have, just in case, and for the giys doing maintenance.

Last, not list, for methods implementing an interface, where the Javadoc is already present, I use something like :

/**
 * {@inheritDoc}
 */

Regarding the Operator : The AssertionType class is quite an old one, and it's used for multiple purpose (there is a smell of bad cumulative design here...).

I would favor the definition of a FilterOperator enum for OR, AND and NOT operators only, but available and used everywhere in the code, instead of AssertionType.

Regarding escapting : if you don't escape, the server will simply reject the search in specific case where you have chars like '*', '(', ')', etc in the value. Well, in fact, it's likely that the client API will not be able to produce a valid SearchRequest, which will be encoded in BER.

If a user want to write things like :

equals( "givenName", "emmanuel is a ***" );

that is fine, but behind the curtain, the *** part should be escaped.

Last, not least, the Substring. I thought a lot about the ordering of elements. Beside the fact that we can't use 'final' (damn reserved keywords...), Java does not like ellipsis except at the end of the args.

One way to put the 'any' part in the middle would be to use String[] instead :

substring( String attr, String initial, String[] any, String end )

and if the user does not want to use 'any', he/she would use :

substring( "cn", "Emm", null, "el" );

That coud be an option. (and it's probably a better solution)

Your proposal is interesting, but you have some missing use cases (I = initial, A = any, F = final) :

• I'*' -> startsWith
• '*'F -> endsWith
• ''(A'')+ -> contains
• I''(A'')+ -> ???
• I'*'F -> ???
• ''(A'')+F -> ???
• I''(A'')+F -> substring

Complicated...

Thanks !

PS : I have a few modifciations not yet commited I'd like to discuss befoe pushing it. I'll add some more comment here.

Forgot to mention that we also need to support extensible filters.

Your proposal is interesting, but you have some missing use cases (I = initial, A = any, F = final) :

This should cover it all:

• I'*' -> startsWith

startsWith("givenName", "emm"); // emm*

• '*'F -> endsWith

endsWith("givenName", "uel"); // *uel

• ''(A'')+ -> contains

contains("givenName", "man", "e"); // *man*e*

• I''(A'')+ -> ???

startsWith("givenName", "emm", "n"); // emm*n*

• I'*'F -> ???

substring("givenName", "emm", "l"); // emm*l

• ''(A'')+F -> ???

endsWith("givenName", "n", "l"); // *n*l

• I''(A'')+F -> substring

substring("givenName", "emm", "n", "l" ); // emm*n*l

All the methods would have String... parts...

You convinced me... I'll change the code accordingly.

The vertue of a good discussion in a community never stop striking me !

One thing I'd like to change is the name of the AttributeFilter to PresentFilter if you don't mind, to reflect the terminology used in the LDAP RFC.

Wdyt ?

The startswith() method would looks like that :

    /**
     * Create a SubstringFilter based on the filter elements. Such a filter
     * has a form like <b>Attribute=initial*([any]*)*</b>. We don't expect any
     * <em>final</em> String.
     *
     * @param attribute The AttributeType for this filter
     * @param parts The parts that are the initial string and zero to N any strings
     * @return An instance of a SubstringFilter
     */
    public static SubstringFilter startswith( String attribute, String... parts )
    {
        if ( ( parts != null ) && ( parts.length > 0 ) )
        {
            if ( parts.length > 1 )
            {
                String[] any = new String[parts.length - 1];
                System.arraycopy( parts, 1, any, 0, any.length );

                return new SubstringFilter( attribute, parts[0], any, null );
            }
            else
            {
                return new SubstringFilter( attribute, parts[0], null, null );
            }
        }
        else
        {
            // This is a presence filter, kind of
            return new SubstringFilter( attribute, null, null, null );
        }
    }

Emmanuel Lecharny

Forgot to mention that we also need to support extensible filters.

I just added extended() to FilterBuilder. I think it should be sufficient to allow for extensible filters. What do you think?

I used AttributeFilter because the RFC says:

Filter ::= CHOICE

Unknown macro: { and [0] SET OF Filter, or [1] SET OF Filter, not [2] Filter, equalityMatch [3] AttributeValueAssertion, substrings [4] SubstringFilter, greaterOrEqual [5] AttributeValueAssertion, lessOrEqual [6] AttributeValueAssertion, present [7] AttributeType, approxMatch [8] AttributeValueAssertion }

So I used the left hand side as a specialization of a filter type, and the right hand side as the filter type. I am by no means an LDAP expert so I wanted to leave the name open in case a future version of the spec included a second type (other than present) of AttributeType filter.

So AttributeFilter be it. Anyway, it's hidden from the users, so...

Substring method added and committed.

Sadly, I discovered a bug in the FilterEncoder.encodeFilterValue()...

I would probably throw an IllegalArgumentException in the empty array case. What you have is nicer but if they really did provide an empty list to a {{startsWith}, its a REALLY good chance they made a mistake. You could enforce it compile time with a:

    public static SubstringFilter startswith( String attribute, String init, String... parts )

but I would not do that because there would be no way to ensure this for endsWith and I like the idea of all of them having the same argument list. I think fail fast with the exception would be best. Finally, you probably meant to camelCase startsWith...

Makes sense.

• Exception added in the SubstringFilter class
• Javadoc added in every classes
• A FilterOperator enum has been created to represent all the operators we can use in the filters

We are alost done with this JIRA !

What remains :

• the extensible filter :

extensible     = ( attr [dnattrs]
                           [matchingrule] COLON EQUALS assertionvalue )
                       / ( [dnattrs]
                            matchingrule COLON EQUALS assertionvalue )

• We need to update the web site user guide too. A new page has to be created.

Great addition to the API !

I have added a preliminary page on the web site, which can be checked on http://directory.staging.apache.org/api/user-guide/2.11-filter-builder.html

The 'extended' Filter that is currently implemented is not doing what one would expect for such a method. Here, we have to follow the RFC tightly.

I am working on the RFC extensibleMatch and will rename extended to custom as it is really just an extension point for adding user designed filter builders.

Also, the RFC i was working from, was the wrong one 1960... Now that you pointed me to 4515, we might want to refactor some names... Not sure to what extent...

I have committed all my changes so far, and I'm open for any renaming. What are you proposing ?

I just finished adding extensible, and renaming extended to custom. I forgot how to update the documentation though, can you remind me?

Sure,checkout the apacheds site files :

svn co https://svn.apache.org/repos/asf/directory/site/trunk

then you can update the pages which are in content/api, more specifically this page : content/api/user-guide/2.11-filter-builder.mdtext (btw, you probably can check what I committed. I basically copied what we have in the current Javadoc). Last, not least, commit the page, it will be visible on http://directory.staging.apache.org/api/user-guide/2.11-filter-builder.html

I updated the site adding extensible(), and custom()... However, now that I look at it, I think custom() might be totally useless. First, it was just something I came up with as an extension point (not part of the RFC). Second, the example I show with Muppets, could be simpler if the MuppetFilter.muppet() returned a FilterBuilder instead of a Filter then there would be no need to call custom( muppet ) (which just wraps it in a FilterBuilder}}... So, before i remove it, do you want to weigh in?

Go ahead, remove it completly.

custom has been removed from the code, tests, and site.

Revision: 1655999

Good job !!!