checkins to jcr-server

[Angela Schreiber <an...@day.com>]

hi brian

congratulations to you first commit(s) :)

as you may have noticed i opened a jira issue
some time ago about the jdom, which is heavily
used in the jcr-server but got removed from
the jackrabbit core, due to backwards incompatibility
issues within jdom causing problems.
i opened an jira bug that should address this
within the jcr-server contrib:

JCR-258 Jcr-Server Contrib: Remove JDOM dependencies

i'm currently working on this and you may figure
out that i will cause major changes to the jcr-server.

thus, i'd like to ask you be very conservative in
changing the jcr-server in order to avoid me
struggling with conflicts.

thanks for your comprehension.

angela

btw: i don't think its ok to make just private methods
protected in the DavResource. there was a reason making
them private. i would like to know, why you need them
to be protected. thanks.

Re: checkins to jcr-server

[Angela Schreiber <an...@day.com>]

hi brian

hm. it seems that either i can't express myself
clearly or we just have quite different approaches
while writing programs.

Brian Moseley wrote:
> Angela Schreiber wrote:

>> this is the point about private methods from my point of
>> view. since it's an opensource project, i will
>> not know (and i should not know) about subclasses. but
>> i must pay attention and respect potential subclasses,
>> if a mark a method protected.

> well yeah. there is a happy medium. for a method where you don't have a 
> strong reason to keep it private, make it protected and let subclassers 
> decide whether or not they will use it.

it seems that you define the happy medium :)

i regret to say: i don't agree with your statements.
i don't agree that a method should be protected just for
the potential case that someone could find it useful. i think
it should be the other way round.

maybe for a similar reason, i avoid creating
additional, implementation specific 'public' methods,
that are not defined by the interface(s). that's why
i did not agree with your modification in the resource-
factory where you added extra public getter and setter
methods for private fields that have nothing to do
with the job of the factory (creating resources).

hm... i get the impression that we have some fundamental
disagreement. what a pitty.

have a nice weekend!
kind regards

angela

Re: checkins to jcr-server

[Brian Moseley <bc...@osafoundation.org>]

Angela Schreiber wrote:

> on the other hand there is are a couple of methods, that
> are just there for improving code readability ('getDavName'),
> others that i think contain really internal stuff such
> as 'isLocked', 'isFilteredItem' or 'isJsrLockable'.

let's say i'm implementing a dav extension that defines a new http 
method. my new method is sort of like DavResource.addMember but not 
quite the same, and i can't cleanly override or otherwise reuse 
DavResourceImpl.addMember, so i have to add a method that looks a lot 
like it. suddenly i need access to isLocked, potentially isFilteredItem, 
etc.

this is exactly what happened when i needed to implement MKCALENDAR for 
caldav. of course, with the latest revision of jcr-server, the 
import/export facility meets my needs and i don't need a specialized 
method resource method anymore. but what's to say some sort of situation 
like this couldn't happen again?

> this is the point about private methods from my point of
> view. since it's an opensource project, i will
> not know (and i should not know) about subclasses. but
> i must pay attention and respect potential subclasses,
> if a mark a method protected.

well yeah. there is a happy medium. for a method where you don't have a 
strong reason to keep it private, make it protected and let subclassers 
decide whether or not they will use it.

> just one more example: the 'setIsCollection' method in the 
> DavResourceImpl. this one is a hack from my point of view.
> since i wanted to complete my modifications i left it...
> but i don't like it a all and i would not want this to
> be protected.

it's totally a hack, but one that is required since we're using the same 
class to model resources that already exist in the repository as well as 
ones that are being created in the current request. you'll see i had to 
do the same thing in CosmoDavResourceImpl, with a setIsCalendarCollection.

i have briefly considered making a new implementation of DavResource for 
resources which are in the process of being created, but i haven't 
looked at specifically how to do it yet.

Re: checkins to jcr-server

[Angela Schreiber <an...@day.com>]

hi brian

thanks for your explanation.

Brian Moseley wrote:

> <http://svn.osafoundation.org/server/cosmo/branches/0.2/src/main/java/org/osaf/cosmo/dav/impl/CosmoDavResourceImpl.java> 

> however, i also have to override initProperties in order to initialize 
> the resource's caldav properties. this is an example of a jcr-server 
> method that wasn't designed with subclassing in mind.

the initProperties is indeed a case where i could see a benefit of
the protected access modifier and you may noticed, that i left it 
protected :).

> the fact is, DavResourceImpl is almost perfectly usable for me (it was 
> less so in the past, but it's much better now after the latest 
> refactoring).

glad to hear

> if we keep 
> extensibility as a primary goal for the simple server, then i'll be able 
> to reuse your fine work instead of having to copy it and tweaking little 
> bits here and there.

don't get me wrong, i'm not against extensibility and
i'm happy if you implementation is useful for you
project.

but it feels awkward to me, if all private methods of a
implementation need to be protected... i could never
express things like jukka did, but i agreed.

there are a few non-interface methods that are protected,
because they cover things that are really implementation
specific (nothing to do with Dav-Library in general), but
on the other ask for being used by subclasses:

