directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "lucas theisen (JIRA)" <j...@apache.org>
Subject [jira] [Reopened] (DIRAPI-165) Add a FilterBuillder
Date Wed, 28 Jan 2015 13:12:35 GMT

     [ https://issues.apache.org/jira/browse/DIRAPI-165?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

lucas theisen reopened DIRAPI-165:
----------------------------------

[~elecharny]
{quote}
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.
{quote}
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?
{quote}
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 -.
{quote}
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.
{quote}
Last, not least, the Filter.build(...) is clearly missing a lot of things : the attribute
value is to be escaped wrt RFC 4515  (assertionvalue) :
{quote}
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:
{code}
    equals( "givenName", "emman*" );
{code}
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:
{code}
    public static FilterBuilder substring( String attribute, String initial, String end, String...
any )
{code}
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:
{code}
    /** 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)
{code}
This way the parts always show up in order.

What do you think?


> Add a FilterBuillder
> --------------------
>
>                 Key: DIRAPI-165
>                 URL: https://issues.apache.org/jira/browse/DIRAPI-165
>             Project: Directory Client API
>          Issue Type: New Feature
>    Affects Versions: 1.0.0-M21
>            Reporter: lucas theisen
>            Priority: Minor
>             Fix For: 1.0.0-M29
>
>
> Looking for something Fluent, in the _spirit_ of Hibernate Criteria.  May not seem like
much, but can drastically reduce query syntax issues. Also, you would likely be using a StringBuilder
anyway, so its not much different. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message