Return-Path: Delivered-To: apmail-directory-dev-archive@www.apache.org Received: (qmail 81637 invoked from network); 15 Jun 2010 16:34:24 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 15 Jun 2010 16:34:24 -0000 Received: (qmail 69409 invoked by uid 500); 15 Jun 2010 16:34:24 -0000 Delivered-To: apmail-directory-dev-archive@directory.apache.org Received: (qmail 69346 invoked by uid 500); 15 Jun 2010 16:34:24 -0000 Mailing-List: contact dev-help@directory.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: "Apache Directory Developers List" Delivered-To: mailing list dev@directory.apache.org Received: (qmail 69339 invoked by uid 99); 15 Jun 2010 16:34:23 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 15 Jun 2010 16:34:23 +0000 X-ASF-Spam-Status: No, hits=-1997.8 required=10.0 tests=ALL_TRUSTED,HTML_MESSAGE,T_RP_MATCHES_RCVD X-Spam-Check-By: apache.org Received: from [140.211.11.9] (HELO minotaur.apache.org) (140.211.11.9) by apache.org (qpsmtpd/0.29) with SMTP; Tue, 15 Jun 2010 16:34:20 +0000 Received: (qmail 81463 invoked by uid 99); 15 Jun 2010 16:33:58 -0000 Received: from localhost.apache.org (HELO mail-fx0-f50.google.com) (127.0.0.1) (smtp-auth username elecharny, mechanism plain) by minotaur.apache.org (qpsmtpd/0.29) with ESMTP; Tue, 15 Jun 2010 16:33:58 +0000 Received: by fxm1 with SMTP id 1so3941509fxm.37 for ; Tue, 15 Jun 2010 09:33:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.98.19 with SMTP id o19mr7554069fan.80.1276619636413; Tue, 15 Jun 2010 09:33:56 -0700 (PDT) Reply-To: elecharny@apache.org Received: by 10.223.119.67 with HTTP; Tue, 15 Jun 2010 09:33:56 -0700 (PDT) In-Reply-To: References: <20100615155846.D2CEA23889B3@eris.apache.org> Date: Tue, 15 Jun 2010 18:33:56 +0200 Message-ID: Subject: Re: svn commit: r954942 - in /directory/apacheds/trunk: core/src/main/java/org/apache/directory/server/core/exception/ExceptionInterceptor.java xdbm-partition/src/main/java/org/apache/directory/server/xdbm/AbstractStore.java From: Emmanuel Lecharny To: Apache Directory Developers List Content-Type: multipart/alternative; boundary=0015174ff094f115f00489142beb X-Virus-Checked: Checked by ClamAV on apache.org --0015174ff094f115f00489142beb Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable This is also my vision. If you do it higher in the stack, you don't protect the backend from wrong operation, but that's not really a problem. What I think is that if we decide to put those checks in the chain, then we have to carefuly think about the place we have to check for those conditions, because many interceptors migh have an influence on the result. For instance, if we do a rename, checking for existence before passing through the SchemaInterceptor is a waste if the renamed entry is not compatible with the entry OC (for instance because the new RDN is not valid). Also I think that we know exactly what to do in the backend for each of the operation : - Rename : the old entry must exist, the new entry must be absent - Move : the old entry must exist, the new superior must exist, the new entry must be absent - MoveAndRename : the old entry must exist, the new superior must exist, th= e new entry must be absent Those checks are very basic, and I don't see the added value of having an interceptor, as anyway all those cheks are done in the AbstractStore class (so they are common to all the Partition implementations). Here, it's a choice, and I prefer doing such a checks very late nd very close to the storage, as it's really unlikely that we have some failure. On Tue, Jun 15, 2010 at 6:14 PM, Stefan Seelmann wrote= : > I also wondered how to deal with those duplicate checks. > > On one side it would be nice to do the checks in the interceptor chain > because then the checks don't need to be implemented for each backend. > > However I think it is right that each backend does these checks > because the backend must protect itself and should not accept invalid > data. > > Kind Regards, > Stefan > > > On Tue, Jun 15, 2010 at 5:58 PM, wrote: > > Author: elecharny > > Date: Tue Jun 15 15:58:46 2010 > > New Revision: 954942 > > > > URL: http://svn.apache.org/viewvc?rev=3D954942&view=3Drev > > Log: > > Removed the checks for existence in the Exception interceptor, as those > checks are already done in the backend. > > > > Modified: > > > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/= core/exception/ExceptionInterceptor.java > > > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/directo= ry/server/xdbm/AbstractStore.java > > > > Modified: > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/c= ore/exception/ExceptionInterceptor.java > > URL: > http://svn.apache.org/viewvc/directory/apacheds/trunk/core/src/main/java/= org/apache/directory/server/core/exception/ExceptionInterceptor.java?rev=3D= 954942&r1=3D954941&r2=3D954942&view=3Ddiff > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > --- > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/c= ore/exception/ExceptionInterceptor.java > (original) > > +++ > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/c= ore/exception/ExceptionInterceptor.java > Tue Jun 15 15:58:46 2010 > > @@ -409,34 +409,14 @@ public class ExceptionInterceptor extend > > throws LdapException > > { > > DN oldDn =3D moveAndRenameContext.getDn(); > > - DN newSuperiorDn =3D moveAndRenameContext.getNewSuperiorDn(); > > > > + // Don't allow M&R in the SSSE > > if ( oldDn.equals( subschemSubentryDn ) ) > > { > > throw new LdapUnwillingToPerformException( > ResultCodeEnum.UNWILLING_TO_PERFORM, I18n.err( I18n.ERR_258, > > subschemSubentryDn, subschemSubentryDn ) ); > > } > > > > - // check if child to move exists > > - String msg =3D "Attempt to move to non-existant parent: "; > > - assertHasEntry( moveAndRenameContext, msg, oldDn ); > > - > > - // check if parent to move to exists > > - msg =3D "Attempt to move to non-existant parent: "; > > - assertHasEntry( moveAndRenameContext, msg, newSuperiorDn ); > > - > > - // check to see if target entry exists > > - DN newDn =3D moveAndRenameContext.getNewDn(); > > - > > - if ( nextInterceptor.hasEntry( new EntryOperationContext( > moveAndRenameContext.getSession(), newDn ) ) ) > > - { > > - // we must calculate the resolved name using the user > provided Rdn value > > - LdapEntryAlreadyExistsException e; > > - e =3D new LdapEntryAlreadyExistsException( I18n.err( > I18n.ERR_250_ENTRY_ALREADY_EXISTS, newDn ) ); > > - //e.setResolvedName( new DN( upTarget.getName() ) ); > > - throw e; > > - } > > - > > // Remove the original entry from the NotAlias cache, if needed > > synchronized ( notAliasCache ) > > { > > > > Modified: > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/director= y/server/xdbm/AbstractStore.java > > URL: > http://svn.apache.org/viewvc/directory/apacheds/trunk/xdbm-partition/src/= main/java/org/apache/directory/server/xdbm/AbstractStore.java?rev=3D954942&= r1=3D954941&r2=3D954942&view=3Ddiff > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > --- > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/director= y/server/xdbm/AbstractStore.java > (original) > > +++ > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/director= y/server/xdbm/AbstractStore.java > Tue Jun 15 15:58:46 2010 > > @@ -1203,8 +1203,7 @@ public abstract class AbstractStore > */ > > public synchronized void moveAndRename( DN oldDn, DN newSuperiorDn, > RDN newRdn, Entry modifiedEntry, boolean deleteOldRdn ) throws Exception > > { > > - // Check that the old entry exists, that the new superior exist= s > and > > - // that the new DN does not exist > > + // Check that the old entry exists > > ID oldId =3D getEntryId( oldDn ); > > > > if ( oldId =3D=3D null ) > > @@ -1215,6 +1214,7 @@ public abstract class AbstractStore > throw nse; > > } > > > > + // Check that the new superior exist > > ID newSuperiorId =3D getEntryId( newSuperiorDn ); > > > > if ( newSuperiorId =3D=3D null ) > > > > > > > --=20 Regards, Cordialement, Emmanuel L=E9charny www.iktek.com --0015174ff094f115f00489142beb Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable This is also my vision. If you do it higher in the stack, you don't pro= tect the backend from wrong operation, but that's not really a problem.= =A0

