On 29/11/11 15:28, Marek Potociar wrote:
>
>
> On 11/29/2011 03:15 PM, Sergey Beryozkin wrote:
>> On 29/11/11 13:43, Marek Potociar wrote:
>>> My opinion on the topic is following:
>>>
>>> Filters should only stop the filter chain processing when returning a response (or throwing an exception). I agree with
>>> Bill on this one.
>>>
>>> Making a filter to return a response as suggested by Sergey does not seem like a good design to me. I don't like the
>>> idea that we would be making decisions based on whether a returned response is null or not. Also, as a request filter
>>> developer, I would be very unhappy about the code that needs to return a null after updating the request. It just reads
>>> plain ugly. But most importantly such design would be difficult to extend to support other types of actions in the
>>> future (e.g. non-blocking filters that may require to programmatically return a "suspend" action and a resume callback
>>> somehow).
>>>
>> returning null to indicate the chain can continue is not cool, agreed, but is easy to grasp IMHO
>>> I do still agree with Sergey that currently used FilterAction enum provides only very limited functionality. I would
>>> propose to convert FilterAction to an opaque interface that is returned from the filter. Instances of the interface
>>> would be produced by a call to the provided FilterContext instance (which would be made immutable), e.g.:
>>>
>>> public class MyFilter implements RequestFilter {
>>>
>>> public FilterAction preFilter(FilterContext context) {
>>> // ... do some request processing
>>> if (stopProcessing) {
>>> return context.stop(response);
>>> }
>>>
>>> return context.continue(request);
>>> }
>>> }
>>>
>> Why should users suffer like this ? Just for them to anticipate some future non-main-stream enhancements like the
>> support for the suspended invocations from filters ?
>
> Not sure I understand what you mean by suffering.
see Bill's response, but see more on it below
> What is more readable, this:
>
> public class LoggingFilter implements RequestFilter {
> public FilterAction preFilter(FilterContext context) {
> logRequest(context.getRequest());
> return context.continue();
> }
> }
>
> public class UpdatingFilter implements RequestFilter {
> public FilterAction preFilter(FilterContext context) {
> RequestBuilder rb = context.getRequestBuilder();
>
> ...
>
> return context.continue(rb.build());
> }
> }
>
> Or this:
>
> public class LoggingFilter implements RequestFilter {
> public Response preFilter(FilterContext context) {
> logRequest(context.getRequest());
> return null;
> }
> }
>
> public class UpdatingFilter implements RequestFilter {
> public FilterAction preFilter(FilterContext context) {
> RequestBuilder rb = context.getRequestBuilder();
>
> ...
>
> context.setRequest(rb.build());
> return null;
> }
> }
>
> ???
>
> If you ask me, the first set of examples is more readable and more comprehensible (no confusion about the meaning of the
> "return null;" line).
>
Sorry but you missed my point, the 2nd option is
public class UpdatingFilter implements RequestFilter {
public Response preFilter(FilterContext context) {
RequestBuilder rb = context.getRequestBuilder();
return rb.build();
}
}
That is so much simpler IMHO. Generally speaking using the input
parameter to indicate the course of action is a bad idea IMHO, and it is
just unreadable with your proposal, at least the Bill;s one make it a
bit better, but still a wrong approach
Sergey
Sergey
> Marek
>
>>
>>> This way we would solve the issue with the disconnected return enum value from the actual filter continuation.
>>> Continuation (FilterAction) would be constructed all at once including the relevant continuation values. Definition of
>>> continuation factory methods on the FilterContext would also enforce the proper set of the output data required for each
>>> particular continuation. This solution is also easily extensible in the future - we would just need to make sure to
>>> properly document that FilterContext must not be extended by the API users as it can be subject to extension in the
>>> future.
>>>
>> I'd propose introducing a SuspendedInvocation exception for filter developers to be able to indicate it instead of
>> making it really complex
>>
>> Sergey
>>
>>
>>> Marek
>>>
>>> On 11/18/2011 05:36 PM, Santiago Pericas-Geertsen wrote:
>>>>
>>>> On Nov 18, 2011, at 11:11 AM, Bill Burke wrote:
>>>>
>>>>>
>>>>>
>>>>> On 11/18/11 10:30 AM, Santiago Pericas-Geertsen wrote:
>>>>>>
>>>>>> On Nov 18, 2011, at 10:21 AM, Bill Burke wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 11/18/11 10:14 AM, Santiago Pericas-Geertsen wrote:
>>>>>>>> Hi Sergey,
>>>>>>>>
>>>>>>>> It's an interesting observation. However, I think there may be use cases in which you want to stop the chain
>>>>>>>> _without_ producing a response.
>>>>>>>>
>>>>>>>> Suppose an app has 2 authentication filters (e.g. for different authentication mechanisms it supports): if the
>>>>>>>> first filter authenticates the user, then there's no need to run the next filter but there's also no response to
>>>>>>>> be set. This is why I think there's value in separating continuations from responses.
>>>>>>>>
>>>>>>>
>>>>>>> A filter should not be able to stop the chain unless it is providing a response. How would a filter know whether
>>>>>>> stopping the chain was correct behavior without full knowledge of the whole filter stack?
>>>>>>
>>>>>> User defined filters have the least priority in the chain (by default). As a developer, I know which filters I
>>>>>> installed, why shouldn't I be able to control the execution of my chain?
>>>>>>
>>>>>
>>>>> Because its really bad programming practice and prone to serious head-scratching errors?
>>>>
>>>> I'm not sure about the bad programming part (this pattern has been uses successfully in other frameworks), but I do
>>>> agree it can lead to errors. It's extra power with extra responsibility. Having said that, there's always value in
>>>> simplifying things too, and Sergey's suggestion does get rid of FilterAction.
>>>>
>>>> Any other votes/opinions on the topic?
>>>>
>>>> -- Santiago
>>>>
>>>
>>>
>>
>>
--
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com