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, 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 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 <[email protected]>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, <[email protected]> wrote: > > Author: elecharny > > Date: Tue Jun 15 15:58:46 2010 > > New Revision: 954942 > > > > URL: http://svn.apache.org/viewvc?rev=954942&view=rev > > 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/directory/server/xdbm/AbstractStore.java > > > > Modified: > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/core/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=954942&r1=954941&r2=954942&view=diff > > > ============================================================================== > > --- > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/core/exception/ExceptionInterceptor.java > (original) > > +++ > directory/apacheds/trunk/core/src/main/java/org/apache/directory/server/core/exception/ExceptionInterceptor.java > Tue Jun 15 15:58:46 2010 > > @@ -409,34 +409,14 @@ public class ExceptionInterceptor extend > > throws LdapException > > { > > DN oldDn = moveAndRenameContext.getDn(); > > - DN newSuperiorDn = 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 = "Attempt to move to non-existant parent: "; > > - assertHasEntry( moveAndRenameContext, msg, oldDn ); > > - > > - // check if parent to move to exists > > - msg = "Attempt to move to non-existant parent: "; > > - assertHasEntry( moveAndRenameContext, msg, newSuperiorDn ); > > - > > - // check to see if target entry exists > > - DN newDn = 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 = 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/directory/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=954942&r1=954941&r2=954942&view=diff > > > ============================================================================== > > --- > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/directory/server/xdbm/AbstractStore.java > (original) > > +++ > directory/apacheds/trunk/xdbm-partition/src/main/java/org/apache/directory/server/xdbm/AbstractStore.java > Tue Jun 15 15:58:46 2010 > > @@ -1203,8 +1203,7 @@ public abstract class AbstractStore<E, I > > */ > > 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 exists > and > > - // that the new DN does not exist > > + // Check that the old entry exists > > ID oldId = getEntryId( oldDn ); > > > > if ( oldId == null ) > > @@ -1215,6 +1214,7 @@ public abstract class AbstractStore<E, I > > throw nse; > > } > > > > + // Check that the new superior exist > > ID newSuperiorId = getEntryId( newSuperiorDn ); > > > > if ( newSuperiorId == null ) > > > > > > > -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
