Modernize request parameter handling #641

[d-maurer]

Adresses #641.

From CHANGES.rst:

• fully recursive aggregation which can handle structures of arbitrary depth

• simplified processing model

• support for special HTML5 features: _charset_ informs
  about the used form encoding; character references used
  to work around encoding limitations

• treats parameter values internally as text
  (this means unicode for Python 2).
  For Python 2, the conversion to unicode is skipped if it
  results in a UnicodeDecodeError. The value is then used
  as is.

  For Python 2,
  the final values for parameters without converter and encoding directive
  are encoded with Zope's default encoding; character references
  are used for characters which cannot be encoded.

• Errors encountered during request parameter processing are
  not reported immediately (at this stage, application specific
  error handling has not yet been set up). Instead, a
  post_traverse is registered which will raise
  a RequestParameterError exception after the traversal
  in the proper application context.
  The RequestParameterError describes all errors
  encountered during request parameter processing.

• FileUpload has new attributes type (the associated
  MIME type or None) and the dict type_options
  (containing the provided MIME type parameters).

Backward incompatibilities:

• a parameter must now follow a corresponding default parameter
  to override the default paramter; formerly the relative order
  of parameter and default parameter was of no importance.

  There is a new directive "conditional" which can also be used
  to define a default value. A conditional parameter is ignored,
  if there is already a corresponding parameter, and
  otherwise acts like a default parameter. Thus, its behaviour
  is comparable to the former bahaviour of "default".

• aggregators are now applied from left to right; especially,
  their relative order is important.
  Formerly, aggregators were applied in a fixed order --
  independent of the order in which they were specified.

• the converter functions (in ZPublisher.Converters)
  no longer support the conversion of files (because they
  do not know the encoding applicable for the file).
  The converter directives, however, can still be applied
  to (uploaded) files. They use the encoding explicitly
  specified via an encoding directive or fall back to
  Zope's default encoding.

Fixes #638, #634, #558.

Due to the backward incompatibilities, we might want to wait until Zope 5. In this case, and if Zope 5 drops Python 2 support, some Python 2 complexities could be removed.

[icemac]

@d-maurer Wow, this one is huge! Thank you for tackling these issues.
The current Zope roadmap says that Zope 4 will only a short term release. So there should be no 4.x feature releases. On the other hand Zope 5 should only drop Python 2 compatibility thus code successfully running on Zope 4 on Python 3 should also run without modifications on Zope 5. This means I'd like to postpone this PR until Zope 5 is released for the Zope 5.1 release.
Is this okay for you?

[d-maurer]

Michael Howitz wrote at 2019-6-12 23:01 -0700:

