Hi Paul,I agree, that's what I tried to express in my comments on https://issues.apache.org/jira/browse/OFBIZ-10047:
"7. Someone should check this solution from an architectural view. I appreciate the efforts but I am also in doubt if we should put this feature into the new release. It's very fresh, deals with a very central functionality and should be field tested more."
I really do think that we should have another process when we introduce this kind of new functionality, make use of new third party API etc..
You and Scott made valuable points in this discussion and if we would have had some kind of voting for this, they might be brought up earlier. The Jira discussions just slip through if there is no discussion on the dev list.
Thanks, Michael Am 25.03.18 um 03:02 schrieb Paul Foxworthy:
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? 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? 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 issupposedto 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.RequestFacadeto services when asking for javax.servlet.http.HttpServletRequest in services definition. Since Tomcat 8.5 RequestFacade implements javax.servlet.http.HttpServletRequestindirectly 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 notspotthat 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.HttpServletRequestand 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 becauseHttpServletRequestWrapperimplements 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 didnotbreak. 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"> JacquesRegards 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 thewrapperin 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'resaying 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 shouldpasstype 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 serviceactuallyreceive an org.apache.catalina.connector.RequestFacade(RequestFacadeimplements 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 fileshows.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 beingmuchmore frightening, I know I did it once with Geronimo). Since the check is done at the service level, it's hard to dootherwiseBut 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 changessolvesand 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: jlerouxDate: 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 iswrong(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.xmlModified: 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">
smime.p7s
Description: S/MIME Cryptographic Signature
