On Wed, 2015-06-10 at 10:41 +0100, Ian Boston wrote:
> Hi,
>
> On 10 June 2015 at 09:41, Robert Munteanu <[email protected]> wrote:
>
> > On Tue, 2015-06-09 at 17:01 +0200, Julian Reschke wrote:
> > > On 2015-06-09 16:41, Ian Boston wrote:
> > > > Hi,
> > > > Should the opaque String key be abstracted into a DocumentKey
> > > > interface so
> > > > that how the key is interpreted, and how it might be associated
> > > > with a
> > > > certain type of storage can be abstracted as well, rather than
> > > > relying on
> > > > some out of band specification of the key to be serialised and
> > > > parsed at
> > > > every transition ?
> > > > Best Regards
> > > > Ian
> > > > ...
> > >
> > > Absolutely.
> > >
> > > That would also be relevant for some some parts of the
> > > MongoDocumentStore implementation which currently make
> > > assumptions
> > > about
> > > ID structure for cache invalidation purposes.
> >
> > If your suggestion aims towards something like
> >
> > @@ -59,7 +59,7 @@ public interface DocumentStore {
> > * @return the document, or null if not found
> > */
> > @CheckForNull
> > - <T extends Document> T find(Collection<T> collection, String
> > key);
> > + <T extends Document> T find(Collection<T> collection,
> > DocumentKey
> > key);
> >
> > /**
> > * Get the document with the {@code key}. The implementation
> > may
> > serve the
> >
> > that would be a very invasive change which will propagate
> > throughout
> > the Oak code base and also break backwards compatibility ( unless
> > we
> > keep a set of deprecated methods alongside ) , neither of which
> > seems
> > very nice.
> >
>
>
> agreed. Very invasive and not very nice, however the interface does
> not
> appear to be exported from the core bundle so deprecation might not
> be
> required. Nothing outside the code-bundle should be binding to it or
> implementing it.
>
> Did I read META-INF/MANIFEST.MF from the built jar correctly ?
Ah, I was under the impression that .plugins.document was exported.
However it's not and this makes the proposed change simpler.
>
> One of the advantages of changing the interface is it will ensure no
> unexpected use of the key in string form as such code won't compile.
Yup
>
> btw, supporting multi tenancy probably does require some exported API
> changes, even if zero downtime does not. I have resorted to making
> those
> changes (in a fork) to see if a PoC can work, rather than avoiding
> the
> changes completely, as it leads to a cleaner solution. Hopefully the
> changes will be acceptable, if the PoC works.
>
>
> > Another possible approach is to have a DocumentKey class ( or
> > alternatively an interface backed by a DocumentKeyFactory class if
> > you
> > prefer ), which can be used as follows
> >
> > DocumentKey docKey = DocumentKey.from(String key);
> > log.info("Path is {}, depth is {}", docKey.getPath(),
> > docKey.getId());
> >
>
>
> That could also work, and does centralise the out of band
> interpretation of
> the key.
Given that changing the API is less invasive I slightly favour adding a
type for the key. However, I still need to align this with the ideas
from OAK-1328
Robert
