On 23/03/2023 12:02, Konstantin Kolinko wrote:
Thanks for the continued feedback. Having someone to bounce ideas off is
really helpful.
ср, 22 мар. 2023 г. в 14:38, Mark Thomas <ma...@apache.org>:
Any more thoughts on this?
1. If we cannot agree on the required behaviour, it is one more reason
to make it configurable.
I think you are right. I have proposed a new
RequestParameterErrorListener over at
https://github.com/jakartaee/servlet/issues/431
The details aren't defined yet. I'm just looking for consensus on the
general approach at this point.
As I said, it would be more useful to configure it at a Context.
Agreed.
2. Regarding the default behaviour,
Throwing an exception was also my first thought,
and it seems more natural.
Valid cases are being made for both throwing an exception and ignoring
the error. Which is what makes this so tricky.
I'm thinking default to current behaviour but with simple options on the
context to change to other approaches.
Regarding implementation, I thought that in
org.apache.catalina.connector.Request all lines
if (!parametersParsed) {
parseParameters();
}
could be amended with "if (parseFailed) throw new
IllegalStateException(parseFailedReason)", or maybe put this "throw"
into parseParameters().
BTW, Spring Framework has a feature that routing of requests can be
configured with annotations.
https://docs.spring.io/spring-framework/docs/5.3.25/reference/html/web.html#mvc-ann-requestmapping-params-and-headers
In this case parameters parsing is hidden from the caller (done by
framework), and also a Request may be omitted from method signature
(so one wouldn't check its attributes to check for failed parsing). In
this case it makes sense to throw an exception to report a failure.
Agreed.
3. Regarding UserDataHelper,
1) If we rely on it, it means being too late.
At the time one considers reading the logs, data loss has already happened.
2) If you look at my mail regarding code paths (7th email in this thread),
if I have read the code correctly, I think that in case of
"d) Request.getParameter() was called, and request was "multipart/form-data"."
there is no logging.
That probably needs fixing.
4. Regarding the value for maxParameterCount
500 parameters may mean 100 rows of 5 values each;
100 rows may mean daily values for 3 months.
1000 parameters may mean a year of daily data with 3 values each day.
It is not what one would frequently see in practice, but it could happen.
There hasn't been much movement from the spec EG on this, so my current
thinking is to revert this change for 10.1.x and earlier to wait and see
what the Servlet EG decides.
I'm still leaning in this direction at the moment.
5. If someone is thinking about improved API,
a) I wonder whether ExtendedAccessLogValve calling of getParameter()
could be improved,
so that it does not trigger parameter parsing that includes reading
the body of a POST.
Or maybe do reading, but with a lower timeout. Essentially in the same
way as when skipping a body of a failed request.
It is not a job for an AccessLogValve to spend time on parameter parsing.
b) I wonder whether parameter parsing could be done asynchronously.
I have seen feature requests along those lines. async is going to
complicate error handling. But the error listener may help here.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
