Created attachment 29684 [details] Interface of ELInterpreter In some cases, applications need doing code generation for EL to make EL evaluation more faster. It's better for tomcat to provide an extensible EL Interpreter. So application can inject it's own ELInterpreter to replace the default JspUtil.interpreterCall. Attached an implementation.
Created attachment 29685 [details] ELInterpreterFactory
Created attachment 29686 [details] Patch for Generator.java
Comment on attachment 29685 [details] ELInterpreterFactory In principle this looks like a good idea. I have a couple of concerns with the patch as currently written: 1. No documentation. 2. No test cases. 3. The use of enum for the default instance is rather odd. 4. I dislike the use of system properties when they are not necessary. If the class name was handled as a servlet context initialization parameters then Tomcat already has the necessary plumbing for global, per host and per web application configuration. 5. Error messages need to use the standard i18n support.
Thanks Mark. I'll provide document & test cases for BUG 54239-54242.
Created attachment 29693 [details] Interface of ELInterpreter Add more comments
Created attachment 29694 [details] ELInterpreterFactory Added more comments
Created attachment 29695 [details] Patch for Generator.java Patch for Generator.java & LocalStrings.*
Created attachment 29696 [details] Test case for ELInterpreterFactory Test case for ELInterpreterFactory
(In reply to comment #3) > Comment on attachment 29685 [details] > ELInterpreterFactory > > In principle this looks like a good idea. > > I have a couple of concerns with the patch as currently written: > 1. No documentation. ~~~~~~~~~~~~~~~~~ Added more comments in ELInterpreter & ELInterpreterFactory > 2. No test cases. ~~~~~~~~~~~~~~~~~ Provided a test case for ELInterpreterFactory > 3. The use of enum for the default instance is rather odd. ~~~~~~~~~~~~~~~~~ Fixed > 4. I dislike the use of system properties when they are not necessary. If > the class name was handled as a servlet context initialization parameters > then Tomcat already has the necessary plumbing for global, per host and per > web application configuration. ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context initialization parameters. Still keep the feature from System Property. In our case, System Property can be easily passed in from such kind of Daemon-Watcher, For example: JSW. > 5. Error messages need to use the standard i18n support. ~~~~~~~~~~~~~~~~~~ Fixed
(In reply to comment #9) > (In reply to comment #3) > > 4. I dislike the use of system properties when they are not necessary. If > > the class name was handled as a servlet context initialization parameters > > then Tomcat already has the necessary plumbing for global, per host and per > > web application configuration. > ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context > initialization parameters. Still keep the feature from System Property. > In our case, System Property can be easily passed in from such kind of > Daemon-Watcher, For example: JSW. I haven't looked at the details here but it might be useful for you that system properties are automaticaly supported in most Tomcat config files, e.g. in server.xml and context.xml. What should work if you already support a className attribute is className="${my.el.interpreter}" (using a system property reference) and setting the system property via "-Dmy.el.interpreter=some.class.i.Use". No need for special code or a fixed system property name for this type of use case. Regards, Rainer
Thank you very much for your information! I'll try to follow up with this approach. (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #3) > > > > 4. I dislike the use of system properties when they are not necessary. If > > > the class name was handled as a servlet context initialization parameters > > > then Tomcat already has the necessary plumbing for global, per host and per > > > web application configuration. > > ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context > > initialization parameters. Still keep the feature from System Property. > > In our case, System Property can be easily passed in from such kind of > > Daemon-Watcher, For example: JSW. > > I haven't looked at the details here but it might be useful for you that > system properties are automaticaly supported in most Tomcat config files, > e.g. in server.xml and context.xml. What should work if you already support > a className attribute is className="${my.el.interpreter}" (using a system > property reference) and setting the system property via > "-Dmy.el.interpreter=some.class.i.Use". No need for special code or a fixed > system property name for this type of use case. > > Regards, > > Rainer
Created attachment 29704 [details] Test case for ELInterpreterFactory
Created attachment 29705 [details] ELInterpreterFactory
All have been fixed by your suggestion. Please take a look. Thanks. (In reply to comment #3) > Comment on attachment 29685 [details] > ELInterpreterFactory > > In principle this looks like a good idea. > > I have a couple of concerns with the patch as currently written: > 1. No documentation. > 2. No test cases. > 3. The use of enum for the default instance is rather odd. > 4. I dislike the use of system properties when they are not necessary. If > the class name was handled as a servlet context initialization parameters > then Tomcat already has the necessary plumbing for global, per host and per > web application configuration. > 5. Error messages need to use the standard i18n support.
Created attachment 29706 [details] Interface of ELInterpreter Added License information at the front of java file
Created attachment 29707 [details] ELInterpreterFactory Added APLV2.0 into ELInterpreterFactory.java
Created attachment 29708 [details] Test case for ELInterpreterFactory Added APL into TestELInterpreterFactory.java
Hi Mark, The code has been refined. Test cases and document also have been provided. Could you please take a look ?
Patch applied to trunk and 7.0.x with a few tweaks here and there and the addition of some very basic documentation. It will be included in 7.0.37 onwards.