Ideas about OFBiz servlet filters

[Jacopo Cappellato-5]

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

[Jacques Le Roux]

Thanks Jacopo,

This sounds like a plan to me. I see Jinghai is already working on the most culprits, I hope he will get this message ASAP, to avoid doing useless
work. Then we can work together to clean the stuff

Jacques

Le 09/09/2016 à 10:07, Jacopo Cappellato a écrit :

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

[Pierre Smits]

I like the concept.

Best regards,

Pierre Smits

ORRTIZ.COM <http://www.orrtiz.com>
OFBiz based solutions & services

OFBiz Extensions Marketplace
http://oem.ofbizci.net/oci-2/

On Fri, Sep 9, 2016 at 10:07 AM, Jacopo Cappellato <
[hidden email]> wrote:

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

[Jacopo Cappellato-5]

And here is a proposal for some actionable items to contribute to this
effort:

1) review and document the current usage (by application) of filters
(classes and their order)
2) review the logic in the filters and compare it to the one of
ContextFilter (what is different, what is added, what is duplicated etc...)
3) identify the filters that "extends" the ContextFilter class and figure
out how to refactor their code to work in a filter chain where the first
filter is ContextFilter

Jacopo

On Fri, Sep 9, 2016 at 10:07 AM, Jacopo Cappellato <
[hidden email]> wrote:

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

[Pierre Smits]

I hope the proposal for actionable items will materialise rather sooner
than later in our JIRA.

Best regards,

Pierre Smits

ORRTIZ.COM <http://www.orrtiz.com>
OFBiz based solutions & services

OFBiz Extensions Marketplace
http://oem.ofbizci.net/oci-2/

On Fri, Sep 9, 2016 at 10:23 AM, Jacopo Cappellato <
[hidden email]> wrote:

> And here is a proposal for some actionable items to contribute to this
> effort:
>
> 1) review and document the current usage (by application) of filters
> (classes and their order)
> 2) review the logic in the filters and compare it to the one of
> ContextFilter (what is different, what is added, what is duplicated etc...)
> 3) identify the filters that "extends" the ContextFilter class and figure
> out how to refactor their code to work in a filter chain where the first
> filter is ContextFilter
>
> Jacopo
>
> On Fri, Sep 9, 2016 at 10:07 AM, Jacopo Cappellato <
> [hidden email]> wrote:
>
> > 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
> >
>

[Jacopo Cappellato-5]

I prefer to start the discussion in this list and consider to move to Jira
only in a second phase if a more structured coordination of tasks will be
useful.

Jacopo

On Fri, Sep 9, 2016 at 10:25 AM, Pierre Smits <[hidden email]>
wrote:

> I hope the proposal for actionable items will materialise rather sooner
> than later in our JIRA.
>
> Best regards,
>
> Pierre Smits
>
> ORRTIZ.COM <http://www.orrtiz.com>
> OFBiz based solutions & services
>
> OFBiz Extensions Marketplace
> http://oem.ofbizci.net/oci-2/
>
> On Fri, Sep 9, 2016 at 10:23 AM, Jacopo Cappellato <
> [hidden email]> wrote:
>
> > And here is a proposal for some actionable items to contribute to this
> > effort:
> >
> > 1) review and document the current usage (by application) of filters
> > (classes and their order)
> > 2) review the logic in the filters and compare it to the one of
> > ContextFilter (what is different, what is added, what is duplicated
> etc...)
> > 3) identify the filters that "extends" the ContextFilter class and figure
> > out how to refactor their code to work in a filter chain where the first
> > filter is ContextFilter
> >
> > Jacopo
> >
> > On Fri, Sep 9, 2016 at 10:07 AM, Jacopo Cappellato <
> > [hidden email]> wrote:
> >
> > > 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
> > >
> >
>

[Shi Jinghai-3]

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:[hidden email]]
发送时间: 2016年9月9日 16:07
收件人: [hidden email]
主题: 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

[Jacopo Cappellato-5]

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 <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月9日 16:07
> 收件人: [hidden email]
> 主题: 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
>

[Shi Jinghai-3]

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:[hidden email]]
发送时间: 2016年9月13日 16:30
收件人: [hidden email]
主题: 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 <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月9日 16:07
> 收件人: [hidden email]
> 主题: 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
>

[Jacopo Cappellato-5]

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 <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月13日 16:30
> 收件人: [hidden email]
> 主题: 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 <[hidden email]> 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:[hidden email]]
> > 发送时间: 2016年9月9日 16:07
> > 收件人: [hidden email]
> > 主题: 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
> >
>

[Jacques Le Roux]

Hi Jacopo,

I cursorily reviewed and it looks good to me (thanks for the documentation)

It seems a good idea to me and maybe we can go further with the idea of chaining filters and find other blocks in filters that can be shared

Thanks

