You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Justin Edelson <ju...@justinedelson.com> on 2010/12/07 03:52:51 UTC
issue with Jackrabbit upgrade - change in method signature in AccessManager
Hi,
I'm attempting to upgrade Sling to use the current Jackrabbit 2.1.x release
and ran into a fairly significant snag.
In r950440 (as part of JCR-2573),
org.apache.jackrabbit.core.security.AccessManager.canRead() was changed from
a single argument (Path) method to a two-argument method (Path, ItemId). In
o.a.s.jcr.jackrabbit.server.impl.security.PluggableDefaultAccessManager, we
override this method (from o.a.j.core.security.DefaultAccessManager) and
delegate to an implementation of the AccessManagerPlugin if available.
The obviously easy thing to do is to modify AccessManagerPlugin to have a
two-argument canRead() method, but this will both break any existing
implementations of AccessManagerPlugin *and* will require that
o.a.j.core.idbe exported by the jackrabbit-server bundle (there are
probably other
downsides to making this change - these are just the first two I can think
of). On the other hand, I don't think it is quite tenable for us to be stuck
at Jackrabbit 2.1.1 forever, so we may just need to bite this bullet, do a
major version change and roll with it. However, in this case, I'd be
inclined to wait until Jackrabbit 2.2.0 is released.
Changeset is here: http://codereview.appspot.com/3490041
WDYT?
Justin
P.S. Has anyone gotten reviews.apache.org to work?
Re: issue with Jackrabbit upgrade - change in method signature in AccessManager
Posted by Justin Edelson <ju...@justinedelson.com>.
FYI - I added a new patch here: http://codereview.appspot.com/3490041/.
Interface names called Something2 (i.e. AccessManagerPlugin2 in this case)
are not my favorite, but I don't know what else to call it.
Justin
On Tue, Dec 7, 2010 at 8:42 AM, Justin Edelson <ju...@justinedelson.com>wrote:
>
> On Dec 7, 2010, at 4:00 AM, Felix Meschberger wrote:
>
> > Hi,
> >
> > Am Montag, den 06.12.2010, 21:52 -0500 schrieb Justin Edelson:
> >> Hi,
> >> I'm attempting to upgrade Sling to use the current Jackrabbit 2.1.x
> release
> >> and ran into a fairly significant snag.
> >>
> >> In r950440 (as part of JCR-2573),
> >> org.apache.jackrabbit.core.security.AccessManager.canRead() was changed
> from
> >> a single argument (Path) method to a two-argument method (Path, ItemId).
> In
> >> o.a.s.jcr.jackrabbit.server.impl.security.PluggableDefaultAccessManager,
> we
> >> override this method (from o.a.j.core.security.DefaultAccessManager) and
> >> delegate to an implementation of the AccessManagerPlugin if available.
> >
> > Even though Jackrabbit probably considers this internal/private API, it
> > is IMHO not acceptable to have a breaking API change in a micro release
> > (am I understanding this correctly that this happened between 2.1.2 and
> > 2.1.3 ?) ... In fact it is even hard to accept in a minor release.
>
> This happened between 2.1.1 and 2.1.2.
>
> >
> > This issue should be raised with the Jackrabbit project.
> >
>
> Yeah...
>
> > WDYT ?
> >
> >>
> >> The obviously easy thing to do is to modify AccessManagerPlugin to have
> a
> >> two-argument canRead() method, but this will both break any existing
> >> implementations of AccessManagerPlugin *and* will require that
> >> o.a.j.core.idbe exported by the jackrabbit-server bundle (there are
> >> probably other
> >> downsides to making this change - these are just the first two I can
> think
> >> of). On the other hand, I don't think it is quite tenable for us to be
> stuck
> >> at Jackrabbit 2.1.1 forever, so we may just need to bite this bullet, do
> a
> >
> > I am not a fan of exposing Jackrabbit Core packages. But maybe in this
> > case, it might work.
> >
> This is definitely the lesser problem.
>
> > But we must try to not break our own API .... This is what I am weary
> > of.
> >
> > So here is my proposal:
> >
> > * Get back at Jackrabbit to check for the incompatible API change
> > * Keep on supporting the old API (can we ?)
>
> Sure. The problem is that if canRead() is called with a null path and a
> non-null ItemId, an old AccessManagerPlugin won't be invoked.
>
> > * Add a new interface extending the old adding the new method
> >
> > Does this make sense?
>
> We'll see what jackrabbit-dev has to say...
>
> Justin
> >
> > Regards
> > Felix
> >
> >> major version change and roll with it. However, in this case, I'd be
> >> inclined to wait until Jackrabbit 2.2.0 is released.
> >>
> >> Changeset is here: http://codereview.appspot.com/3490041
> >>
> >> WDYT?
> >>
> >> Justin
> >>
> >> P.S. Has anyone gotten reviews.apache.org to work?
> >
> >
>
>
Re: issue with Jackrabbit upgrade - change in method signature in AccessManager
Posted by Justin Edelson <ju...@justinedelson.com>.
On Dec 7, 2010, at 4:00 AM, Felix Meschberger wrote:
> Hi,
>
> Am Montag, den 06.12.2010, 21:52 -0500 schrieb Justin Edelson:
>> Hi,
>> I'm attempting to upgrade Sling to use the current Jackrabbit 2.1.x release
>> and ran into a fairly significant snag.
>>
>> In r950440 (as part of JCR-2573),
>> org.apache.jackrabbit.core.security.AccessManager.canRead() was changed from
>> a single argument (Path) method to a two-argument method (Path, ItemId). In
>> o.a.s.jcr.jackrabbit.server.impl.security.PluggableDefaultAccessManager, we
>> override this method (from o.a.j.core.security.DefaultAccessManager) and
>> delegate to an implementation of the AccessManagerPlugin if available.
>
> Even though Jackrabbit probably considers this internal/private API, it
> is IMHO not acceptable to have a breaking API change in a micro release
> (am I understanding this correctly that this happened between 2.1.2 and
> 2.1.3 ?) ... In fact it is even hard to accept in a minor release.
This happened between 2.1.1 and 2.1.2.
>
> This issue should be raised with the Jackrabbit project.
>
Yeah...
> WDYT ?
>
>>
>> The obviously easy thing to do is to modify AccessManagerPlugin to have a
>> two-argument canRead() method, but this will both break any existing
>> implementations of AccessManagerPlugin *and* will require that
>> o.a.j.core.idbe exported by the jackrabbit-server bundle (there are
>> probably other
>> downsides to making this change - these are just the first two I can think
>> of). On the other hand, I don't think it is quite tenable for us to be stuck
>> at Jackrabbit 2.1.1 forever, so we may just need to bite this bullet, do a
>
> I am not a fan of exposing Jackrabbit Core packages. But maybe in this
> case, it might work.
>
This is definitely the lesser problem.
> But we must try to not break our own API .... This is what I am weary
> of.
>
> So here is my proposal:
>
> * Get back at Jackrabbit to check for the incompatible API change
> * Keep on supporting the old API (can we ?)
Sure. The problem is that if canRead() is called with a null path and a non-null ItemId, an old AccessManagerPlugin won't be invoked.
> * Add a new interface extending the old adding the new method
>
> Does this make sense?
We'll see what jackrabbit-dev has to say...
Justin
>
> Regards
> Felix
>
>> major version change and roll with it. However, in this case, I'd be
>> inclined to wait until Jackrabbit 2.2.0 is released.
>>
>> Changeset is here: http://codereview.appspot.com/3490041
>>
>> WDYT?
>>
>> Justin
>>
>> P.S. Has anyone gotten reviews.apache.org to work?
>
>
Re: issue with Jackrabbit upgrade - change in method signature in
AccessManager
Posted by Felix Meschberger <fm...@gmail.com>.
Hi,
Am Montag, den 06.12.2010, 21:52 -0500 schrieb Justin Edelson:
> Hi,
> I'm attempting to upgrade Sling to use the current Jackrabbit 2.1.x release
> and ran into a fairly significant snag.
>
> In r950440 (as part of JCR-2573),
> org.apache.jackrabbit.core.security.AccessManager.canRead() was changed from
> a single argument (Path) method to a two-argument method (Path, ItemId). In
> o.a.s.jcr.jackrabbit.server.impl.security.PluggableDefaultAccessManager, we
> override this method (from o.a.j.core.security.DefaultAccessManager) and
> delegate to an implementation of the AccessManagerPlugin if available.
Even though Jackrabbit probably considers this internal/private API, it
is IMHO not acceptable to have a breaking API change in a micro release
(am I understanding this correctly that this happened between 2.1.2 and
2.1.3 ?) ... In fact it is even hard to accept in a minor release.
This issue should be raised with the Jackrabbit project.
WDYT ?
>
> The obviously easy thing to do is to modify AccessManagerPlugin to have a
> two-argument canRead() method, but this will both break any existing
> implementations of AccessManagerPlugin *and* will require that
> o.a.j.core.idbe exported by the jackrabbit-server bundle (there are
> probably other
> downsides to making this change - these are just the first two I can think
> of). On the other hand, I don't think it is quite tenable for us to be stuck
> at Jackrabbit 2.1.1 forever, so we may just need to bite this bullet, do a
I am not a fan of exposing Jackrabbit Core packages. But maybe in this
case, it might work.
But we must try to not break our own API .... This is what I am weary
of.
So here is my proposal:
* Get back at Jackrabbit to check for the incompatible API change
* Keep on supporting the old API (can we ?)
* Add a new interface extending the old adding the new method
Does this make sense?
Regards
Felix
> major version change and roll with it. However, in this case, I'd be
> inclined to wait until Jackrabbit 2.2.0 is released.
>
> Changeset is here: http://codereview.appspot.com/3490041
>
> WDYT?
>
> Justin
>
> P.S. Has anyone gotten reviews.apache.org to work?