On 11 December 2012 20:21, Robert Matthews <[email protected]>wrote:
> I'm now running a patched version with this change and all is working > again. > That's good to hear. > > Before you press ahead with changes to the PersistenceManager ... I didn't intend to make any changes, actually. At least, not until we bottom out this thread. > it not just the object stores that need just the identity, another example > is in a URL for a web page showing an object. If that page can be > bookmarked then just the identity is needed, whereas a form that will > operate on an object will be one (hidden) field lighter with the merged > version. I assume that restfulObjects needs to consider the same thing. > Hmm. Actually, I see the OID used within a URL as being opaque from a viewer's standpoint. But what a viewer should be able to rely on is that, when converted from string form back to an Oid object, that that Oid can be used with AdapterManager#adapterFor(oid, ConcurrencyChecking.CHECK) and the persistor would be able to enforce concurrency checking if that was what was required. If concurrency checking is not required (eg for the bookmarks), then the viewer would simply call AdapterManager#adapterFor(oid, ConcurrencyChecking.NO_CHECK). Somewhat related - and associated with this concept of an OID's string form being opaque - I do see it as a responsibility of a viewer to encrypt the OID. Otherwise the system becomes hackable... a malicious end-user can type in random URLs trying to uncover information that they otherwise wouldn't have access to. Having said that, I'm not sure that any of our viewers do this or do this well enough. But back to the main point: I don't think that we should be stripping out version information from OIDs in the viewers. The whole point of putting encoding the information in the OID is so that the viewer doesn't need to separately track when a particular object was rendered to a user. Let's keep talking on this thread; we obviously need to get to a common understanding on this important topic. Dan > Rob > > > On 12/07/12 11:36, Dan Haywood wrote: > >> On 7 December 2012 09:43, Robert Matthews <[email protected]>** >> wrote: >> >> I think I'm presuming that the enstring mechanism was there first and was >>> doing the work that use to be done with the identifiers in the viewers >>> and >>> elsewhere, but this change and your centralizing of the optimistic lock >>> checking came along together. I guess when you replaced the uses of the >>> old OIDs you or I just used the enstring method globally. Seaching >>> through >>> there are number of places where the non-versioned one are actually >>> needed, >>> including the SQL Store. So we just need to finish that off. I'll try >>> that >>> later today as I need to get objects persisting in my application. >>> >>> OK, that's a good point. The RootOidDefault class has an >> "enStringNoVersion()" method that could be used. >> >> Or - and perhaps more correct - the PersistenceManager could "strip out" >> the version information whenever it passes an oid to an objectstore. This >> could perhaps be through a new RootOidDefault#stripVersion() method. It >> ought to return a new instance of RootOidDefault, though ... oids are >> immutable. >> >> Dan >> >> PS: as a by-the-by, oids also have value semantics, hence implement equals >> and hashCode. Note that the version information is NOT part of the >> equality check... this is by design. >> >> >> >> >> Rob >>> >>> >>> >>> >>> On 12/07/12 08:33, Dan Haywood wrote: >>> >>> On 6 December 2012 18:57, Robert Matthews <[email protected] >>>> >** >>>> wrote: >>>> >>>> Dan >>>> >>>>> Can you clarify the changes to OID encoding and decoding to and from >>>>> Strings. I see that you have allowed the version and user to be >>>>> included, >>>>> which could be useful in certain circumstances. >>>>> >>>>> Yup, it enables the PersistenceSession/****AdapterManager to be >>>>> able to >>>>> >>>> do >>>> optimistic locking on behalf of all viewers. >>>> >>>> Previously this was a responsibility of every viewer; each viewer impl >>>> had >>>> to somehow hold a cache of OIDs against versions and detect if the >>>> object >>>> returned by the PersistenceSession had changed compared to "last time". >>>> This was complicated by the fact that different objectstores had >>>> different >>>> Oid implementations, and not every viewer could be guaranteed to work >>>> against every object store. As you'll recall, our Oid implementations >>>> are >>>> now fixed: RootOidDefault, AggregatedOid, CollectionOid; these all >>>> support >>>> serialization into strings (useful for all viewers, in particular for >>>> the >>>> restfulobjects viewer). >>>> >>>> When I was working on the wicket viewer - which up until that point >>>> didn't >>>> have any optimistic locking support - I decided to move this >>>> responsibility >>>> for optimistic locking checking into the PersistenceSession. This tied >>>> in >>>> with the simplification of the Oid into a single implementation, so that >>>> the responsibility of object stores is merely to generate IDs. (Again >>>> as >>>> you'll recall, the old OidGenerator interface has been replaced by >>>> IdentifierGenerator) >>>> >>>> The last thing to state is that the PersistenceSession/**** >>>> AdapterManager >>>> >>>> API >>>> to retrieve objects has been overloaded so that a viewer can optionally >>>> specify whether an optimistic locking check should be performed. This >>>> is >>>> in AdapterManager API: >>>> >>>> ObjectAdapter adapterFor(TypedOid oid); >>>> ObjectAdapter adapterFor(TypedOid oid, ConcurrencyChecking >>>> concurrencyChecking); >>>> >>>> The first of these delegates to the second, with >>>> ConcurrencyChecking.NO_CHECK. So for viewers (like HTML viewer and, >>>> perhaps? Scimpi) that still do their own optimistic locking, it should >>>> be >>>> easy enough to replace the calls to adapterFor(oid) with adapterFor(oid, >>>> ConcurrencyChecking.CHECK) wherever it is required. >>>> >>>> >>>> So, to summarize: >>>> - object stores are responsible for persisting objects and identifying >>>> those objects with an Oid >>>> - the PersistenceSession supplements those Oids with version info for >>>> optimistic locking >>>> - the viewers, when calling into the PersistenceSession, can use the >>>> correct version of adapterFor(...) to indicate whether or not they wish >>>> for >>>> an optimistic locking check to be performed. >>>> >>>> >>>> >>>> >>>> >>>> >>>> This breaks some things though, or that extra could at least be >>>> >>>>> redundant. >>>>> I guess the real question is why this is the default behaviour rather >>>>> than >>>>> a special behaviour. >>>>> >>>>> I don't think it does break things; or at least, if there are any >>>>> >>>> viewers >>>> that are parsing an oid string, then they shouldn't. >>>> >>>> The default behaviour of the system is still NOT to perform optimistic >>>> locking, unless asked. >>>> >>>> >>>> >>>> >>>> As an example, my data show this >>>> >>>>> "inUseBy" : "CUS:28^8:admin:1354818065713" >>>>> >>>>> where before it would be this >>>>> >>>>> "inUseBy" : "CUS:28" >>>>> >>>>> >>>>> I think this new form is the special case and not the normal case. That >>>>> is, I think any existing code work the way it has and any code that >>>>> wants >>>>> to explicitly include the other details should make that change. >>>>> >>>>> OK, so I disagree... To use our new vocabulary, I've moved the >>>>> >>>> responsibility for optimistic locking into core (PersistenceSession) >>>> rather >>>> than in the components (viewers). This seems absolutely correct to >>>> me... >>>> it is a really useful bit of core functionality. It also gives >>>> PersistenceSession a raison d'etre: it does more than just wrap an >>>> ObjectStore impl, it also takes care of lifecycle management of the >>>> adapters, in particular with respect to optimistic locking. >>>> >>>> Dan >>>> >>>> >>>> >>>> Regards >>>> >>>>> Rob >>>>> >>>>> >>>>> >
