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. 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).
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
>>>
>>
>>
>
>