On 11/29/2011 04:35 PM, Sergey Beryozkin wrote:
> 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();
> }
> }
Note that you are trying to create a response from a request builder. In any case, returning response from a request
filter is not a major use case IMO. And optimizing for a 20% case is just wrong.
>
> 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
Not sure I follow. Where in my proposal am I using the input parameter to indicate the course of action?
Marek
>
> 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
>>>>>
>>>>
>>>>
>>>
>>>
>
>