Sounds good to me
Cheers, Sergey
On 10/05/12 20:42, Marek Potociar wrote:
> All,
> thanks for the useful discussion so far. Let me summarize what I see as
> something we can (hopefully) agree upon:
>
> 1. ContainerRequestFilterContext
> * remove writeEntity()
> * keep readEntity()
> o invokes interceptors
> o does not buffer automatically
> * keep bufferEntity()
> o buffers un-intercepted entity stream
> * keep get/setInputStream()
> o these methods by-pass interceptors
> o invoking setInputStream resets the "buffered flag"
> * additional isRead/Buffered etc. methods might be proved as
> useful, but might be an overkill for now
> o we can add those later if we find out they are really needed
> 2. Interceptors
> * reader interceptors are invoked on inbound data, writer on
> outbound data only
> 3. Other business
> * adding support for a Feature on the server side would be nice
> o 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:
>
> 1. interceptors are global only (? not sure if we can all agree on that)
> 2. 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.
>>>
>>>
>>
>
--
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com