login_required(doConnectionCleanup=False) will close conn unless streaming

[will-moore]

This PR ensures that when you to use @login_required(doConnectionCleanup=False) decorator (for methods that return a ConnCleaningHttpResponse to stream data from OMERO), if you return any other responses (e.g. Http404 or other page that doesn't need to stream from OMERO), then the @login_required decorator will call conn.close() for you.

If you previously needed to use @login_required(doConnectionCleanup=False), but ended up returning a Http404 then the connection wouldn't not get closed.

This also allows you to be more flexible and your view methods can return either a Streaming HttpResponse OR a standard HttpResponse. Previously, if you wanted to also return a non-streaming response from the same view method then you needed to make sure you called conn.close() manually.

I needed this logic for another PR (coming soon on top of this commit).

Testing:

• Need to check web logs to see when conn is closed e.g.

2020-08-12 09:00:15,207  INFO [                           omero.gateway] (proc.37498) close():1988 closed connection (uuid=410000b1-0e43-49d9-a911-6bf250750134)

• Test download of archived files will valid Image ID and with invalid ID (to get Not Found response - actually 500) /webgateway/archived_files/download/?image=12795345345 or with no query at all /webgateway/archived_files/download/
• Test original files at /webclient/get_original_file/398157/ with valid ID (e.g. open script dialog and click on View Script) and invalid ID - should get a custom 404 page (from handlerInternalError()) - check conn is closed in logs for both.

cc @chris-allan

[chris-allan]

My preference is the original style.

Yes, if you have set doConnectionCleanup to False you are responsible for closing the connection on error or if you return a different response type. I don't see having to call conn.close() a particularly difficult thing to ask and knowing the difference is important.

[will-moore]

If you wish to prevent cleanup, you can still set doConnectionCleanup to False as before and nothing will change.

All that this PR does is prevent @login_required calling conn.close() when you return an instance of ConnCleaningHttpResponse.
Is there ever a use-case where you'd want conn.close() to be called for a ConnCleaningHttpResponse?

It sounds easy to call conn.close() but we have found many cases before where we've failed to do that.
This makes it even easier to "do the right thing".

[chris-allan]

I understand what the PR does and I appreciate it may make it easier to do the right thing for your use case.

However, I don't agree that hiding connection semantics behind response class type checks makes these sorts of scenarios easier to not screw up overall. It is now just as easy to have changes in your response class hierarchy cause a scenario where you do not get your expected behaviour and then encourages people to use doConnectionCleanup=False. Equally so if you override ConnCleaningHttpResponse.close(). Dealing with streaming responses that rely on an OMERO session is very tricky to get right regardless.

[will-moore]

Hi, Apologies - This was a bit of a last-minute PR before I was away for 2 weeks...

Having revisited these changes, I realise that the removal of doConnectionCleanup=False allows
us to simplify the handling of exceptions or 404s. See commit above: 85e33b3

Previously with the use of doConnectionCleanup=False we needed to diligently return an instance of
ConnCleaningHttpResponse in every case, as in 451f959
However, in a method with many return statements, it becomes easy to miss one.
Also, this restricted what you could return, e.g. couldn't return a redirect or delegate to return handlerInternalError() as before.

Re #191 (comment): If ConnCleaningHttpResponse.close() is overridden and doesn't actually call conn.close() then
this would cause issues even without this PR.

[chris-allan]

If ConnCleaningHttpResponse.close() is overridden and doesn't actually call conn.close() then this would cause issues even without this PR.

Of course it would. My point is that there is inherent complexity here and it's not good design to pretend that there isn't. Stating doConnectionCleanup=False emphatically in the login_required() decorator declaratively states what the intent is. If you are then overriding close() on a class whose literal name is ConnCleaningHttpResponse you at least then know that close() was likely to have been required and looking at its implementation is a smart thing to do.

Trading one set of complexity (return values) for another (class hierarchy) is just that. One is better in certain circumstances, one is better in others. One is not objectively better universally.

If you want to discuss making connection handling easier in streaming response cases more widely there are lots of ways to approach that. Likely better ones than return values or class hierarchy checks. Django, our use of it, and our experience has evolved a lot in the 8 years since this code was added.

We are now conflating a fundamental platform and design change in this PR with real, useful, and desired functionality from #192. The changes in #192 do not require the changes from this PR in order to be implemented and should be proposed utilizing the style already established.

[will-moore]

8af5949 fixes exception handling as in https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/486/testReport/OmeroWeb.test.integration.test_links/TestLinks/test_link_datasets_images/

[sbesson]

Briefly discussed during standup. It might be worth looking into whether https://github.com/ome/omero-weberror can simply be extended to simplify the testing of the new functionality/contract introduced by this PR.

[will-moore]

Added tests without using weberror. See ome/openmicroscopy#6248

[joshmoore]

Looks like this may well need the test fix from ome/devspace#169

cc: @manics @sbesson

[sbesson]

I suspect we'll need to review all places where pytest-xdist is used across our codebase indeed - https://github.com/search?q=org%3Aome+pytest-xdist&type=Code

[will-moore]

Travis is failing with:

py36 run-test: commands[0] | flake8 .
./omeroweb/webgateway/views.py:2551:44: F821 undefined name 'StringIO'
ERROR: InvocationError for command /src/.tox/py36/bin/flake8 . (exited with code 1)

But that file doesn't contain StringIO anywhere?!
https://github.com/ome/omero-web/blob/056ad63740d4440fb454bc2100e0e3074aa33810/omeroweb/webgateway/views.py

[will-moore]

Hmmm - the 'Details' link above goes to https://travis-ci.org/github/ome/omero-web/builds/719232030 which is for commit 1d0e394 which isn't right.
Need to force Travis to use the correct commit!

[will-moore]

So, travis seems to be merging each commit into ba64516 before running flake8, which is then failing.

[joshmoore]

👍