Hi Bill,
On 4/26/12 6:29 PM, Bill Burke wrote:
> On 4/26/12 8:29 AM, Marek Potociar wrote:
>> 1. Name-bound interceptors seem to clash with pre-matching filters. If
>> a pre-matching filter will try to manipulate the entity, the
>> name-bound interceptors would not get invoked, which may be fatal
>> (see Example 1).
>
> There's a few things we can do here. See later comments.
Haven't seen anything in your e-mail indicating why we need name-bound
interceptors.
To me, interceptors are something that encodes/decodes/processes the
"on-the-wire" stream based on the headers - e.g. gzip,
encryption/decryption, signature checking. Caching being special - see
below.
>
>> 2. The generic type information on the interceptor seems redundant. It
>> is not clear what should the runtime do when an interceptor is
>> configured for a type that it does not support in it's generic type
>> declaration.
>
> I have yet to write an read interceptor that is type specific.
>
>> 3. The reader interceptors are now not restricted in any way wrt.
>> changing the returned entity Java type to a type that is
>> incompatible with the Java type requested by the caller (either
>> JAX-RS runtime or an interceptor up in the chain). Changing the type
>> of the returned entity in a non-compatible way seems to always have
>> fatal consequences as, by definition, the caller expects to receive
>> the original type it asked for.
>
> EJB Interception has a similar issue, so, IMO this is ok. Remember
> this is an advanced feature. We shouldn't have to protect the user
> from every stupid mistake they *possibly*, however improbable, could
> make. BTW, I also remember having a similar discussion when EJB 3.0
> was defining interceptors.
The caching example bypassing MBW is a nice one. See below for another
proposal.
>
>> 4. Filters may un-intentionally corrupt the entity stream in
>> applications configured asymmetrically wrt. interceptors (see
>> Example 2)
>>
>
> #4 appears in the Servlet specification as well. There's nothing
> stopping a servlet Filter from corrupting the InputStream. There's
> also the case of HttpServletRequest.getParameters() and
> HttpServletRequest.getInputStream(). If you getParameter() you could
> possibly corrupt the InputStream. So, IMO, corrupting the stream is
> acceptable behavior. We shouldn't have to protect the user from
> everything.
>
> In Resteasy we do not allow unmarshalling within a Filter. Only
> getting/setting of OutputStreams. Filters are generally only used to
> check/add/remove/modify headers, at least in the features I've
> implemented.
>
>> We haven't found any reasonable way to resolve the outlined issues
>> without changing the API. Here are couple of options for a solution we
>> can see:
>>
>> * /Option 1/
>> o interceptors should not be typed
>
> +1 Interceptors should not be typed. I have yet to write one that was.
>
>> should be stream focused -
>
> Not sure. I like the option of being able to change the returned
> object even though I've never actually used that feature. Again...We
> don't have to protect the user from being stupid. This is an advanced
> feature, and EJB has similar issue.
>
>> i.e. could be renamed to Input/OutputStreamInterceptors
>> + people should use MBW and/or filters for entity
>> type-dependent manipulation
>
> -1. I like the current names and they fit.
>
>> o interceptors should only be global (as they may need to be
>> invoked in the pre-matching phase)
>
> -1. I'll make a counter proposal later.
Why -1? What is the issue with this? I don't like the counter proposal -
see below.
>
>> o interceptors should be executed just once per request/response
>> when reading from/writing to the wire, not for intermediate
>> transformations (hence calling it MBW interceptors may be
>> misleading)
>
> -1. See counter proposal.
Is this -1 on the name or on the note that it should be called only
once. I hope it is the first one. As I don't think you want to
zip/unzip, encrypt/decrypt, etc. for the intermediate transformations
repeatedly as you are doing read/writeEntity in the filters. IMO the
interceptors should kick in only when reading the original stream from
the wire and when writing it back to the wire. Again, caching being
seemingly the only special thing that seems to break it.
See later for an example of filters that read/write entities.
>
>> o filters should work with entities (decoded) - with one possible
>> kind of entity being a stream
>> + drop the get/setEntityStream methods as redundant (or keep
>> them just as convenience shortcuts for read/writeEntity
>> counterparts)
>>
>
> -1.
Why?
>
>> Option 1 seems to allow the interceptors to be reused between client and
>> server side. But not sure if this is not only useful in a very limited
>> set of use cases (e.g. real life may show that in most cases a server
>> and client-side implementation may need to be separate anyway due to the
>> need of attaching/checking different headers on the client side than on
>> the server side etc.).
>>
>
>
> Again, I've already used a similar structure "in real life".
> Interceptor re-usability is a reality.
>
>
> Counter Proposal:
>
> - Consider not allowing unmarshalling within a PreMatch filter (but
> allow get/set of Stream). PreMatching should exist primarily to
> reroute the request, modify headers (i.e. the Accept header), or
> change the HTTP Method. This is a reasonable restriction, IMO.
> - Consider removing marshalling/unmarshalling capabilities altogether
> from within Filters. Only allow get/set of Streams. This is
> Resteasy's current behavior. We do not allow
> marshalling/unmarshalling within Filters.
Logging, authentication filters (auth params may be in headers, query
params as well as in the entity), Post method override filter (that
reads the method that should override post from the form param) are just
a few examples of filters we have in Jersey that access the entity.
Moreover what is the getStream() good for in the filter, if all you get
is zipped and encrypted thing you can't do much with.
So, -1.
>
>
> If you want to keep readEntity() in all conditions here are some options:
>
> * Always buffer the entity if you unmarshall within a Filter, makes
> things simpler to define.
+1, I believe that's already the case, but the interceptor chain should
kick in when buffering IMO - i.e. the entity should be buffered in the
"decoded" state, since when somebody needs to work with entity, I don't
think you want to run interceptors (i.e. unzip and decrypt) every time
you read from the buffer, do you?
Seems to me there are two classes of use-cases that have conflicting
requirements - both are now mixed into the interceptors proposal. These are:
1) Encoding/decoding the on-the-wire stream - Gzip, encrypt, signature
interceptors fall into this category
2) caching - is it a one-member class? any other fall here?
Marek's proposal mainly considers class 1) and for that I think it is
the right way to go. For these you want to invoke the interceptors only
when working with the original stream, not repeatedly.
For class 2) you want to invoke the read interceptor per type you pass
to the readEntity() method (or send in writeEntity()).
So perhaps this should be looked at separately and not mixed into the
same single concept as it really is a different thing.
A heretic question might be if we think class 2) is generic enough -
i.e. do we expect many people providing custom implementation of this
kind of interceptor - so that it has to be part of the standard rather
than being an implementation detail of read/writeEntity method of a
particular JAX-RS impl.
On the other hand, supporting this use-case may be as simple as coming
up with appropriate decorator annotation indicating that the interceptor
is entity specific - and defining that those would kick in for every
read/writeEntity(), not just when reading the on-the-wire stream (and
IMO should always be called after stream-oriented read
interceptors/before stream-oriented write interceptors).
Does that seem reasonable?
Martin