Re: Refactoring of the backupTool

[Jukka Zitting <ju...@gmail.com>]

Hi,

On 8/31/06, Nicolas <nt...@gmail.com> wrote:
> - RestoreBatchedItemOperations extends BatchedItemOperations. This class is
> made to restore empty workspaces. I overload only one method:
> createNodeState to escape the checks.

How about refactoring the original BatchedItemOperations class to a
base class that doesn't do any of the checks (or has just empty
placeholder methods for the checks) and a subclass that contains the
checks. Or even better, refactor the checks to a separate Checker
class to use composition instead of inheritance for the functinality.
This way there wouldn't be a need to override and duplicate existing
functionality in a subclass as you could achieve the goal simply by
using the base class or a different composition.

> - SysViewImporter: takes a system view XML document and import it as it is
> in an empty repository. It is heavily based on the WorkspaceImporter but
> doesn't extend it (I would have to put too many methods in protected for
> this; tell me if you would be ok with this change though).

I think it would make sense in here as well to refactor the
WorkspaceImporter class instead of duplicating the code in a separate
class.

> - NodeVersionHistoriesUpdatableStateManager: is made to update easily the
> Node Version Histories.

This seems a bit overkill for just the version import. Could the
importer be refactored so that it could work with a smaller interface
than the full UpdatableStateManager?

> - Switched RepositoryImpl.getVersionManager to public instead of private.

I'm a bit concerned about this, the version manager should be private
to the repository. I'd rather see something like a more focused
WorkspaceImpl.getVersionHistoryImporter() method that only works when
you have administrator access.

BR,

Jukka Zitting

-- 
Yukatan - http://yukatan.fi/ - info@yukatan.fi
Software craftsmanship, JCR consulting, and Java development

Re: Refactoring of the backupTool

[Nicolas <nt...@gmail.com>]

Hi

On 9/2/06, Jukka Zitting <ju...@gmail.com> wrote:

> How about refactoring the original BatchedItemOperations class to a
> base class that doesn't do any of the checks (or has just empty
> placeholder methods for the checks) and a subclass that contains the
> checks. Or even better, refactor the checks to a separate Checker
> class to use composition instead of inheritance for the functinality.
> This way there wouldn't be a need to override and duplicate existing
> functionality in a subclass as you could achieve the goal simply by
> using the base class or a different composition.


I agree with you this would be better but one of the design goal of the
backup
tool was to minimize impact to the existing core.

I think it would make sense in here as well to refactor the
> WorkspaceImporter class instead of duplicating the code in a separate
> class.


It is exactly the same issue: some checks are escaped and more control is
given than with the WspImporter.

What I propose is to finish the backup tool (basically comment the classes
in contrib package) and implement those refactor before the version 2 of the
backup tool: there is nothing urgent, nor imperative to them. The updates
would be transparent for everyone.


This seems a bit overkill for just the version import. Could the
> importer be refactored so that it could work with a smaller interface
> than the full UpdatableStateManager?


No, I tried this before :) Well it is possible, but not easily done. I
couldn't use the BatchedItemOps. Any idea?


I'm a bit concerned about this, the version manager should be private
> to the repository. I'd rather see something like a more focused
> WorkspaceImpl.getVersionHistoryImporter() method that only works when
> you have administrator access.


I don't understand your concern. Basically it is pretty much the same as
session.getVersionManager() (already public) but I am sure I can cast it to
VersionManagerImpl whereas a Session.getVersionManager might be a
VersionManagerImpl or a XAVersionManagerImpl. To me it is just a convenience
method.

The other solution was to add a method to VersionManager interface and one
method to XAVersionManagerImpl and VersionManagerImpl but since we want to
minimize change, my approach was best and since you would access through a
session the versionManager, there was no real concern about putting it
public.

BR
Nico
my blog! http://www.deviant-abstraction.net !!

Re: Refactoring of the backupTool

[Jukka Zitting <ju...@gmail.com>]

Hi,

On 9/4/06, Nicolas <nt...@gmail.com> wrote:
> I am following our IM chat where you explained me a few things I didn't
> understand at first. I will summarize it here on the ML.

Thanks!

> - Actually between RestoreBatchedItemOperations and BatchedItemOperations, I
> only have deleted the "recursively add 'auto-create' child nodes defined in
> node type" part. This way, I only create node. This way we only create node
> and so don't check for protection (since a node to be created is not yet
> protected :p). I could add some composition to this, but only to this part
> seems overkill. I don't really see what I can do on this. What do you think?

If it's just a single feature then adding a flag for
enabling/disabling it could also solve the need.

> - NodeVersionHistoriesUpdatableStateManager and UpdatableStateItemManager.
> Actually, I created one new UISM because I couldn't understand how the
> EventStateCollectionFactory worked (I tried null and to instantiate one but
> it didn't work), therefore couldn't reuse the LocalItemStateManager. If
> someone can explain it to me, I might be able to delete this class.

The ESCF is used for sending observation events. For example the
VersionManagerImpl contains the escFactory instance that you should be
able to use.

BR,

Jukka Zitting

-- 
Yukatan - http://yukatan.fi/ - info@yukatan.fi
Software craftsmanship, JCR consulting, and Java development

Re: Refactoring of the backupTool

[Nicolas <nt...@gmail.com>]

Hi Jukka,

I am following our IM chat where you explained me a few things I didn't
understand at first. I will summarize it here on the ML.

- The getVersionManager in SessionImpl shouldn't be public and will probably
be switched to protected in a later refactoring. Therefore we should create
a getVersionHistoryImporter method in SessionImpl

-  WorkspaceImporter and SysViewImporter should be refactored to avoid
duplicate code. The best way is to use composition with a base class and a
Checkers class called (with different switches to activate different
checks).

I have yet two issues pending (I know we discussed those this morning but I
have new informations):

- Actually between RestoreBatchedItemOperations and BatchedItemOperations, I
only have deleted the "recursively add 'auto-create' child nodes defined in
node type" part. This way, I only create node. This way we only create node
and so don't check for protection (since a node to be created is not yet
protected :p). I could add some composition to this, but only to this part
seems overkill. I don't really see what I can do on this. What do you think?


- NodeVersionHistoriesUpdatableStateManager and UpdatableStateItemManager.
Actually, I created one new UISM because I couldn't understand how the
EventStateCollectionFactory worked (I tried null and to instantiate one but
it didn't work), therefore couldn't reuse the LocalItemStateManager. If
someone can explain it to me, I might be able to delete this class.

I am waiting for your feeback before implementing all changes.

BR,
Nicolas