What I think is that if we decide to put those check= s in the chain, then we have to carefuly think about the place we have to c= heck for those conditions, because many interceptors migh have an influence= on the result. For instance, if we do a rename, checking for existence bef= ore passing through the SchemaInterceptor is a waste if the renamed entry i= s not compatible with the entry OC (for instance because the new RDN is not= valid).=A0

Also I think that we know exactly what to do in the bac= kend for each of the operation :
- Rename : the old entry must ex= ist, the new entry must be absent
- Move : the old entry must exi= st, the new superior must exist, the new entry must be absent
- MoveAndRename :=A0the old entry must exist, the new superior must ex= ist, the new entry must be absent

Those checks are= very basic, and I don't see the added value of having an interceptor, = as anyway all those cheks are done in the AbstractStore class (so they are = common to all the Partition implementations).

Here, it's a choice, and I prefer doing such a chec= ks very late nd very close to the storage, as it's really unlikely that= we have some failure.

On Tue, Jun 15, 20= 10 at 6:14 PM, Stefan Seelmann <seelmann@apache.org> wrote:
I also wondered how to deal with those dupl= icate checks.

On one side it would be nice to do the checks in the interceptor chain
because then the checks don't need to be implemented for each backend.<= br>
However I think it is right that each backend does these checks
because the backend must protect itself and should not accept invalid
data.

