Hi Paul, All,
inline...
Le 25/03/2018 à 03:02, Paul Foxworthy a écrit :
Hi all,
The servlet4preview package in Tomcat 8.5 "provides early access to some of
the features of Servlet 4.0" (
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/servlet4preview/package-summary.html
).
Does use of this package make OFBiz dependent on a preview version, and if
so is that a good idea? Should features that use some preview (like support
for Tomcat SSO, it appears) be contained in a branch or plugin until the
API has been stabilised?
What makes you think that Tomcat SSO depends on servlet4preview?
In the the analysis I did for https://issues.apache.org/jira/browse/OFBIZ-10304 I only found that using Tomcat 8.5 (hence servlet4preview) we no
longer can pass a standard HttpServletRequest or HttpServletResponse with current code.
Did you find something else?
Tomcat 9 was released in January, with support for Servlet 4.0. If we now
say OFBiz requires Servlet 4.0 and move to Tomcat 9, could we then use the
standard HttpServletRequest?
Yes, that would remove the problem and is IMO the best solution.
But after upgrading to Tomcat 8.5 at https://issues.apache.org/jira/browse/OFBIZ-9813 (we did not notice the possible issue with servlet4preview, I
must say it's not obvious in Tomcat 8.5 changelog: no warning) Michael tried to update to Tomcat 9 twice w/o success:
* https://issues.apache.org/jira/browse/OFBIZ-10026
* https://issues.apache.org/jira/browse/OFBIZ-10036
So, I think we need now to continue the work initiated by Michael to definitely
close the related dependent issues (by using Tomcat 9):
* OFBIZ-10304 that made the problem to appear. Actually it was already
existing for all services which were using standard HttpServletRequest or
HttpServletResponse as IN or OUT or IN/OUT attributes. Not much OOTB, but we
don't know for users... Note that with OFBIZ-9813 R17.12 uses Tomcat
8.5. So it's not only trunk and we need to consider this a bug and update
R17.12 as well.
* https://issues.apache.org/jira/browse/OFBIZ-10047 which IMO does not depend
on Tomcat 8.5 but made me research and open OFBIZ-10304 after I could
not revert the HttpServletRequestWrapper at
http://svn.apache.org/viewvc?view=revision&revision=1827441
* https://issues.apache.org/jira/browse/OFBIZ-10307 were I need to revert my
previous work committed for
https://issues.apache.org/jira/browse/OFBIZ-9833 which was partially wrong.
I purposely put the 1st appearance of a link inside the text. I think it's the
easiest way to follow a thought put in an email.
Jacques
Thanks
Paul Foxworthy
On 25 March 2018 at 04:49, Scott Gray <[email protected]> wrote:
So to summarize your long email ;-)
"The service engine has a limitation in that it only checks the interfaces
directly implemented by the object being tested. Tomcat's RequestFacade
doesn't directly implement javax.servlet.http.HttpServletRequest so it
fails to pass the type validation."
On the surface your suggested fix looks fine to me, my only concern being
that we may need to do some performance testing. For every service that
specifies an interface we'll now be checking the full type hierarchy which
I imagine will be more expensive but I'm not sure how expensive.
I'm also not even sure if our custom ObjectType methods for checking this
type of thing are necessary any more with so many new java versions since
it was decided these were needed for performance reasons.
Regards
Scott
On 24 March 2018 at 10:59, Jacques Le Roux <[email protected]>
wrote:
Le 23/03/2018 à 17:09, Scott Gray a écrit :
I don't need to try anything, I *know* that the service engine is
supposed
to accept a concrete class of an interface if the interface is specified
as
the attribute type.
Either the service engine is broken by not accepting concrete
implementations, or the bug report is incorrect.
Neither, it's "unfortunate", and a bit complicated.
Tomcat servlet4preview was introduced with Tomcat 8.5
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
lina/servlet4preview/package-summary.html
Before James introduced Tomcat SSO, we had only one service passing a
javax.servlet.http.HttpServletRequest to a service:
payPalCheckoutUpdate.
AFAIK, we have actually always passed a org.apache.catalina.connector.
RequestFacade
to services when asking for javax.servlet.http.HttpServletRequest in
services definition.
Since Tomcat 8.5 RequestFacade implements javax.servlet.http.
HttpServletRequest
indirectly through org.apache.catalina.servlet4pr
eview.http.HttpServletRequest
https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/cata
lina/connector/RequestFacade.html
Since we have no tests on the payPalCheckoutUpdate service we did not
spot
that Tomcat servlet4preview was introduced with Tomcat 8.5
The classes check is done by the deeper interfaceOf() method of the
ObjectType class using Class.getInterfaces()
https://docs.oracle.com/javase/8/docs/api/java/lang/Class.
html#getInterfaces--
Class.getInterfaces() does not recurse and stops at one level up. So in
case of RequestFacade it returns only servlet4preview.http.
HttpServletRequest
and not javax.servlet.http.HttpServletRequest
So when interfaceOf() compares the Classes it fails.
What happened with my introduction of the HttpServletRequestWrapper in
ContextFilter is it hid the RequestFacade because
HttpServletRequestWrapper
implements javax.servlet.http.HttpServletRequest
https://docs.oracle.com/javaee/7/api/javax/servlet/http/
HttpServletRequest.html
So when James introduced Tomcat SSO and optionally passed a
javax.servlet.http.HttpServletRequest to the userLogin service it did
not
break.
But when I removed HttpServletRequestWrapper from ContextFilter it popped
up
Summary: it's unfortunate because we have no tests on the
payPalCheckoutUpdate service.
Because I temporarily introduced HttpServletRequestWrapper James was able
to pass a javax.servlet.http.HttpServletRequest, like in
payPalCheckoutUpdate.
When I reverted (removed HttpServletRequestWrapper from ContextFilter) I
discovered that we had a problem with Tomcat 8.5.
I propose this fix which uses org.apache.commons.lang3.
ClassUtils.getAllInterfaces()
and works
http://commons.apache.org/proper/commons-lang/javadocs/api-
release/src-html/org/apache/commons/lang3/ClassUtils.html
Index: framework/base/src/main/java/org/apache/ofbiz/base/util/Obje
ctType.java
===================================================================
--- framework/base/src/main/java/org/apache/ofbiz/base/util/
ObjectType.java
(révision 1827594)
+++ framework/base/src/main/java/org/apache/ofbiz/base/util/
ObjectType.java
(copie de travail)
@@ -263,7 +263,7 @@
*/
public static boolean interfaceOf(Class<?> objectClass, Class<?>
interfaceClass) {
while (objectClass != null) {
- Class<?>[] ifaces = objectClass.getInterfaces();
+ List<Class<?>> ifaces = org.apache.commons.lang3.Class
Utils.getAllInterfaces(objectClass);
for (Class<?> iface: ifaces) {
if (iface == interfaceClass) {
Index: framework/common/servicedef/services.xml
===================================================================
--- framework/common/servicedef/services.xml (révision 1827594)
+++ framework/common/servicedef/services.xml (copie de travail)
@@ -379,7 +379,7 @@
<service name="userLogin" engine="java" location="
org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
<description>Used to Automatically Authenticate a
username/password; create a UserLogin object</description>
<implements service="authenticationInterface"/>
- <attribute name="request" mode="IN" type="org.apache.catalina.
connector.RequestFacade"
optional="true"/>
+ <attribute name="request" mode="IN" type="javax.servlet.http.
HttpServletRequest"
optional="true"/>
</service>
<service name="createUserLogin" engine="java" auth="false"
location="org.apache.ofbiz.common.login.LoginServices"
invoke="createUserLogin">
Jacques
Regards
Scott
On 23 March 2018 at 07:36, Jacques Le Roux <
[email protected]>
wrote:
Did you try what I said?
You can easily check by svn updating to r1819133 and removing the
wrapper
in ContextFilter.java.
Maybe we need to revert Tomcat SSO then?
Jacques
Le 23/03/2018 à 03:39, Scott Gray a écrit :
Something else must be wrong Jacques, I can't understand what you're
saying
in the ticket description or in your reply above but I do know the
following:
OFBiz is perfectly capable of knowing that
org.apache.catalina.connector.RequestFacade
implements the javax.servlet.http.HttpServletRequest and it should
pass
type validation without errors.
Since we know that, this commit becomes unnecessary.
Regards
Scott
On 22 March 2018 at 16:06, Jacques Le Roux <
[email protected]>
wrote:
Michael,
I commited http://svn.apache.org/viewvc/o
fbiz/ofbiz-framework/trunk/fra
mework/webapp/src/main/java/org/apache/ofbiz/webapp/contro
l/ContextFilter.java?r1=1813679&r2=1813678&pathrev=1813679
Where I added a wrapper. Then for "Tomcat SSO" (OFBIZ-10047) James
committed
http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
mework/common/servicedef/services.xml?r1=1819133&r2=1819132&
pathrev=1819133
If I had not committed the wrapper in ContextFilter.java beforehand,
James
would not have been allowed to commit
<attribute name="request" mode="IN" type="javax.servlet.http.HttpS
ervletRequest"
optional="true"/>
Because it's then rejected, because then the userLogin service
actually
receive an org.apache.catalina.connector.RequestFacade
(RequestFacade
implements HttpServletRequest.)
You can easily check by svn updating to r1819133 and removing the
wrapper
in ContextFilter.java.
You are right that it ties OFBiz to Tomcat. We took this decision
sometimes ago and we no longer support other webapp servers OOTB, nor
tools
like Jetty.
So OFBiz OOTB totally depends on Tomcat as the build.gradle file
shows.
So
I think it's safe to use a RequestFacade there.
If an user feels the need to use another webapp servers or Jetty, I
think
changing that would not be the most of the worries (the rest being
much
more frightening, I know I did it once with Geronimo).
Since the check is done at the service level, it's hard to do
otherwise
But I must say I did not dig too much and it's maybe possible, we can
discuss that...
Jacques
Le 22/03/2018 à 16:22, Michael Brohl a écrit :
Mmhhh, Jacques,
I think this is problematic because it ties a special implementation
for
Tomcat to the service. I didn't see this anywhere else.
The issue (https://issues.apache.org/jira/browse/OFBIZ-10304) is a
bit
unclear and I don't get the purpose of this change.
Can you please explain more clearly which problem this changes
solves
and
why we'll need org.apache.catalina.connector.RequestFacade as the
type?
Thanks,
Michael
Am 21.03.18 um 21:53 schrieb [email protected]:
Author: jleroux
Date: Wed Mar 21 20:53:41 2018
New Revision: 1827439
URL: http://svn.apache.org/viewvc?rev=1827439&view=rev
Log:
Fixed: The "request" attribute type of the userLogin service is
wrong
(OFBIZ-10304)
It should have been org.apache.catalina.connector.RequestFacade"
instead of javax.servlet.http.HttpServletRequest see the Jira for
details
Modified:
ofbiz/ofbiz-framework/trunk/framework/common/servicedef/
services.xml
Modified: ofbiz/ofbiz-framework/trunk/fr
amework/common/servicedef/serv
ices.xml
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
mework/common/servicedef/services.xml?rev=1827439&r1=1827438
&r2=1827439&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
ices.xml
(original)
+++ ofbiz/ofbiz-framework/trunk/framework/common/servicedef/serv
ices.xml
Wed Mar 21 20:53:41 2018
@@ -379,7 +379,7 @@ under the License.
<service name="userLogin" engine="java" location="
org.apache.ofbiz.common.login.LoginServices" invoke="userLogin">
<description>Used to Automatically Authenticate a
username/password; create a UserLogin object</description>
<implements service="authenticationInterface"/>
- <attribute name="request" mode="IN"
type="javax.servlet.http.HttpServletRequest" optional="true"/>
+ <attribute name="request" mode="IN"
type="org.apache.catalina.connector.RequestFacade"
optional="true"/>
</service>
<service name="createUserLogin" engine="java" auth="false"
location="org.apache.ofbiz.common.login.LoginServices"
invoke="createUserLogin">
