directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pierre-Arnaud Marcelot ...@marcelot.net>
Subject Re: svn commit: r1414171 - /directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
Date Tue, 27 Nov 2012 14:01:44 GMT
On 27 nov. 2012, at 14:46, Kiran Ayyagari <kayyagari@apache.org> wrote:

> On Tue, Nov 27, 2012 at 7:04 PM, Pierre-Arnaud Marcelot <pa@marcelot.net> wrote:
> Hi Kiran,
> 
> This is not exactly what we usually call a setter method as we never clear the list of
attributes and we keep adding things to it each time we use the method.
> 
> After the SyncReplConfiguration object is instanciated, there's no way to set the actual
list of attributes to something you want or to reset it.
> 
> actually this isn't supposed to be modified after instantiating, but I have never added
such protection in this class

Hehe, you never know what people will do... ;-)

> That's why I changed it a few days ago to be able to use in the configuration editor
where I need to be able to store values from the user.
> 
> just curious, aren't we supposed to use the ReplConsumerBean for configuration editing?


Yeah, you're right, that's the class we're using.
I might have been using SyncReplConfiguration at first and figured out the setAttributes()
method cumulative behavior because of it, I guess.
It's not used anymore in the editor now.

> I'm not against a cumulative method, but I think it would better to have it named addAttributes()
and leave the setter as a way to really set the actual list of attributes.
> 
> WDYT?
> 
> no, this is fine, I have thought of moving this(addition of + to attribute list) to the
server side but didn't, apparently now
> is the time to do it

Yeah, that might be safer, especially after we have a graphical editor where people can do
lots of nasty stuff.
It '+' is mandatory for the replication logic on the server-side, I think too the server should
add it.
Otherwise, the replication can't work properly.

One last thing.
In the current implementation we test the presence of the lower-cased attr in the list but
we're storing the attribute as-is.
So I guess, it might be possible to add the same attribute multiple times ('ObjectClass',
'objectClass', 'objectclass' for example).

Regards,
Pierre-Arnaud


> Regards,
> Pierre-Arnaud
> 
> On 27 nov. 2012, at 14:16, kayyagari@apache.org wrote:
> 
> > Author: kayyagari
> > Date: Tue Nov 27 13:16:43 2012
> > New Revision: 1414171
> >
> > URL: http://svn.apache.org/viewvc?rev=1414171&view=rev
> > Log:
> > do not remove operational attributes from the requested attribute types
> >
> > Modified:
> >    directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
> >
> > Modified: directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
> > URL: http://svn.apache.org/viewvc/directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java?rev=1414171&r1=1414170&r2=1414171&view=diff
> > ==============================================================================
> > --- directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
(original)
> > +++ directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
Tue Nov 27 13:16:43 2012
> > @@ -304,11 +304,26 @@ public class SyncReplConfiguration imple
> >      */
> >     public void setAttributes( String[] attrs )
> >     {
> > -        attributes.clear();
> > -
> > -        for ( String attr : attrs )
> > +        if ( attrs == null )
> >         {
> > -            attributes.add( attr );
> > +            throw new IllegalArgumentException( "attributes to be replicated cannot
be null or empty" );
> > +        }
> > +
> > +        // if user specified some attributes then remove the * from attributes
> > +        // NOTE: if the user specifies * in the given array that eventually gets
added later
> > +        if ( attrs.length > 0 )
> > +        {
> > +            attributes.remove( SchemaConstants.ALL_USER_ATTRIBUTES );
> > +        }
> > +
> > +        for ( String at : attrs )
> > +        {
> > +            at = at.trim();
> > +
> > +            if ( !attributes.contains( Strings.toLowerCase( at ) ) )
> > +            {
> > +                attributes.add( at );
> > +            }
> >         }
> >     }
> >
> >
> >
> 
> 
> 
> 
> -- 
> Kiran Ayyagari
> http://keydap.com


Mime
View raw message