Re: [tomcat] branch main updated: Update Servlet API for parameter error handling changes

2023-08-15 Thread Mark Thomas 

On 15/08/2023 14:24, Christopher Schultz wrote:

Mark,

On 8/9/23 10:13, Mark Thomas wrote:

On 09/08/2023 14:45, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new c880673cff Update Servlet API for parameter error handling 
changes

c880673cff is described below

commit c880673cffad6b8884c1d2464e5a736e852e7eeb
Author: Mark Thomas 
AuthorDate: Thu Jun 15 12:15:08 2023 +0100

 Update Servlet API for parameter error handling changes


Hi all,

I have started work on implementing these changes but wanted to gather 
feedback before I go much further with the implementation work.


The key question is how much control do we want to provide over the 
behaviour if invalid parameters are encountered?


Question 1.
The default behavior will be to throw an exception. Do we want to use 
a single exception type or do we want to use specialized exceptions 
for each type of error? It is more (boilerplate) code but specialized 
exceptions allow applications to identify what went wrong without 
having to do things like parse error messages.


While I generally like specialized exception types, I think Tomcat would 
have to create these exception classes itself, meaning that applications 
sensitive to them would be non-portable.


Should we be encouraging such non-portability?


Good point. I have taken a couple of different approaches to implement 
this change and so far they have turned out to be too invasive for my 
liking. My current attempt (the 4th) is looking better and having a 
single exception type would further simplify things.


I guess there are three options:

a) use IllegalStateException

b) use a single new exception type that extends IllegalStateException

c) use multiple new exception types that extend IllegalStateException

The argument in favour of b) is it allows us to suppress the generation 
of stack traces which given that they are i) relatively expensive and 
ii) triggered by user input in this case




Question 2.
Do we want to support non-default behavior which, most likely, would 
be to ignore the invalid parameter(s) and continue processing? 
Assuming we do, do we want to make that configurable for each type of 
error or just have a single "ignoreInvalidParameters" option?


Ignoring parameter values is almost always NOT what the application is 
expecting, and may cause breakage. Definitely this should NOT be the 
default. I'm not sure if it even makes sense to provide a configuration 
option to ignore such parameter values (or names).



Option 1.
My current implementation is using specialized exceptions and per 
error type configuration although it is still in progress. I'm also 
logging all invalid parameters at debug level.


Option 2.
This is creating quite a lot of plumbing. I am wondering, given that 
this is essentially handling for invalid requests, whether it is worth 
it. The alternative approach I have in mind is:

- specialized exceptions for each type of error
- log errors at debug level
- no options to allow invalid parameters
- wait and see if users request configuration options as they start to
   migrate to Tomcat 11

At this point I am leaning towards option 2 while keeping the 
implementation I have so far for option 1 in a branch in case we 
decide to add it later.


Thoughts?


Option 2 seems more like "yes and" rather than "no but" -- an extension 
to Option 1, really. I guess except for the configuration option(s). I 
like option 2 because I'm not sure all that additional configuration 
will be worth it.


Thanks for the feedback. I'll hopefully have some code for folks to 
review later this week.


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Update Servlet API for parameter error handling changes

2023-08-15 Thread Christopher Schultz 

Mark,

On 8/9/23 10:13, Mark Thomas wrote:

On 09/08/2023 14:45, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new c880673cff Update Servlet API for parameter error handling 
changes

c880673cff is described below

commit c880673cffad6b8884c1d2464e5a736e852e7eeb
Author: Mark Thomas 
AuthorDate: Thu Jun 15 12:15:08 2023 +0100

 Update Servlet API for parameter error handling changes


Hi all,

I have started work on implementing these changes but wanted to gather 
feedback before I go much further with the implementation work.


The key question is how much control do we want to provide over the 
behaviour if invalid parameters are encountered?


Question 1.
The default behavior will be to throw an exception. Do we want to use a 
single exception type or do we want to use specialized exceptions for 
each type of error? It is more (boilerplate) code but specialized 
exceptions allow applications to identify what went wrong without having 
to do things like parse error messages.


While I generally like specialized exception types, I think Tomcat would 
have to create these exception classes itself, meaning that applications 
sensitive to them would be non-portable.


Should we be encouraging such non-portability?


Question 2.
Do we want to support non-default behavior which, most likely, would be 
to ignore the invalid parameter(s) and continue processing? Assuming we 
do, do we want to make that configurable for each type of error or just 
have a single "ignoreInvalidParameters" option?


