Bug 54239 - Extensible EL Interpreter
Summary: Extensible EL Interpreter
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Jasper (show other bugs)
Version: trunk
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks: 54499
  Show dependency tree
 
Reported: 2012-12-04 06:56 UTC by Sheldon Shao
Modified: 2013-02-12 14:57 UTC (History)
0 users



Attachments
Interface of ELInterpreter (1.11 KB, text/plain)
2012-12-04 06:56 UTC, Sheldon Shao
Details
ELInterpreterFactory (3.15 KB, text/plain)
2012-12-04 06:56 UTC, Sheldon Shao
Details
Patch for Generator.java (2.36 KB, patch)
2012-12-04 06:57 UTC, Sheldon Shao
Details | Diff
Interface of ELInterpreter (1.04 KB, text/plain)
2012-12-05 09:37 UTC, Sheldon Shao
Details
ELInterpreterFactory (3.60 KB, text/plain)
2012-12-05 09:37 UTC, Sheldon Shao
Details
Patch for Generator.java (6.08 KB, patch)
2012-12-05 09:38 UTC, Sheldon Shao
Details | Diff
Test case for ELInterpreterFactory (2.82 KB, text/plain)
2012-12-05 09:39 UTC, Sheldon Shao
Details
Test case for ELInterpreterFactory (2.47 KB, text/plain)
2012-12-06 05:09 UTC, Sheldon Shao
Details
ELInterpreterFactory (3.38 KB, text/plain)
2012-12-06 05:10 UTC, Sheldon Shao
Details
Interface of ELInterpreter (1.84 KB, text/plain)
2012-12-06 06:01 UTC, Sheldon Shao
Details
ELInterpreterFactory (4.18 KB, text/plain)
2012-12-06 06:02 UTC, Sheldon Shao
Details
Test case for ELInterpreterFactory (3.26 KB, text/plain)
2012-12-06 06:03 UTC, Sheldon Shao
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sheldon Shao 2012-12-04 06:56:18 UTC
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.
Comment 1 Sheldon Shao 2012-12-04 06:56:48 UTC
Created attachment 29685 [details]
ELInterpreterFactory
Comment 2 Sheldon Shao 2012-12-04 06:57:26 UTC
Created attachment 29686 [details]
Patch for Generator.java
Comment 3 Mark Thomas 2012-12-04 08:37:15 UTC
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.
Comment 4 Sheldon Shao 2012-12-04 13:46:14 UTC
Thanks Mark.

I'll provide document & test cases for BUG 54239-54242.
Comment 5 Sheldon Shao 2012-12-05 09:37:12 UTC
Created attachment 29693 [details]
Interface of ELInterpreter

Add more comments
Comment 6 Sheldon Shao 2012-12-05 09:37:47 UTC
Created attachment 29694 [details]
ELInterpreterFactory

Added more comments
Comment 7 Sheldon Shao 2012-12-05 09:38:30 UTC
Created attachment 29695 [details]
Patch for Generator.java

Patch for Generator.java & LocalStrings.*
Comment 8 Sheldon Shao 2012-12-05 09:39:24 UTC
Created attachment 29696 [details]
Test case for ELInterpreterFactory

Test case for ELInterpreterFactory
Comment 9 Sheldon Shao 2012-12-05 09:50:06 UTC
(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
Comment 10 Rainer Jung 2012-12-05 13:50:58 UTC
(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
Comment 11 Sheldon Shao 2012-12-06 00:45:20 UTC
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
Comment 12 Sheldon Shao 2012-12-06 05:09:32 UTC
Created attachment 29704 [details]
Test case for ELInterpreterFactory
Comment 13 Sheldon Shao 2012-12-06 05:10:14 UTC
Created attachment 29705 [details]
ELInterpreterFactory
Comment 14 Sheldon Shao 2012-12-06 05:11:16 UTC
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.
Comment 15 Sheldon Shao 2012-12-06 06:01:30 UTC
Created attachment 29706 [details]
Interface of ELInterpreter

Added License information at the front of java file
Comment 16 Sheldon Shao 2012-12-06 06:02:32 UTC
Created attachment 29707 [details]
ELInterpreterFactory

Added APLV2.0 into ELInterpreterFactory.java
Comment 17 Sheldon Shao 2012-12-06 06:03:46 UTC
Created attachment 29708 [details]
Test case for ELInterpreterFactory

Added APL into TestELInterpreterFactory.java
Comment 18 Sheldon Shao 2013-01-04 09:15:30 UTC
Hi Mark,

The code has been refined. Test cases and document also have been provided. 

Could you please take a look ?
Comment 19 Mark Thomas 2013-02-12 14:57:01 UTC
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.