You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Pierre-Arnaud Marcelot <pa...@marcelot.net> on 2012/11/27 14:34:45 UTC
Re: svn commit: r1414171 - /directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
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.
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.
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?
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 );
> + }
> }
> }
>
>
>
Re: svn commit: r1414171 - /directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
Posted by Kiran Ayyagari <ka...@apache.org>.
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 <ka...@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
Re: svn commit: r1414171 - /directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
Posted by Pierre-Arnaud Marcelot <pa...@marcelot.net>.
On 27 nov. 2012, at 14:46, Kiran Ayyagari <ka...@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
Re: svn commit: r1414171 - /directory/apacheds/trunk/protocol-ldap/src/main/java/org/apache/directory/server/ldap/replication/SyncReplConfiguration.java
Posted by Kiran Ayyagari <ka...@apache.org>.
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
> 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?
> 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
> 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