Ignoring parameter values is almost always NOT what the application is 
expecting, and may cause breakage. Definitely this should NOT be the 
default. I'm not sure if it even makes sense to provide a configuration 
option to ignore such parameter values (or names).



Option 1.
My current implementation is using specialized exceptions and per error 
type configuration although it is still in progress. I'm also logging 
all invalid parameters at debug level.


Option 2.
This is creating quite a lot of plumbing. I am wondering, given that 
this is essentially handling for invalid requests, whether it is worth 
it. The alternative approach I have in mind is:

- specialized exceptions for each type of error
- log errors at debug level
- no options to allow invalid parameters
- wait and see if users request configuration options as they start to
   migrate to Tomcat 11

At this point I am leaning towards option 2 while keeping the 
implementation I have so far for option 1 in a branch in case we decide 
to add it later.


Thoughts?


Option 2 seems more like "yes and" rather than "no but" -- an extension 
to Option 1, really. I guess except for the configuration option(s). I 
like option 2 because I'm not sure all that additional configuration 
will be worth it.


-chris

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Update Servlet API for parameter error handling changes

2023-08-14 Thread Mark Thomas 

On 09/08/2023 15:13, Mark Thomas wrote:

On 09/08/2023 14:45, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new c880673cff Update Servlet API for parameter error handling 
changes

c880673cff is described below

commit c880673cffad6b8884c1d2464e5a736e852e7eeb
Author: Mark Thomas 
AuthorDate: Thu Jun 15 12:15:08 2023 +0100

 Update Servlet API for parameter error handling changes


Hi all,

I have started work on implementing these changes but wanted to gather 
feedback before I go much further with the implementation work.


The key question is how much control do we want to provide over the 
behaviour if invalid parameters are encountered?


Question 1.
The default behavior will be to throw an exception. Do we want to use a 
single exception type or do we want to use specialized exceptions for 
each type of error? It is more (boilerplate) code but specialized 
exceptions allow applications to identify what went wrong without having 
to do things like parse error messages.


Question 2.
Do we want to support non-default behavior which, most likely, would be 
to ignore the invalid parameter(s) and continue processing? Assuming we 
do, do we want to make that configurable for each type of error or just 
have a single "ignoreInvalidParameters" option?



Option 1.
My current implementation is using specialized exceptions and per error 
type configuration although it is still in progress. I'm also logging 
all invalid parameters at debug level.


Option 2.
This is creating quite a lot of plumbing. I am wondering, given that 
this is essentially handling for invalid requests, whether it is worth 
it. The alternative approach I have in mind is:

- specialized exceptions for each type of error
- log errors at debug level
- no options to allow invalid parameters
- wait and see if users request configuration options as they start to
   migrate to Tomcat 11

At this point I am leaning towards option 2 while keeping the 
implementation I have so far for option 1 in a branch in case we decide 
to add it later.


Thoughts?


Based on the (lack of) feedback so far, I'm likely to start work along 
the lines of option 2 later this week.


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Update Servlet API for parameter error handling changes

2023-08-09 Thread Mark Thomas 

On 09/08/2023 14:45, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new c880673cff Update Servlet API for parameter error handling changes
c880673cff is described below

commit c880673cffad6b8884c1d2464e5a736e852e7eeb
Author: Mark Thomas 
AuthorDate: Thu Jun 15 12:15:08 2023 +0100

 Update Servlet API for parameter error handling changes


Hi all,

I have started work on implementing these changes but wanted to gather 
feedback before I go much further with the implementation work.


The key question is how much control do we want to provide over the 
behaviour if invalid parameters are encountered?


Question 1.
The default behavior will be to throw an exception. Do we want to use a 
single exception type or do we want to use specialized exceptions for 
each type of error? It is more (boilerplate) code but specialized 
exceptions allow applications to identify what went wrong without having 
to do things like parse error messages.


Question 2.
Do we want to support non-default behavior which, most likely, would be 
to ignore the invalid parameter(s) and continue processing? Assuming we 
do, do we want to make that configurable for each type of error or just 
have a single "ignoreInvalidParameters" option?



Option 1.
My current implementation is using specialized exceptions and per error 
type configuration although it is still in progress. I'm also logging 
all invalid parameters at debug level.


