nexedi / erp5

/cc @bminusl @jerome

To me that looks OK

• status code: I guess we always get 500 here, but that does not look correct and we should set status with lock, like in erp5_hal_json_style's Base_redirect

If we have to do:

response.setBody(something, lock=True)
response.setStatus(whatever, lock=True)

to be able to control the status code this looks ok I think.

• error log: shouldn't we avoid filling the error log (and waste time generating a error page), which is doable by raising Redirect (which anyway imply to set status explicitly, since a redirect status is clearly wrong)

Maybe we could introduce a special exception ( something like AbortedTransactionResponse ) and list it in the default "Ignored exception types" from error log ?

As I wrote above, implementation details are off-topic in this MR. I just want to know the expected behaviour from the point of view of the user agent.

I've just updated the description to ask about a 3rd point: content type.

How about this:

• status code:
  • if response.setStatus(status, lock=True) was called, status code is status
  • otherwise if exception is zcException.Redirect it's 302
  • otherwise it can be 50X (it's 503 with medusa).
• error log (more generally logs): if exception type is marked as ignored in error_log, don't record exception in error_log neither in log file (ZServer don't put in log files, but I feel it would be OK to put in log file anyway).
• content type: when setBody(lock=True) was used it's should be handled the same way as if something has been written to the response, ie. don't change content-type header. But we should keep that behaviour that displaying the default error page should reset content type to html when nothing was written to response.

Please no discussion about implementation.

The commit shows 4 cases that need to be clarified:

• 2 are about a erp5_toolbox feature I don't know, and no idea why there's a raise ValueError("Abort Transaction"). But it looks like they're success cases anyway and the status should be 200. From their name, I also guess that the content types for BusinessTemplate_getDiffWithZODBAsHTML and BusinessTemplate_getDiffWithZODBAsText should text/html and text/x-diff respectively.
• 2 are about import of categories and you know more than me about this. Probably 200 for both, and maybe setBody can guess the content type (text/html ?).

IOW, it looks like that among the 4 cases, there isn't any server-side error, and nothing should go to the error log.

Once we know what we want to return to the user agent, implementing with the current workarounds is trivial. In parallel, improving the API to get rid of the workarounds can't be done as long as we haven't defined a good API to stream data (no really the same thing, but some parts are implemented in the same place and I don't want to rewrite them twice).

You seem impatient so, well, the plan is simply to be able to do:

  raise Success(body)

To prevent the transaction from being committed, one first do transaction.doom()

No more use of setStatus or setBody.

I did not know about this erp5_toolbox either, but I think I know the background story. At some point Tristan was working on extracting the local changes on Nexedi ERP5, so he wanted to use business template export to compare the changes in the git working copy. Then he realised that every time we use business template export, some objects in business templates ( probably the *TemplateItem objects ) where modified and this was using lots of disk space for ZODB, that's why he started to use this trick to prevent the transaction to be committed and to prevent the disk usage. But yes, for these two cases, It seems we want to reply with 200 OK, content type text/html and text/x-diff (or text/plain), don't log error and of course don't commit transaction.

The import of categories takes a category spreadsheet as input and create categories. It has a so called "simulation mode" in which the transaction is aborted and an ERP5 form is rendered with the list of problems or created/modified categories. So it's again a case where we want 200 OK, content-type text/html, no error logged and don't commit transaction.