james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peter M. Goldstein" <peter_m_goldst...@yahoo.com>
Subject RE: [PATCH] NNTP Server fixes
Date Wed, 02 Oct 2002 00:21:09 GMT

Harmeet,

> bugs:
> Good stuff about the bugs. Group command was a miss and LIST seems to
have
> been a really bad one. It was half correct half wrong, but the net
effect
> was all wrong. The strange thing is that I have tested with Outlook,
> Netsacpe and KClient and still it did not show up. Others tested with
news
> group clients and it did not show either.

IMHO, there is very little use in testing protocol servers with generic
clients without doing lower level protocol testing first.  Clients are
always designed to be robust in the face of misbehaving servers.  You
start by getting the protocol exactly right (usually by automated tests
or by simple telnet sessions) and then you test with common clients to
find any gotchas - either errors in your implementation or client
quirks.  I'm not surprised that the clients missed these issues.
 
> authservice:
> You are right about the AuthService. Should never have been a service,
> however I think it is a useful abstraction. Would it be possible to
have
> an
> AuthServiceFactory as a service instead of AuthService. UsersStore
etc.
> are
> ideally not a concern of the Handler, but a concern of AuthService. It
> should be easy fix AuthService this way and still have the advantages
of
> the
> abstraction.

IMHO the AuthService is not worth fixing, for all the reasons I laid out
in my previous email.  I have very strong feelings on this matter - I
have an extensive background in authentication and authorization - and
really feel that the AuthService should be retired.  Designing and
implementing a pluggable authentication/authorization framework is not
trivial.  Moreover there are a number of generally accepted tools (JAAS)
and others under development (JSR 115) that could be relevant.

This needs to be designed properly, not patched together at the end of a
release cycle to fix something that didn't work.  And considering that
this bug has been languishing for nine months or so, I don't think there
has been any extensive exercise of this code.  It can wait until next
version.

> comments:
> prefer the older style more. We can define our own coding standard if
you
> prefer that. Copying a Jakarta coding style if that involves cut paste
> comments from base classes seems less than good. @see is the lesser of
two
> evils if there is no agreement. Let us reduce comments, unless they
are
> really helpful. A comment like '// see section <...>' is far more
helpful
> to
> me than say javadoc style comments on a private variable. Too much
chaff.
> I prefer 'C: ' and 'S: ' to indicate the client and server side, it
seems
> more standard and is less verbose.

Just as Danny feels strongly about some elements of the Jakarta project,
this is one I feel we must hold to.  Javadoc is one of the real
strengths of the Java language, and I want to take full advantage of it.
As I mentioned before, I have no objection to the use of @see tags to
reduce the size of the comments - must they must be present.  I also
believe that focusing on the comments for overridden/inherited methods
misses the big picture - that's not even close to the majority of
methods commented in this or in other documentation changes.  I've
already expressed the reasons why I believe that extensive commenting is
essential to a healthy open source project, so I won't repeat them here.
Suffice to say, I'm very happy with the improvement in source code
comments over the last few months and I hope to see further improvement
before release.
 
> malformed messages:
> right error messages are being returned in most handlers. If they are
not
> then that should be fixed. Your patch does not seem to have changed
> malformed headers significantly. Did you have another patch that
address
> this.

As I said, this is an open issue.  But basically, once you get past the
parseCommand method and into a doXXX method, there appears to be no real
syntax checking.  There were random RuntimeExceptions thrown (see the
former check method) as well as potential NPEs, other exceptions at a
variety of locations (commands expecting arguments that don't get them).
I'm working on some of this now.

> Regd: throwing exception the idea was that server need not go crazy
> checking
> all possible bad arguments. It should at some point throw an exception
and
> terminate connection from an obviously bad client (or test program).

This is in violation of the spec.  There is no such thing as an
"obviously bad client".  The specification dictates the appropriate
behavior for cases of malformed values.  So we can't do that.

 > Excellent patches and bad bugs killed.

Thank you.

--Peter



--
To unsubscribe, e-mail:   <mailto:james-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:james-dev-help@jakarta.apache.org>


Mime
View raw message