Jacques


Le 14/09/2016 à 17:33, Jacopo Cappellato a écrit :

> 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 <[hidden email]> 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:[hidden email]]
>> 发送时间: 2016年9月13日 16:30
>> 收件人: [hidden email]
>> 主题: 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 <[hidden email]> 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:[hidden email]]
>>> 发送时间: 2016年9月9日 16:07
>>> 收件人: [hidden email]
>>> 主题: 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
>>>

[Shi Jinghai-3]

Hi Jacopo,

Thanks for your message!

The new ControlFilter works well as expected.

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? :)

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

Anyway, please do leave a message here if you have further changes on the filters.

Kind Regards,

Shi Jinghai


-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月14日 23:34
收件人: [hidden email]
主题: 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 <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月13日 16:30
> 收件人: [hidden email]
> 主题: 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 <[hidden email]> 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:[hidden email]]
> > 发送时间: 2016年9月9日 16:07
> > 收件人: [hidden email]
> > 主题: 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
> >
>

[Jacopo Cappellato-5]

Hi Jinghai,

please see inline:

On Sat, Sep 17, 2016 at 3:16 PM, Shi Jinghai <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月14日 23:34
> 收件人: [hidden email]
> 主题: 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 <[hidden email]>
> 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:[hidden email]]
> > 发送时间: 2016年9月13日 16:30
> > 收件人: [hidden email]
> > 主题: 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 <[hidden email]>
> 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:[hidden email]]
> > > 发送时间: 2016年9月9日 16:07
> > > 收件人: [hidden email]
> > > 主题: 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
> > >
> >
>

[Shi Jinghai-3]

Thanks Jacopo!

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


-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月17日 22:25
收件人: [hidden email]
主题: Re: Ideas about OFBiz servlet filters

Hi Jinghai,

please see inline:

On Sat, Sep 17, 2016 at 3:16 PM, Shi Jinghai <[hidden email]> 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:[hidden email]]
> 发送时间: 2016年9月14日 23:34
> 收件人: [hidden email]
> 主题: 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 <[hidden email]>
> 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:[hidden email]]
> > 发送时间: 2016年9月13日 16:30
> > 收件人: [hidden email]
> > 主题: 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 <[hidden email]>
> 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:[hidden email]]
> > > 发送时间: 2016年9月9日 16:07
> > > 收件人: [hidden email]
> > > 主题: 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
> > >
> >
>

[c.schinzer-2]

Great proposal, I had quite some headaches with this 2-3 years ago when I
was diving deep into the multitenancy with several (mini) shops and content
sites for different tenants.. ContextFilter handles it well, but the others
don't for the reasons you mention. I was hesitating to propose anything
because I did not feel safe enough with my .. .well ... workarounds. They
were likely not generic and robust enough under different circumstances
(more traffic, concurrent traffic to different sites).

Good to have a documented guideline now.

Thanks & regards

Carsten

2016-09-09 10:07 GMT+02:00 Jacopo Cappellato <
[hidden email]>:

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

[Jacopo Cappellato-5]

Hi Jinghai,

during the weekend I committed a series of changes to leverage the new
ControlFilter (and simplify the ContextFilter and other filters) in all the
OFBiz applications.
It would be great if you (and anyone else) could help reviewing my work and
testing the applications.
Feel free to continue the cleanup/refactoring in the SEO filter (or any
other filter): I just performed the most obvious ones.

Thank you,

Jacopo

On Sat, Sep 17, 2016 at 5:00 PM, Shi Jinghai <[hidden email]> wrote:

> Thanks Jacopo!
>
> I'll try to change the SEO filter next Monday.
>
>
> -----邮件原件-----
> 发件人: Jacopo Cappellato [mailto:[hidden email]]
> 发送时间: 2016年9月17日 22:25
> 收件人: [hidden email]
> 主题: Re: Ideas about OFBiz servlet filters
>
> Hi Jinghai,
>
> please see inline:
>
> On Sat, Sep 17, 2016 at 3:16 PM, Shi Jinghai <[hidden email]> 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:[hidden email]]
> > 发送时间: 2016年9月14日 23:34
> > 收件人: [hidden email]
> > 主题: 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 <[hidden email]>
> > 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:[hidden email]]
> > > 发送时间: 2016年9月13日 16:30
> > > 收件人: [hidden email]
> > > 主题: 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 <[hidden email]>
> > 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:[hidden email]]
> > > > 发送时间: 2016年9月9日 16:07
> > > > 收件人: [hidden email]
> > > > 主题: 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
> > > >
> > >
> >
>

[Jacopo Cappellato-5]

Thank you Carsten,

