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 <[email protected]>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? > > > > > >