Option 2.
This is creating quite a lot of plumbing. I am wondering, given that 
this is essentially handling for invalid requests, whether it is worth 
it. The alternative approach I have in mind is:

- specialized exceptions for each type of error
- log errors at debug level
- no options to allow invalid parameters
- wait and see if users request configuration options as they start to
  migrate to Tomcat 11

At this point I am leaning towards option 2 while keeping the 
implementation I have so far for option 1 in a branch in case we decide 
to add it later.


Thoughts?

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[tomcat] branch main updated: Update Servlet API for parameter error handling changes

2023-08-09 Thread markt 
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
 new c880673cff Update Servlet API for parameter error handling changes
c880673cff is described below

commit c880673cffad6b8884c1d2464e5a736e852e7eeb
Author: Mark Thomas 
AuthorDate: Thu Jun 15 12:15:08 2023 +0100

Update Servlet API for parameter error handling changes
---
 java/jakarta/servlet/ServletRequest.java | 56 
 1 file changed, 56 insertions(+)

diff --git a/java/jakarta/servlet/ServletRequest.java 
b/java/jakarta/servlet/ServletRequest.java
index 795502f3b4..1e700dd900 100644
--- a/java/jakarta/servlet/ServletRequest.java
+++ b/java/jakarta/servlet/ServletRequest.java
@@ -162,11 +162,25 @@ public interface ServletRequest {
  * 
  * If the parameter data was sent in the request body, such as occurs with 
an HTTP POST request, then reading the
  * body directly via {@link #getInputStream} or {@link #getReader} can 
interfere with the execution of this method.
+ * 
+ * If not already parsed, calling this method will trigger the parsing of 
the query string, POSTed form data where
+ * the request body has content type is 
application/x-www-form-urlencoded and POSTed form data where
+ * the request body has content-type multipart/form-data and 
the Servlet is configured with a
+ * {@link jakarta.servlet.annotation.MultipartConfig} annotation or a 
multipart-config element in the
+ * deployment descriptor.
  *
  * @param name a String specifying the name of the parameter
  *
  * @return a String representing the single value of the 
parameter
  *
+ * @throws IllegalStateException if parameter parsing is triggered and a 
problem is encountered parsing the
+ *   parameters including, but not limited 
to: invalid percent (%nn) encoding;
+ *   invalid byte sequence for the 
specified character set; I/O errors reading the
+ *   request body; and triggering a 
container defined limit related to parameter
+ *   parsing. Containers may provide 
container specific options to handle some or
+ *   all of these errors in an alternative 
manner that may include not throwing an
+ *   exception.
+ *
  * @see #getParameterValues
  */
 String getParameter(String name);
@@ -175,9 +189,23 @@ public interface ServletRequest {
  * Returns an Enumeration of String objects 
containing the names of the parameters
  * contained in this request. If the request has no parameters, the method 
returns an empty
  * Enumeration.
+ * 
+ * If not already parsed, calling this method will trigger the parsing of 
the query string, POSTed form data where
+ * the request body has content type is 
application/x-www-form-urlencoded and POSTed form data where
+ * the request body has content-type multipart/form-data and 
the Servlet is configured with a
+ * {@link jakarta.servlet.annotation.MultipartConfig} annotation or a 
multipart-config element in the
+ * deployment descriptor.
  *
  * @return an Enumeration of String objects, 
each String containing the name
  * of a request parameter; or an empty 
Enumeration if the request has no parameters
+ *
+ * @throws IllegalStateException if parameter parsing is triggered and a 
problem is encountered parsing the
+ *   parameters including, but not limited 
to: invalid percent (%nn) encoding;
+ *   invalid byte sequence for the 
specified character set; I/O errors reading the
+ *   request body; and triggering a 
container defined limit related to parameter
+ *   parsing. Containers may provide 
container specific options to handle some or
+ *   all of these errors in an alternative 
manner that may include not throwing an
+ *   exception.
  */
 Enumeration getParameterNames();
 
@@ -186,11 +214,25 @@ public interface ServletRequest {
  * null if the parameter does not exist.
  * 
  * If the parameter has a single value, the array has a length of 1.
+ * 
+ * If not already parsed, calling this method will trigger the parsing of 
the query string, POSTed form data where
+ * the request body has content type is 
application/x-www-form-urlencoded and POSTed form data where
+ * the request body has content-type multipart/form-data and 
the Servlet is configured with a
+ * {@link jakarta.servlet.annotation.MultipartConfig}