this is exactly the feedback I was looking for, very interesting.
During the weekend I have committed additional cleanups/fixes and it would
be great if you could test your applications with them and report any
issues you may find.
Over the years, several layers of changes piled up in the *Filters,
ControlServlet, *ViewHandler, *Widget, *Login code dealing with context
values and it has become a scary exercise to try to define the ones (and
their rules) that should be expected and used by the applications.
I took some risks and my recent work is a small step in that direction,
that I am sure will be beneficial to several adopters.
My next step is to further refactor the ContextFilter (and impacted
artifacts) and document its behavior; I am also planning to contribute some
unit tests for this refactored classes.

Yours and anyone feedback and help is very much appreciated!

Regards,

Jacopo

On Sat, Sep 17, 2016 at 6:22 PM, Carsten Schinzer <[hidden email]>
wrote:

> Great proposal, I had quite some headaches with this 2-3 years ago when I
> was diving deep into the multitenancy with several (mini) shops and content
> sites for different tenants.. ContextFilter handles it well, but the others
> don't for the reasons you mention. I was hesitating to propose anything
> because I did not feel safe enough with my .. .well ... workarounds. They
> were likely not generic and robust enough under different circumstances
> (more traffic, concurrent traffic to different sites).
>
> Good to have a documented guideline now.
>
> Thanks & regards
>
> Carsten
>
>

[Jacopo Cappellato-5]

In rev. 1761424 I did some additional cleanup.
Please test and report any issues you may find.

Thanks,

Jacopo

On Mon, Sep 19, 2016 at 8:44 AM, Jacopo Cappellato <
[hidden email]> wrote:

> Thank you Carsten,
>
> this is exactly the feedback I was looking for, very interesting.
> During the weekend I have committed additional cleanups/fixes and it would
> be great if you could test your applications with them and report any
> issues you may find.
> Over the years, several layers of changes piled up in the *Filters,
> ControlServlet, *ViewHandler, *Widget, *Login code dealing with context
> values and it has become a scary exercise to try to define the ones (and
> their rules) that should be expected and used by the applications.
> I took some risks and my recent work is a small step in that direction,
> that I am sure will be beneficial to several adopters.
> My next step is to further refactor the ContextFilter (and impacted
> artifacts) and document its behavior; I am also planning to contribute some
> unit tests for this refactored classes.
>
> Yours and anyone feedback and help is very much appreciated!
>
> Regards,
>
> Jacopo
>
> On Sat, Sep 17, 2016 at 6:22 PM, Carsten Schinzer <[hidden email]>
> wrote:
>
>> Great proposal, I had quite some headaches with this 2-3 years ago when I
>> was diving deep into the multitenancy with several (mini) shops and
>> content
>> sites for different tenants.. ContextFilter handles it well, but the
>> others
>> don't for the reasons you mention. I was hesitating to propose anything
>> because I did not feel safe enough with my .. .well ... workarounds. They
>> were likely not generic and robust enough under different circumstances
>> (more traffic, concurrent traffic to different sites).
>>
>> Good to have a documented guideline now.
>>
>> Thanks & regards
>>
>> Carsten
>>
>>

[Shi Jinghai-3]

Hi Jacopo,

Happy all work well!

I made a minor change on SeoContextFilter, moved several variables from doGet method to class private ones and initialized in init method (rev. 1761514).

Cheers,

Shi Jinghai

-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月19日 14:38
收件人: [hidden email]
主题: Re: Ideas about OFBiz servlet filters

Hi Jinghai,

during the weekend I committed a series of changes to leverage the new ControlFilter (and simplify the ContextFilter and other filters) in all the OFBiz applications.
It would be great if you (and anyone else) could help reviewing my work and testing the applications.
Feel free to continue the cleanup/refactoring in the SEO filter (or any other filter): I just performed the most obvious ones.

Thank you,

Jacopo

On Sat, Sep 17, 2016 at 5:00 PM, Shi Jinghai <[hidden email]> wrote:

> Thanks Jacopo!
>
> I'll try to change the SEO filter next Monday.
>
>
> -----邮件原件-----
> 发件人: Jacopo Cappellato [mailto:[hidden email]]
> 发送时间: 2016年9月17日 22:25
> 收件人: [hidden email]
> 主题: Re: Ideas about OFBiz servlet filters
>
> Hi Jinghai,
>
> please see inline:
>
> On Sat, Sep 17, 2016 at 3:16 PM, Shi Jinghai <[hidden email]> 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:[hidden email]]
> > 发送时间: 2016年9月14日 23:34
> > 收件人: [hidden email]
> > 主题: 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 <[hidden email]>
> > 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:[hidden email]]
> > > 发送时间: 2016年9月13日 16:30
> > > 收件人: [hidden email]
> > > 主题: 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
> > > <[hidden email]>
> > 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:[hidden email]]
> > > > 发送时间: 2016年9月9日 16:07
> > > > 收件人: [hidden email]
> > > > 主题: 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
> > > >
> > >
> >
>