You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Stefan Seelmann <se...@apache.org> on 2010/06/15 18:14:08 UTC
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
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, <el...@apache.org> 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 )
>
>
>
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
Posted by Emmanuel Lecharny <el...@apache.org>.
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 <se...@apache.org>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, <el...@apache.org> 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