Thanks Jacopo!

I'll try to change the SEO filter next Monday.


-----邮件原件-----
发件人: Jacopo Cappellato [mailto:jacopo.cappell...@hotwaxsystems.com] 
发送时间: 2016年9月17日 22:25
收件人: dev@ofbiz.apache.org
主题: Re: Ideas about OFBiz servlet filters

Hi Jinghai,

please see inline:

On Sat, Sep 17, 2016 at 3:16 PM, Shi Jinghai <huaru...@hotmail.com> wrote:

> Hi Jacopo,
>
> Thanks for your message!
>
> The new ControlFilter works well as expected.
>
>
I am glad it worked well; hopefully in the weekend I am going to remove the
(now) duplicated logic from the ContextFilter (and other filters), similarly to 
what you did with the Solr filter here, and chain this filter in all the other 
web.xml.


> I checked in a new OFBizSolrContextFilter in rev. 1761214 to follow up 
> this change, the web.xml is changed as well.
>
> Personally, I feel the name ControlFilter and ContextFilter are a bit 
> strange, see web.xml:
>     <filter-mapping>
>         <filter-name>ControlFilter</filter-name>
>         <url-pattern>/*</url-pattern>
>     </filter-mapping>
>
>     <filter-mapping>
>         <filter-name>ContextFilter</filter-name>
>         <url-pattern>/control/*</url-pattern>
>     </filter-mapping>
>
> Should we exchange their names? :)
>

Well, while in the above example it may look a bit strange, I think that the 
names are still appropriate:
* ControlFilter performs some "controller" tasks (some of them with the help of 
the ControlServlet)
* ContextFilter prepares the contexts (session, request) with the objects 
expected by the framework and application code


> There is a servletContext attribute stored in request, I'm not sure 
> whether it should be removed or set in ControlFilter.
>

servletContext is already set by the ContextFilter, so I don't think it is 
required in the ControlFilter; what do you think?

Thanks,

Jacopo


>
> Anyway, please do leave a message here if you have further changes on 
> the filters.
>
> Kind Regards,
>
> Shi Jinghai
>
>
> -----邮件原件-----
> 发件人: Jacopo Cappellato [mailto:jacopo.cappell...@hotwaxsystems.com]
> 发送时间: 2016年9月14日 23:34
> 收件人: dev@ofbiz.apache.org
> 主题: Re: Ideas about OFBiz servlet filters
>
> In rev. 1760725 I have committed the new ControlFilter that is 
> designed to be chained with (before or after) ContextFilter and other filters.
> Please review it and let me know what you think.
> My next step is to remove the duplicated logic from ContextFilter and 
> update all the applications' web.xml files with this new filter.
>
> Thank you,
>
> Jacopo
>
> On Tue, Sep 13, 2016 at 11:03 AM, Shi Jinghai <huaru...@hotmail.com>
> wrote:
>
> > Hi Jacopo,
> >
> > Thanks for your consideration!
> >
> > I like the name ControlFilter. On the sequence of the filters, 
> > personally I think it's a policy for entrance. If put the 
> > ControlFilter 1st, it's a strict control. If put it last, it's a 
> > loose
> control. Anyway, we need it.
> >
> > When you complete, I will try to change SEO and solr filters to 
> > follow your update.
> >
> > Kind Regards,
> >
> > Shi Jinghai
> >
> >
> > -----邮件原件-----
> > 发件人: Jacopo Cappellato [mailto:jacopo.cappell...@hotwaxsystems.com]
> > 发送时间: 2016年9月13日 16:30
> > 收件人: dev@ofbiz.apache.org
> > 主题: Re: Ideas about OFBiz servlet filters
> >
> > Thank you Jinghai,
> >
> > I agree we should separate the concerns and split the ContextFilter 
> > into two filters; I am going to work on this and I am planning to 
> > separate the "controller" related concerns (like allowPaths and 
> > redirectPath functions) into a new filter named ControlFilter.
> > But, shouldn't the ControlFilter be executed before the ContextFilter?
> > Will it conflict with the behavior of the CatalogUrlFilter and other 
> > filters?
> >
> > Jacopo
> >
> > On Sun, Sep 11, 2016 at 8:13 AM, Shi Jinghai <huaru...@hotmail.com>
> wrote:
> >
> > > Great, Jacopo!
> > >
> > > I think it would be better to separate the allowPaths and 
> > > redirectPath functions to a new filter. If ContextFilter be the 
> > > 1st filter, the new filter will be the last I guess. Between them,
> CatalogUrlFilter and etc.
> > > will be there.
> > >
> > > Kind Regards,
> > >
> > > Shi Jinghai
> > >
> > > -----邮件原件-----
> > > 发件人: Jacopo Cappellato 
> > > [mailto:jacopo.cappell...@hotwaxsystems.com]
> > > 发送时间: 2016年9月9日 16:07
> > > 收件人: dev@ofbiz.apache.org
> > > 主题: Ideas about OFBiz servlet filters
> > >
> > > A web application, in order to leverage the OFBiz framework, 
> > > requires that a series of objects are in its contexts (servlet 
> > > context, session and
> > > request) such as "delegator", "delegatorName", "dispatcher", "security"
> > > etc. etc...
> > > This setup is performed by the logic contained in the servlet 
> > > filter implemented by the following class:
> > >
> > > org.apache.ofbiz.webapp.control.ContextFilter
> > >
> > > The execution of this logic is required for the application to run 
> > > properly.
> > > However, this filter is deployed in most but not all the web 
> > > application in the OFBiz codebase: there are few exceptions due to 
> > > the fact that a few web applications require the execution of 
> > > other filters
> > (e.g.
> > > CatalogUrlFilter, etc...).
> > >
> > > Unfortunately the way these filters have been implemented have 
> > > issues
> > > including:
> > > * some of them extend the ContextFilter and override its behavior 
> > > by copying some logic and adding new one; in these cases the 
> > > ContextFilter is also deployed but after the execution of the 
> > > extended filter
> > > * some of them have been copied from ContextFilter and then 
> > > adapted, introducing a lot of redundant code difficult to 
> > > maintain; in these cases the ContextFilter is not deployed
> > >
> > > There is now a chance for the community to help cleaning up these 
> > > classes and I am proposing the following guidelines:
> > >
> > > 1) servlet filters should be chained (rather than extended or
> > > replaced)
> > > 2) ContextFilter should always be used and should always be the 
> > > first
> > > (OFBiz) filter in the chain
> > > 3) if some of the behavior/logic of ContextFilter conflicts with 
> > > the ones of other filters, then ContextFilter should be enhanced 
> > > to prevent that (e.g. we can improve the code, move some of its 
> > > logic in a separate filter that can be executed after etc...)
> > > 4) the other filters should work well after the ContextFilter and 
> > > add behavior rather than overriding behavior or duplicating 
> > > behavior
> > >
> > > As a beneficial side effect of this effort, we will get a cleaner 
> > > picture (documented by the logic of ContextFilter) of all the 
> > > context objects required by OFBiz web applications.
> > >
> > > I hope it helps
> > >
> > > Jacopo
> > >
> >
>

Reply via email to