Kind Regards,
Stefan


On Tue, Jun 15, 2010 at 5:58 PM, =A0<elecharny@apache.org> wrote:
> Author: elecharny
> Date: Tue Jun 15 15:58:46 2010
> New Revision: 954942
>
> URL: http://svn.apache.org/viewvc?rev=3D954942&view=3D= rev
> Log:
> Removed the checks for existence in the Exception interceptor, as thos= e checks are already done in the backend.
>
> Modified:
> =A0 =A0directory/apacheds/trunk/core/src/main/java/org/apache/director= y/server/core/exception/ExceptionInterceptor.java
> =A0 =A0directory/apacheds/trunk/xdbm-partition/src/main/java/org/apach= e/directory/server/xdbm/AbstractStore.java
>
> Modified: directory/apacheds/trunk/core/src/main/java/org/apache/direc= tory/server/core/exception/ExceptionInterceptor.java
> URL: http://svn.apache.org/viewvc/directory/apacheds/trunk/cor= e/src/main/java/org/apache/directory/server/core/exception/ExceptionInterce= ptor.java?rev=3D954942&r1=3D954941&r2=3D954942&view=3Ddiff<= br> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D
> --- directory/apacheds/trunk/core/src/main/java/org/apache/directory/s= erver/core/exception/ExceptionInterceptor.java (original)
> +++ directory/apacheds/trunk/core/src/main/java/org/apache/directory/s= erver/core/exception/ExceptionInterceptor.java Tue Jun 15 15:58:46 2010
> @@ -409,34 +409,14 @@ public class ExceptionInterceptor extend
> =A0 =A0 =A0 =A0 throws LdapException
> =A0 =A0 {
> =A0 =A0 =A0 =A0 DN oldDn =3D moveAndRenameContext.getDn();
> - =A0 =A0 =A0 =A0DN newSuperiorDn =3D moveAndRenameContext.getNewSuper= iorDn();
>
> + =A0 =A0 =A0 =A0// Don't allow M&R in the SSSE
> =A0 =A0 =A0 =A0 if ( oldDn.equals( subschemSubentryDn ) )
> =A0 =A0 =A0 =A0 {
> =A0 =A0 =A0 =A0 =A0 =A0 throw new LdapUnwillingToPerformException( Res= ultCodeEnum.UNWILLING_TO_PERFORM, I18n.err( I18n.ERR_258,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 subschemSubentryDn, subschemSubentryDn= ) );
> =A0 =A0 =A0 =A0 }
>
> - =A0 =A0 =A0 =A0// check if child to move exists
> - =A0 =A0 =A0 =A0String msg =3D "Attempt to move to non-existant = parent: ";
> - =A0 =A0 =A0 =A0assertHasEntry( moveAndRenameContext, msg, oldDn ); > -
> - =A0 =A0 =A0 =A0// check if parent to move to exists
> - =A0 =A0 =A0 =A0msg =3D "Attempt to move to non-existant parent:= ";
> - =A0 =A0 =A0 =A0assertHasEntry( moveAndRenameContext, msg, newSuperio= rDn );
> -
> - =A0 =A0 =A0 =A0// check to see if target entry exists
> - =A0 =A0 =A0 =A0DN newDn =3D moveAndRenameContext.getNewDn();
> -
> - =A0 =A0 =A0 =A0if ( nextInterceptor.hasEntry( new EntryOperationCont= ext( moveAndRenameContext.getSession(), newDn ) ) )
> - =A0 =A0 =A0 =A0{
> - =A0 =A0 =A0 =A0 =A0 =A0// we must calculate the resolved name using = the user provided Rdn value
> - =A0 =A0 =A0 =A0 =A0 =A0LdapEntryAlreadyExistsException e;
> - =A0 =A0 =A0 =A0 =A0 =A0e =3D new LdapEntryAlreadyExistsException( I1= 8n.err( I18n.ERR_250_ENTRY_ALREADY_EXISTS, newDn ) );
> - =A0 =A0 =A0 =A0 =A0 =A0//e.setResolvedName( new DN( upTarget.getName= () ) );
> - =A0 =A0 =A0 =A0 =A0 =A0throw e;
> - =A0 =A0 =A0 =A0}
> -
> =A0 =A0 =A0 =A0 // Remove the original entry from the NotAlias cache, = if needed
> =A0 =A0 =A0 =A0 synchronized ( notAliasCache )
> =A0 =A0 =A0 =A0 {
>
> Modified: directory/apacheds/trunk/xdbm-partition/src/main/java/org/ap= ache/directory/server/xdbm/AbstractStore.java
> URL: http://svn.apache.org/viewvc/directory/apacheds/trunk/xdbm-part= ition/src/main/java/org/apache/directory/server/xdbm/AbstractStore.java?rev= =3D954942&r1=3D954941&r2=3D954942&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D
> --- directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/d= irectory/server/xdbm/AbstractStore.java (original)
> +++ directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/d= irectory/server/xdbm/AbstractStore.java Tue Jun 15 15:58:46 2010
> @@ -1203,8 +1203,7 @@ public abstract class AbstractStore<E, I
> =A0 =A0 =A0*/
> =A0 =A0 public synchronized void moveAndRename( DN oldDn, DN newSuperi= orDn, RDN newRdn, Entry modifiedEntry, boolean deleteOldRdn ) throws Except= ion
> =A0 =A0 {
> - =A0 =A0 =A0 // Check that the old entry exists, that the new superio= r exists and
> - =A0 =A0 =A0 // that the new DN does not exist
> + =A0 =A0 =A0 // Check that the old entry exists
> =A0 =A0 =A0 =A0 ID oldId =3D getEntryId( oldDn );
>
> =A0 =A0 =A0 =A0 if ( oldId =3D=3D null )
> @@ -1215,6 +1214,7 @@ public abstract class AbstractStore<E, I
> =A0 =A0 =A0 =A0 =A0 =A0 throw nse;
> =A0 =A0 =A0 =A0 }
>
> + =A0 =A0 =A0 =A0// Check that the new superior exist
> =A0 =A0 =A0 =A0 ID newSuperiorId =3D getEntryId( newSuperiorDn );
>
> =A0 =A0 =A0 =A0 if ( newSuperiorId =3D=3D null )
>
>
>



--
Regards,Cordialement,
Emmanuel L=E9charny
w= ww.iktek.com
--0015174ff094f115f00489142beb--