directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kiran Ayyagari <kayyag...@apache.org>
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:06:10 GMT
On Tue, Nov 27, 2012 at 7:31 PM, Pierre-Arnaud Marcelot <pa@marcelot.net>wrote:

> 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... ;-)
>
> yeap, lesson learned

> 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.
>
> great

> 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).
>
> yeah, this logic is no more present, (otoh, this is a mistake but adding
the same attribute multiple times is harmless cause server works with
AttributeType instances)

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
>
>
>


-- 
Kiran Ayyagari
http://keydap.com

Mime
View raw message