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