Yes, sounds reasonable. I will create a branch.
- Florian
On 10/11/2010 19:41, Florent Guillaume wrote:
Could you do it in a branch for a few days?
I have a fix to do for the AtomPub bindings (they generate bad atom
alternate links for renditions), and I'll need it for the Nuxeo server
bindings shortly. I'd rather not change the updateProperties / save
that I may be using right now in our connector's code. In a 1-2 days
I'll commit and make a private snapshot and we can merge your branch.
Is that ok?
Florent
On Wed, Nov 10, 2010 at 8:35 PM, Florian Müller
<florian.muel...@alfresco.com> wrote:
Got it. Thanks.
Would somebody object if I would start the refactoring?
If we don't like it, we can roll it back.
I would also remove the "persistent" and "transient" notion from the session
implementation including renaming those classes.
- Florian
On 10/11/2010 17:43, Florent Guillaume wrote:
It would be an interface.
I agree that it's not obvious in this case that transient behavior can
be obtained using an adapter (and autocompletion doc.get... does not
immediately provide it). But this can be solved by simple
documentation I think. Yes if we think transient documents are
fundamental we can make createTransientDocument a synonym to get this
adapter. It's not mandatory though.
Florent
On Wed, Nov 10, 2010 at 3:16 PM, Florian Müller
<florian.muel...@alfresco.com> wrote:
Hi Florent,
Could you elaborate a bit more on that proposal?
Would TransientDocument.class be an Interface or a Class?
I like the idea of adapters. But it is not intuitive that there is an
adapter the provides a transient paradigm. Would it make sense to add
another method that provides a shortcut to the "transient adapter"?
Thanks,
Florian
On 10/11/2010 13:27, Florent Guillaume wrote:
I've come around to agreeing that 2. is probably better.
I can see either:
TransientDocument transDoc = doc.createTransientDocument():
or
TransientDocument transDoc = doc.getAdapter(TransientDocument.class);
The latter is more extensible for other uses besides transience
(protocol extensions, application-specific adapters).
WDYT?
Florent
On Wed, Nov 10, 2010 at 10:41 AM, Florian Müller
<florian.muel...@alfresco.com> wrote:
I would like to see that sorted out soon. There are more and more users
of OpenCMIS and API changes are usually painful for them. We should
also
consider a new release (0.2.0) when this is done.
So, where are we?
The main question is where we want to handle transient data. We have
two
opinions:
1. Transient and non-transient data access should be covered by the
CmisObject interface. (Florent)
2. Transient data access should be covered by a new interface and
non-transient data access should be covered by the CmisObject
interface.
(Florian)
Personally, I still want to go for option 2 since it provides cleaner
semantics -- especially in multi-threaded environments.
Other opinions?
- Florian
On 08/11/2010 11:51, David Caruana wrote:
Have we reached a conclusion on this issue? It's been a while since we
last discussed.
Regards,
Dave
On 22 Oct 2010, at 15:06, Florian Müller wrote:
Oh, right, there is yet another option. :)
I support all your arguments against b), c) and d). But there are
also
two issues with e).
1. Which value wins if a property is changed in the transient object
_and_ present in the map?
2. save() stores the whole object. If we make ACLs and Policies
transient too (do we want that?) than updateProperties() has the side
effect
of storing those as well.
Which leaves us with a) which is pretty strong. And in this case I
prefer two interfaces because it much is easier to understand and
provides
more compile checks and less runtime surprises.
- Florian
On 22/10/2010 14:44, Florent Guillaume wrote:
On Fri, Oct 22, 2010 at 3:33 PM, Florian Müller
<florian.muel...@alfresco.com> wrote:
So your proposal is to add a method getTransientObject() (or
something like that) to CmisObject. And that method would return a
transient, not thread-safe version of the object but with
setProperty(),
save(), etc. enabled?
Is that correct?
Yes.
In this case we should also add a method isTransient() to
CmisObject.
There should be a way to discover the state of the CmisObject.
Yes that would be useful.
I suggest that if the object is not transient, the transient
methods
should throw an IllegalStateException.
Ok.
I agree that keeping the number of interfaces and classes low helps
to learn and understand an API. But blending two semantics in one
class
could be even more confusing.
For example, what should happen if updateProperties(Map) is called
on
a transient object? There are multiple options:
a) updateProperties(Map) throws an exception because it only
works
for non-transient objects.
b) updateProperties(Map) writes instantly to the repository and
the transient object is refreshed.
(which btw means that transient changes are lost)
c) updateProperties(Map) writes instantly to the repository and
but object keep its (now outdated) state.
d) The property map is merged with the transient properties and
written when save() is called.
And also:
e) The property map is merged with the transient properties and then
save() is automatically called.
It is not obvious. All semantics would be right and wrong at the
same
time.
Agreed, we have to make a choice. But we would make a similar choice
when deciding what methods would be available on the
TransientDocument
interface and what they would do.
But anyway my choices in order of preference would be e) then a).
I don't like b) because it loses changes.
I don't like c) because there's an issue with not keeping the
changes
in order.
I don't like d) because if you didn't do any transient operation it
doesn't behave like its non-transient counterpart.
Florent
On 22/10/2010 13:43, Florent Guillaume wrote:
Hi,
Looking back at your example:
Document doc = (Document) session.getObject(id);
TransientDocument transDoc = doc.createTransientDocument():
transDoc.setProperty("prop1", "value1");
transDoc.setProperty("prop2", "value2");
transDoc.save();
I agree with the need to explicitly getting a transient object if
you
need different semantics.
One thing that will be painful though is having different classes:
TransientCmisObject, TransientDocument, TransientFolder, etc.
Couldn't we simply have all the methods available on the
CmisObject,
Document, Folder, etc. and make them throw
UnsupportedOperationException when called on a non-transient
object?
This would tremendously help learning about the API and avoid
juggling
with many different interfaces for the users.
Florent
On Mon, Oct 18, 2010 at 10:43 PM, Florian Müller
<florian.muel...@alfresco.com> wrote:
Hi Florent,
Well, no, there would be no CmisObject.save() method. The save()
method
would only be on the transient object.
CmisObject would have updateProperties(Map), applyAcl(List<Ace>,
List<Ace>,
AclPropagation), applyPolicy(ObjectId), etc. And all of them
would
write
instantly to the repository and refresh the object.
Florian
On 18/10/2010 17:49, Florent Guillaume wrote:
Ok understood and agreed. There would be a new CmisObject.save()
to
replace updateProperties() then? That works for me as it's a
less
restrictive name that allows for different implementations of
the
transient space that don't deal with just properties.
Regarding your examples I don't need to do all that, because in
the
Nuxeo use cases I described the client Session I'm providing to
the
user of the API is not a PersistentSessionImpl but a completely
new
Nuxeo class that does all the wrapping it needs around native
Nuxeo
objects. So I'm not constrained by the semantics of the current
PersistentSessionImpl. It's just that I need a save()-like API
to
exist on the CmisObject. And of course I want to have semantics
that
are not too far to what's available when you use an actual
remote
connection using pure OpenCMIS PersistentSessionImpl.
Florent
On Mon, Oct 18, 2010 at 6:06 PM, Florian Müller
<florian.muel...@alfresco.com> wrote:
Hi Florent,
We don't want to remove the transient space entirely. The
proposal is to
detach the transient from the non-transient part. We would have
two
separate
objects.
The non-transient object is always consistent, can be shared
across
threads
and is cached. Changes are directly written to the repository.
This
object
would have an updateProperties(Map) method that immediately
refreshes the
object after the update.
The transient object is owned by one thread and not thread
safe.
That
should
prevent inconsistent views on the object. This object would
have
setProperty() and save() methods. Internally it would use the
non-transient
object to access all unchanged data. It's a wrapper around the
non-transient
object that holds the transient data.
So you still could use the pattern that you are using today.
The
only
difference would be that you have to create a transient wrapper
object.
That could look like this:
Document doc = (Document) session.getObject(id);
TransientDocument transDoc = new TransientDocument(doc);
transDoc.setProperty("prop1", "value1");
transDoc.setProperty("prop2", "value2");
transDoc.save(); // that also refreshes the underlying
non-transient
object
Or maybe we do something like this:
Document doc = (Document) session.getObject(id);
TransientDocument transDoc = doc.createTransientDocument():
transDoc.setProperty("prop1", "value1");
transDoc.setProperty("prop2", "value2");
transDoc.save();
Florian
On 18/10/2010 16:07, Florent Guillaume wrote:
Hi,
In Nuxeo the OpenCMIS client API is made available locally as
another
API to manipulate documents, in addition to the Nuxeo native
APIs.
I have a problem with removing CmisObject.updateProperties()
and
the
mini transient space because for me it's quite useful. The
Nuxeo
internal non-CMIS session has a notion of a property-only
transient
space, so in Nuxeo you update several properties and do some
kind of
object save() to flush them. Without a similar flushing
concept
on the
OpenCMIS client API, I'm obliged to make a flush on every
property
write, which leads to severely degraded performance.
If all method calls on a CmisObject immediately write
everything
through the network and potentially refetch a full object
there
will
be many people that aren't happy with CMIS performance once
they
get
to use OpenCMIS. The client API is supposed to provide some
convenience to the user, and having a mini transient space for
properties is IMHO the very first step of convenience. There
may
be
problems with the semantics of interactions between this
transient
space and caching / refetches, but let's solve them rather
than
remove
them.
Dave wrote:
For example, with a transient space, what is the behaviour
when
setProperty has been called and an "update" method is then
called prior
to
updateProperties. Are the transient changes discarded,
flushed,
or just
left
as is?
So if I understand correctly you're talking about the case:
1. doc.setPropertyValue("foo", ...);
2. doc.updateProperties(map);
3. doc.setPropertyValue("bar", ...);
4. doc.updateProperties();
And you're asking if the "foo" change is sent with 2. or with
4.
or
discarded? I'd say let's keep this undefined, as I feel that
it's a
use pattern that's not natural. If we really want to specify
this then
I have no problem mandating that the "foo" change should be
sent
with
2. by saying that updateProperties(map) is the same as
setPropertyValue() on all the properties of the map then
calling
updateProperties() with the transient map.
Florent
On Mon, Oct 18, 2010 at 4:05 PM, Klevenz, Stephan
<stephan.klev...@sap.com> wrote:
Hi,
Coming back to the cache discussion. I would like to support
this
proposal by Dave/Florian and think we can also delete Methods
on
Session
class like cancel() and save() which are currently not
implemented.
+1 for this:
- All write operations provided by CmisObject should
automatically do
an
object
refresh after the update. That guarantees that the object is
always
consistent.
The cost for this consistency is an additional call to the
repository.
- Is some cases you don't need or want this addition cost.
Lets say
you
just
want to update a bunch of objects but you don't work with
them
afterwards.
That's what the operations provided by Session are good for.
They just
do
that and nothing else.
About the transient support we can design this as an optional
add on
with
additional interfaces and additional implementations. With a
clear
separation the API become easier to use.
Regards,
Stephan