@d-maurer Wow, this one is huge! Thank you for tackling these issues. The current [Zope roadmap](https://blog.gocept.com/2019/05/10/zope-roadmap/) says that Zope 4 will only a short term release. So there should be no 4.x feature releases. On the other hand Zope 5 should only drop Python 2 compatibility thus code successfully running on Zope 4 on Python 3 should also run without modifications on Zope 5. This means I'd like to postpone this PR until Zope 5 is released for the Zope 5.1 release. Is this okay for you?

Okay. But it would imply that a minor release (5.1) would introduce backward incompatibilities. Would it not be better to have those incomoatibilities in a major release?

…

-- Dieter

[dataflake]

Okay. But it would imply that a minor release (5.1) would introduce backward incompatibilities. Would it not be better to have those incomoatibilities in a major release?

I'd agree generally, but that pushes it out even further.

We already have internal agreement that we want to make the switch from 4.x to 5.0 as easy as possible so that there is no good reason to stick with 4.x. Release 5 will be purged of any Python 2 workarounds and thus will be a much easier platform to maintain going forward. But that means no new incompatibilities in release 5.0, so this PR cannot go into it.

Michael is suggesting 5.1 as a compromise so this won't have to wait all the time for a release 6, which has no clear time line at the moment.

[gforcada]

just an idea: what about wrapping the code with an environment variable/configuration option/something of the sorts, so that it can be merged right away and be enabled as needed.

Together with some deprecation warnings might be enough signaling to encourage users to enable the new behavior.

[d-maurer]

Gil Forcada Codinachs wrote at 2019-6-13 08:36 -0700:

*just an idea:* what about wrapping the code with an environment variable/configuration option/something of the sorts, so that it can be merged *right away* and be enabled as needed.

I could publish the new request parameter handling in a `dm.zopepatches.*` package. This way, anyone interested could use it right now -- independent on any Zope release decision. However, there may be no need to hasten: The request parameter features have 3 use cases: 1. support the calling of (a bit more complex) functions/scripts over the web, e.g. for testing/debugging purposes 2. providing elementary form support 3. act as a deserializer for the serializers of `ZTUtils` (i.e. `make_query` and `make_hidden_input`). The current features work sufficiently well for simple cases (only converters). This is sufficient for 1.: you would not want to manually construct complex parameters. For form support, you would nowadays use a form subframework (e.g. `z3c.form`, `zope.formlib`). Such a framework gives much better (easier to understand, better user feedback in case of errors, more boiler plate, ...) support. Remains 3. Again: `make_query/make_hidden_input` works for simple cases. It can utterly fail for more complex structures, but few people need to pass on complex objects across requests. My personal interest is in 3: for a customer, I have extended `make_query/make_hidden_query` (--> `dm.zopepatches.ztutils`) to improve its ability to pass on more complex values. However, it will still fail in some cases (e.g. inhomogenous lists of records); at the moment, however, it is sufficient for my customer's needs. I am also interested that software behaves as its documentation suggests. When e.g. the Zope Developer Guide promises that I can use ":default" to define a default value, then I expect that this works also in a more complex case such as `x.a:default:records` (which currently behaves in a strange way). Having fixed the tests which broke due to my changes, I found out that `x:default=2&x:default=3&x=1` currently results in `x = ['1', '2', '3']` and was highly surprised that this was deemed acceptable behaviour for `:default`. Apparently, some current tests have not been used to verify that the implementation behaves as promised but rather to document its (sometimes strange) behaviour. But Zope has lived with the discrepancy for years, there should be no problem to extend this for a few more months.

[icemac]

I think a feature toggle makes the code unnecessary complex as it requires to keep both variants. I find another dm.zopepatches.* package okay. We could add a ticket here for Zope 5.1 or even Zope 6 to reintegrate the package into Zope.

@d-maurer Would this be okay for you?

[d-maurer]

Michael Howitz wrote at 2019-6-20 23:20 -0700:

I think a feature toggle makes the code unnecessary complex as it requires to keep both variants. I find another `dm.zopepatches.*` package okay. We could add a ticket here for Zope 5.1 or even Zope 6 to reintegrate the package into Zope. @d-maurer Would this be okay for you?

I will postpone the publication of a `dm.zopepatches` package for this until this should become necessary -- for the following reason: The `dm.zopepatches` use monkey patching to change Zope code. Most of the changes in this PR are easily achievable via monkey patching (essentially replacing `ZPublisher.HTTPRequest.HTTPRequest.processInputs`). There is however one important exception: postponing the exception reports via `post_traverse` (to get them in the application context) requires a changed handling of `BaseRequest._post_traverse`. Currently, this attribute is set at the start of `BaseRequest.traverse` and deleted at its end. `post_traverse` is therefore currently not available for `processInputs`; to change this, `BaseRequest.traverse` needs to change. But this is a very complex method; I would hate to have a (modified) copy of it in my patch. It might be possible to avoid changing `BaseRequest.traverse` by a nontrivial descriptor for `_post_traverse` but I do not yet know the details. If you agree, I start working on "#619" based on the branch "modernize_request_parms#641", because the new request parameter handling makes `ZTUtils` easier, more reliable and more flexible (my primary aim in implementing the new parameter handling).

[dataflake]

This is a great candidate for Zope 6

[d-maurer]

I ask myself whether I should resume work on this PR.

Part of it has already arrived in the release: sanitize "taint" logic

2 parts remain:

1. postpone request parameter error reports until after traversal (to report them via the "correct" error_log)
2. more general but backward incompatible request parameter control

Currently, I think 1. could still be beneficial.
Regarding 2., developers with sophisticated need for request parameter control likely will use a form library (e.g. z3c.form or zope.formlib) rather than enhanced ZPublisher requst parameters. In view of this 2. is likely not worth the trouble caused by the backward incompatibility.
What do you think?

[dataflake]

I agree.

[d-maurer]

Most parts of this PR are now available in master. Still missing are:

• generalized (and more consistent) request parameters
• better reports for errors during request parameter processing.
  Both would lead to backward incompatibilities. Likely, this is not worth it.

[icemac]

@d-maurer Is the branch still needed or can it be deleted?

[d-maurer]

Michael Howitz wrote at 2024-11-7 23:51 -0800:

@d-maurer Is the branch still needed or can it be deleted?

It might still contain one interesting element: delaying the report of errors during parameter request processing until after the traversal. Recently, we have got an error report concerning the handling of such errors: they have not been handled at all by `Products.SiteErrorLog`. This problem is meanwhile fixed but it seems unnatural that such errors are reported to the top level `error_log` rather than that selected after traversal. Implementing the delay is not trivial because it may want to report not a single but multiple exceptions. The approach in the branch above may not yet be optimal. Should we not be interested in delaying the parameter request processing error reports, the branch can go away.