On 27 nov. 2012, at 14:46, Kiran Ayyagari <[email protected]> wrote:
> On Tue, Nov 27, 2012 at 7:04 PM, Pierre-Arnaud Marcelot <[email protected]> > 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, [email protected] 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
