Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/ specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/ specialpurpose/solr/config/ specialpurpose/solr/entitydef/ specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...

[Shi Jinghai-3]

Hi Jacopo,

Let me clean the OFBizSolrContextFilter now.

Kind Regards,

Shi Jinghai


-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月5日 22:20
收件人: [hidden email]
主题: Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/ specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/ specialpurpose/solr/config/ specialpurpose/solr/entitydef/ specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...

Hi Jinghai,

I have noticed that when you have committed the Solr component you have created a class named OFBizSolrContextFilter.java that is mostly the copy of ContextFilter: as a consequence the logic implemented in its utility methods has been duplicated.
I didn't check, but it is possible that other code in this commit is affected by the same anti pattern.
This is making the maintenance of the code more difficult: for example, I am in the process of cleaning/refactoring the code in ContextFilter and I am not sure how to deal with the same copy of it in OFBizSolrContextFilter.

Is there anything you can do to fix this?

Thank you,

Jacopo


On Sun, Aug 30, 2015 at 3:27 PM, <[hidden email]> wrote:

> Author: shijh
> Date: Sun Aug 30 13:27:07 2015
> New Revision: 1700119
>
> URL: http://svn.apache.org/r1700119
> Log:
> OFBIZ-5042 Apache Solr Implementation.
> 1. Put the patch into trunk.
> 2. Added access control to solr /admin/ pages.
> 3. Upgraded solr to 4.9.0 to match Lucene version in trunk.
> 4. Removed solr.solr.home, used
> SolrResourceLoader loader = new SolrResourceLoader("
> specialpurpose/solr/conf");
> instead.
>
> Added:
> ...

   ofbiz/trunk/specialpurpose/solr/src/org/ofbiz/solr/
> webapp/OFBizSolrContextFilter.java   (with props)
>

[Shi Jinghai-3]

Hi Jacopo,

I cleaned OFBizSolrContextFilter.java and submitted in rev. 1759401. Please check if it's required to do a further refactoring.

Kind Regards,

Shi Jinghai

-----邮件原件-----
发件人: Shi Jinghai [mailto:[hidden email]]
发送时间: 2016年9月6日 13:43
收件人: [hidden email]
主题: Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/ specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/ specialpurpose/solr/config/ specialpurpose/solr/entitydef/ specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...

Hi Jacopo,

Let me clean the OFBizSolrContextFilter now.

Kind Regards,

Shi Jinghai


-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月5日 22:20
收件人: [hidden email]
主题: Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/ specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/ specialpurpose/solr/config/ specialpurpose/solr/entitydef/ specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...

Hi Jinghai,

I have noticed that when you have committed the Solr component you have created a class named OFBizSolrContextFilter.java that is mostly the copy of ContextFilter: as a consequence the logic implemented in its utility methods has been duplicated.
I didn't check, but it is possible that other code in this commit is affected by the same anti pattern.
This is making the maintenance of the code more difficult: for example, I am in the process of cleaning/refactoring the code in ContextFilter and I am not sure how to deal with the same copy of it in OFBizSolrContextFilter.

Is there anything you can do to fix this?

Thank you,

Jacopo


On Sun, Aug 30, 2015 at 3:27 PM, <[hidden email]> wrote:

> Author: shijh
> Date: Sun Aug 30 13:27:07 2015
> New Revision: 1700119
>
> URL: http://svn.apache.org/r1700119
> Log:
> OFBIZ-5042 Apache Solr Implementation.
> 1. Put the patch into trunk.
> 2. Added access control to solr /admin/ pages.
> 3. Upgraded solr to 4.9.0 to match Lucene version in trunk.
> 4. Removed solr.solr.home, used
> SolrResourceLoader loader = new SolrResourceLoader("
> specialpurpose/solr/conf");
> instead.
>
> Added:
> ...

   ofbiz/trunk/specialpurpose/solr/src/org/ofbiz/solr/
> webapp/OFBizSolrContextFilter.java   (with props)
>

[Jacopo Cappellato-5]

On Tue, Sep 6, 2016 at 12:10 PM, Shi Jinghai <[hidden email]> wrote:

> Hi Jacopo,
>
> I cleaned OFBizSolrContextFilter.java and submitted in rev. 1759401.
> Please check if it's required to do a further refactoring.
>
> Kind Regards,
>
> Shi Jinghai
>
>

Thank you, Jinghai.
I still see a lot of code that is duplicated.
Also, I don't think it is a good idea to have a servlet filter that
initializes another filter within its init method:

contextFilter = new ContextFilter();
contextFilter.init(config);

The initialization of filters should be left to the servlet container
(e.g. Tomcat) based on the configuration in web.xml.

As regards the duplication of code, it affects unfortunately several other
filters in OFBiz, like SeoContextFilter, CatalogUrlSeoFilter etc... I think
that all these classes need a serious cleanup.

I will start a separate thread to discuss this topic.

Kind regards,

Jacopo

[Shi Jinghai-3]

I agree completely. I'll work on them this weekend.

-----邮件原件-----
发件人: Jacopo Cappellato [mailto:[hidden email]]
发送时间: 2016年9月8日 21:14
收件人: [hidden email]
主题: Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/ specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/ specialpurpose/solr/config/ specialpurpose/solr/entitydef/ specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...

On Tue, Sep 6, 2016 at 12:10 PM, Shi Jinghai <[hidden email]> wrote:

> Hi Jacopo,
>
> I cleaned OFBizSolrContextFilter.java and submitted in rev. 1759401.
> Please check if it's required to do a further refactoring.
>
> Kind Regards,
>
> Shi Jinghai
>
>

Thank you, Jinghai.
I still see a lot of code that is duplicated.
Also, I don't think it is a good idea to have a servlet filter that initializes another filter within its init method:

contextFilter = new ContextFilter();
contextFilter.init(config);

The initialization of filters should be left to the servlet container (e.g. Tomcat) based on the configuration in web.xml.

As regards the duplication of code, it affects unfortunately several other filters in OFBiz, like SeoContextFilter, CatalogUrlSeoFilter etc... I think that all these classes need a serious cleanup.

I will start a separate thread to discuss this topic.

Kind regards,

Jacopo

[Jacopo Cappellato-5]

Hi Jinghai,

I have started a separate thread with some design notes, see the thread
"Ideas about OFBiz servlet filters". We can discuss this topic there.

Thanks,

Jacopo

On Fri, Sep 9, 2016 at 4:32 AM, Shi Jinghai <[hidden email]> wrote:

> I agree completely. I'll work on them this weekend.
>
> -----邮件原件-----
> 发件人: Jacopo Cappellato [mailto:[hidden email]]
> 发送时间: 2016年9月8日 21:14
> 收件人: [hidden email]
> 主题: Re: svn commit: r1700119 [1/26] - in /ofbiz/trunk: ./ runtime/indexes/
> specialpurpose/ specialpurpose/solr/ specialpurpose/solr/conf/
> specialpurpose/solr/config/ specialpurpose/solr/entitydef/
> specialpurpose/solr/lib/ specialpurpose/solr/lib/compile/ s...
>
> On Tue, Sep 6, 2016 at 12:10 PM, Shi Jinghai <[hidden email]> wrote:
>
> > Hi Jacopo,
> >
> > I cleaned OFBizSolrContextFilter.java and submitted in rev. 1759401.
> > Please check if it's required to do a further refactoring.
> >
> > Kind Regards,
> >
> > Shi Jinghai
> >
> >
> Thank you, Jinghai.
> I still see a lot of code that is duplicated.
> Also, I don't think it is a good idea to have a servlet filter that
> initializes another filter within its init method:
>
> contextFilter = new ContextFilter();
> contextFilter.init(config);
>
> The initialization of filters should be left to the servlet container
> (e.g. Tomcat) based on the configuration in web.xml.
>
> As regards the duplication of code, it affects unfortunately several other
> filters in OFBiz, like SeoContextFilter, CatalogUrlSeoFilter etc... I think
> that all these classes need a serious cleanup.
>
> I will start a separate thread to discuss this topic.
>
> Kind regards,
>
> Jacopo
>