All,
thanks for the useful discussion so far. Let me summarize what I see as something we can (hopefully) agree upon:
ContainerRequestFilterContext
remove writeEntity()
keep readEntity()
invokes interceptors
does not buffer automatically
keep bufferEntity()
buffers un-intercepted entity stream
keep get/setInputStream()
these methods by-pass interceptors
invoking setInputStream resets the "buffered flag"
additional isRead/Buffered etc. methods might be proved as useful, but might be an overkill for now
we can add those later if we find out they are really needed
Interceptors
reader interceptors are invoked on inbound data, writer on outbound data only
Other business
adding support for a Feature on the server side would be nice
needs to distinguish the side either via separate interface or annotation
@Provider is significant only for the purpose of scanning, otherwise it should not be required.
Let me know if there are any objections to the above.
Now a few open points:
interceptors are global only (? not sure if we can all agree on that)
in pre-matching filters the readEntity() creates and invokes interceptor chain based on (any?) annotations passed to the method
still not sure here what is the proposed mechanism / API to allow the interceptor chain assembly
Let's now try to focus on resolving the open points as well as any objections to the closed points.
As for the open points, I am 50-50 here:
If we agree on #1 we don't need to solve issues around #2 but we may miss some use cases (which I'm still not fully convinced about).
OTOH, if we choose #2, we would not miss any potential use cases but we would most likely make the readEntity() unusable in pre-matching filters. Also I don't like the idea forcing people to instantiate annotations with #2 - there is no standard way how to do it which potentially breaks the portability of the code.
I guess right now I would personally lean more towards #1.
Marek
On May 9, 2012, at 5:28 PM, Martin Matula wrote:
> Perhaps the writeEntity() method should be removed from the request context - it adds complexity and I haven't found a use-case for it.
> Martin
>
> On 5/9/12 4:56 PM, Bill Burke wrote:
>>
>>
>> On 5/9/12 9:07 AM, Marek Potociar wrote:
>>> Resending in case you missed this.
>>>
>>
>> Ah I did miss, thanks. Response inline
>>
>>
>>> Marek
>>> On May 3, 2012, at 1:56 PM, Marek Potociar wrote:
>>>
>>>>
>>>> On Apr 27, 2012, at 9:52 PM, Bill Burke wrote:
>>>>
>>>>> Resteasy does not have a PreProcessor interceptor that reads the
>>>>> entity. You guys have a valid strong use case that does. It is a
>>>>> valid issue that we should talk about and work through to see what
>>>>> constraints we should add to the API or to see if we can get an
>>>>> improved API.
>>>>>
>>>>> I freaked out because I thought you were really serious about
>>>>> removing interceptors. Maybe start over and discuss again?
>>>>
>>>> Yes, let's do it.
>>>>
>>>>>
>>>>> I think:
>>>>>
>>>>> a) Removing generics might be a good idea. I've never used them. Have
>>>>> you guys? Then again, what if it could be used as a deployment check?
>>>>> If an typed interceptor is assigned to a resource method with
>>>>> non-matching types, it could be flagged at deployment time
>>>>
>>>> As Santiago mentioned, the generic type on interceptors was removed
>>>> from the API.
>>>>
>>>>> b) We should investigate the behavior of calling write/readEntity
>>>>> within a filter.
>>>>
>>>> Perhaps we need to take a step back and investigate the role the
>>>> interceptors play in consuming/producing messages. From the end user
>>>> point of view, I would specify reader interceptors with regard to the
>>>> inbound message (i.e request on the server and response on the client)
>>>> and writer interceptors with regard to the outbound message (i.e.
>>>> response on the server, request on the client). As a user or a filter
>>>> writer I would thus find it *very confusing* if replacing an entity in
>>>> an inbound filter via writeEntity invoked writer interceptors that
>>>> were set up for processing outbound messages.
>>>>
>>
>> IMO, both outbound and writeEntity() are the same. They are marshalling a Java object to a buffer. I would find it confusing if interceptors weren't invoked. But I don't care that much. I was just trying to offer some solution to Martin's complaint that read/writeEntity() could screw up filters/interceptors down the chain that were expecting the inbound buffer to be a specific format.
>>
>>
>>>> So if we want to keep the ability to replace entity in the inbound
>>>> filter, we must make sure we are not invoking outbound interceptors as
>>>> part of this operation. Alternatively, we can consider removing the
>>>> writeEntity support in inbound filters with the risk of missing some
>>>> use case that would be awkward to implement.
>>>>
>>
>> Again, IMO, global and any interceptor triggered by the annotation array passed into writeEntity() should be applied.
>>
>>
>>
>>>> A short side note: this whole discussion also brings a question
>>>> whether for an end user configuring filters or interceptors directly
>>>> is the right granularity. Perhaps we should consider bringing the
>>>> client-side Feature concept to the server-side too? The feature would
>>>> provide a better granularity and would encapsulate configuration and
>>>> setup of all the providers needed for implementing a feature.
>>
>> I like the idea of a separate ClientFeature and a ServerFeature (or an annotation to designate client or server). BTW, the only problem I see with Feature (both on client and server) is scanning. I assume it will be allowed for filters/interceptors/mbr/mbw that are managed by a Feature to not have to be annotated with @Provider? Otherwise they could be doubly bound/instantiated/configured. Once from the Feature, once from the scanning.
>>
>>
>>>>
>>>>>
>>>>> c) Maybe writeEntity within PreProcessing should create its own
>>>>> interceptor chain based on the type and annotation[] array passed
>>>>> into it?
>>>>
>>>> Just to clarify:
>>>> Are you talking about the named binding annotations here?
>>>> How would you instantiate those custom annotations?
>>>> Would this concept be used in the post-matching inbound filters too?
>>>>
>>
>> I'm saying that anytime anybody calls writeEntity() global interceptors and anything triggered by the annotations you pass in gets invoked. There are ways to instantiate annotations.
>>
>>
>>>> I'm also afraid that this extra dynamism would have a negative
>>>> performance impact. IOW, you cannot calculate everything at
>>>> deploy-time but you need to re-construct the interceptor chains at
>>>> run-time again and again. Any implementation of caching the
>>>> pre-created chains based on values of an array would be rather time
>>>> consuming too.
>>>>
>>
>> Calling writeEntity() is already a performance hit as it is marshalling the Java object to a buffer which will probably be much much higher penalty than any interceptor you invoke.
>>
>>
>>>>> d) Maybe readEntity() should automatically buffer the request?
>>>>
>>>> What about readEntity(InputStream.class)? I thought you are against
>>>> tiny exceptions in the javadoc...
>>>>
>>
>> True. So automatic buffering is probably not a good idea.
>>
>>
>>>>> e) Maybe methods on ContainerRequestContext should be added like
>>>>> wasBuffered(), wasRead() etc., then other interceptors will know that
>>>>> a read/write/buffering happened?
>>>>
>>>> Let's not focus on container and let's talk about inbound filter
>>>> contexts in general instead.
>>>>
>>>> Currently bufferEntity() is documented to be idempotent. So calling it
>>>> in every filter to ensure the entity is buffered does the trick
>>>> without any negative performance impact. But I don't have a strong
>>>> opinion here - if you think that having isEntityBuffered() would make
>>>> the API more understandable by end users, we can add it.
>>>>
>>
>> Its not an understandability issue. Martin is worried about input being emptied or modified by one filter that screws up another filter later on in the chain. I'm wondering if methods like wasRead(), etc. would help the filter developer identify these problem areas. Or maybe its just overkill?
>>
>>>> I don't see a use case for adding isEntityRead().
>>>>
>>>> Also let me go back in the emails history a bit and ask a question on
>>>> your verification example:
>>>> Provided the fact that I have a resource method that requires the
>>>> inbound message (with a potential entity) to be signed, but does not
>>>> consume the entity, how is your signature verification implementation
>>>> based on reader interceptors going to kick in?
>>>>
>>
>> Good point. Verification might be a good usecase for why you'd want to bufferEntity() and readEntity() within a Filter.
>>
>>
>