- getExportContext
- getImportContext
- getNode

on the other hand there is are a couple of methods, that
are just there for improving code readability ('getDavName'),
others that i think contain really internal stuff such
as 'isLocked', 'isFilteredItem' or 'isJsrLockable'.

sometimes i even tend to make private methods for code
that i don't feel totally happy with. i want to encapsulate
it into a separate method to be able to look at this
problem separately. quite often i find those method containing
bugs and sometimes they need to be replaced, if i finally
found a solution that seems better to me. the 'isLocked'
is such a candidate.

this is the point about private methods from my point of
view. since it's an opensource project, i will
not know (and i should not know) about subclasses. but
i must pay attention and respect potential subclasses,
if a mark a method protected.

for methods i can't give this guarantee (for the reasons
mentioned above), i think it would not be ok.

just one more example: the 'setIsCollection' method in the 
DavResourceImpl. this one is a hack from my point of view.
since i wanted to complete my modifications i left it...
but i don't like it a all and i would not want this to
be protected.

hope you understand, what i'm trying to say.
kind regards
angela

Re: checkins to jcr-server

[Brian Moseley <bc...@osafoundation.org>]

Angela Schreiber wrote:

> perhabs we have a little disagreement about the aim of
> the webdav implementations present in the jcr-contrib.

probably :)

i'm attempting to use the "simple impl" as a framework for a 
full class 1 and 2 webdav server with all headers, 
properties, etc defined in rfc 2518.

i know that you're not aiming quite that high, but still, we 
should be able to meet both of our goals with the same codebase.

> the implementations of the webdav-library present in the 'server'
> project are not meant to be 'abstract concrete implementations'.
> there are 2 separate, concrete implementations of the library
> interfaces, each for a specific aim: the 'simple-server' and the 
> 'remoting-server'.
> 
> that's why you find 2 implementions of the DavResource interface,
> its factory etc. i tried to design the interfaces in a way that
> its suitable for any kind of implementations, not only the those
> that are available as open-source code.
> 
> if the implemenation you are using for your Cosmo project (which
> also uses CalDAV if i'm not mistaken), needs all the private
> stuff of the simple/DavResourceImpl a little different, then
> it would maybe wise reconsidering the class inheritance.
> is there something specific the library (thus the interfaces
> are missing), that forces you to subclass a specific implementation
> that obviously does not meant your needs?

"obviously does not meet your needs" dramatically overstates 
the problem. let me show you in detail:

<http://svn.osafoundation.org/server/cosmo/branches/0.2/src/main/java/org/osaf/cosmo/dav/impl/CosmoDavResourceImpl.java>

CosmoDavResourceImpl subclasses DavResourceImpl. you can see 
that i'm only overriding two of the methods defined by 
DavResource and implemented by DavResourceImpl, in order to 
add caldav-specific info to the compliance class and list of 
supported methods.

however, i also have to override initProperties in order to 
initialize the resource's caldav properties. this is an 
example of a jcr-server method that wasn't designed with 
subclassing in mind. i have to keep my own state variable in 
order to keep this method from running more than once, just 
like the DavResourceImpl's initProperties method.

the fact is, DavResourceImpl is almost perfectly usable for 
me (it was less so in the past, but it's much better now 
after the latest refactoring). there's absolutely no way i 
want to go writing my own implementations of all of the 
jcr-server interfaces, because what's there is good enough 
for me nearly all of the time. if we keep extensibility as a 
primary goal for the simple server, then i'll be able to 
reuse your fine work instead of having to copy it and 
tweaking little bits here and there.

> so, the reason would probably be, that i think the 'simple'
> server is a concrete implemenation and not an abstract one.

i don't see how that justifies keeping some of 
DavResourceImpl's methods private, unless you are 
specifically trying to discourage their use by subclasses 
for some reason, or even trying to discourage subclassing 
altogether. if so, we need to talk more about that specifically.

Re: checkins to jcr-server

[Angela Schreiber <an...@day.com>]

hi brian

perhabs we have a little disagreement about the aim of
the webdav implementations present in the jcr-contrib.

i was thinking about this tonight and maybe i have to
write down some of my fundamental considerations, while
writing the jcr-server.

since this will take longer than just a few minutes,
i'd like to send but a short reply to one thing that i think gets
pretty close to our misunderstanding:

Brian Moseley wrote:
> Jukka Zitting wrote:
> 
>> I'm not so familiar with the jcr-server design that I can comment on 
>> design decisions there, but in general i disagree with the above 
>> principle. :-)
>>
>> A method should be private if there is no specific reason otherwise. A 
>> non-private method is a part of the external API contract of a class 
>> and  carries with it all the burden of API compatibility. The narrower 
>> the API contract of a class is the easier the class is to restructure 
>> to match an evolving environment. A narrower API is also easier to 
>> document and maintain. In general it is much easier to relax the 
>> visibility of a method when needed than to restrict the availability 
>> of a non-private method.

> ok, in general i agree with you, so let me refine the principle: for 
> jcr-server framework classes, especially but not limited to those which 
> are abstract concrete implementations of interfaces, we should favor 
> protected access for methods over private unless it can be shown that a 
> subclass should by design never have any interest in the method.

the implementations of the webdav-library present in the 'server'
project are not meant to be 'abstract concrete implementations'.
there are 2 separate, concrete implementations of the library
interfaces, each for a specific aim: the 'simple-server' and the 
'remoting-server'.

that's why you find 2 implementions of the DavResource interface,
its factory etc. i tried to design the interfaces in a way that
its suitable for any kind of implementations, not only the those
that are available as open-source code.

if the implemenation you are using for your Cosmo project (which
also uses CalDAV if i'm not mistaken), needs all the private
stuff of the simple/DavResourceImpl a little different, then
it would maybe wise reconsidering the class inheritance.
is there something specific the library (thus the interfaces
are missing), that forces you to subclass a specific implementation
that obviously does not meant your needs?

> angela mentioned that there was a specific reason that she wanted 
> certain CosmoDavResourceImpl methods private. i hope she'll share that 
> reason. 

oh... i always to to share my reasons :)) even if i'm struggling
finding the proper words in english.

so, the reason would probably be, that i think the 'simple'
server is a concrete implemenation and not an abstract one.

hope, this solves some of the points.

kind regards
angela

Re: checkins to jcr-server

[Brian Moseley <bc...@osafoundation.org>]

Jukka Zitting wrote:

> I'm not so familiar with the jcr-server design that I can comment on 
> design decisions there, but in general i disagree with the above 
> principle. :-)
> 
> A method should be private if there is no specific reason otherwise. A 
> non-private method is a part of the external API contract of a class and 
>  carries with it all the burden of API compatibility. The narrower the 
> API contract of a class is the easier the class is to restructure to 
> match an evolving environment. A narrower API is also easier to document 
> and maintain. In general it is much easier to relax the visibility of a 
> method when needed than to restrict the availability of a non-private 
> method.

ok, in general i agree with you, so let me refine the 
principle: for jcr-server framework classes, especially but 
not limited to those which are abstract concrete 
implementations of interfaces, we should favor protected 
access for methods over private unless it can be shown that 
a subclass should by design never have any interest in the 
method.

i think it's safe to say that the designer of a framework 
can't imagine all the possible uses or extensions of it. 
there have been several cases where i've needed to override 
jcr-server superclass methods but doing so has caused me to 
be dependent on other, til-then private superclass methods.

it's certainly true that angela's recent refactoring has 
gotten rid of most if not all of these dependencies from my 
code. that doesn't mean the situation won't occur again in 
the future. following the guideline i propose above will 
make it easier for framework extenders to cope with such 
situations.

angela mentioned that there was a specific reason that she 
wanted certain CosmoDavResourceImpl methods private. i hope 
she'll share that reason. i'm not opposed to making specific 
design decisions that make methods private. i just think 
that, as a general principle, when there is no overriding 
reason to keep a method private, framework classes should 
use protected methods in order to allow full flexibility for 
extension classes.

Re: checkins to jcr-server

[Jukka Zitting <ju...@zitting.name>]

Hi,

Brian Moseley wrote:
> i think the burden of proof should be on the person who wants to keep 
> methods private ;)

I'm not so familiar with the jcr-server design that I can comment on 
design decisions there, but in general i disagree with the above 
principle. :-)

A method should be private if there is no specific reason otherwise. A 
non-private method is a part of the external API contract of a class and 
  carries with it all the burden of API compatibility. The narrower the 
API contract of a class is the easier the class is to restructure to 
match an evolving environment. A narrower API is also easier to document 
and maintain. In general it is much easier to relax the visibility of a 
method when needed than to restrict the availability of a non-private 
method.

BR,

Jukka Zitting

Re: checkins to jcr-server

[Brian Moseley <bc...@osafoundation.org>]

Angela Schreiber wrote:

> congratulations to you first commit(s) :)

thanks!

> thus, i'd like to ask you be very conservative in
> changing the jcr-server in order to avoid me
> struggling with conflicts.

no problem. i'm looking forward to seeing jdom go away. i've 
got a bunch of other changes queued up in the osaf svn 
repository, but i'll wait to address them until you've 
integrated these changes.

> btw: i don't think its ok to make just private methods
> protected in the DavResource. there was a reason making
> them private. i would like to know, why you need them
> to be protected. thanks.

i have a subclass of DavResource that in past revisions has 
needed to access those methods. see 
<http://svn.osafoundation.org/server/cosmo/branches/0.2/src/main/java/org/osaf/cosmo/dav/impl/CosmoDavResourceImpl.java>.

since integrating your last major refactoring, i've had to 
override fewer of the public DavResourceImpl methods, and 
consequently i currently need access to fewer of the 
private/protected ones.

however, in general with jcr-server, i think the default 
approach should be to make all non-public, non-interface 
methods protected unless there is a strong reason for 
keeping them private.

with my caldav and ticket extensions i've found a need to 
customize many jcr-server classes, and this usually means 
accessing methods of concrete superclasses that aren't 
defined by interfaces.

i think the burden of proof should be on the person who 
wants to keep methods private ;)