Rdef query checks

[kkoz]

Add a decorator validate_rdef_query which checks the validity of the query parameters supplied to a decorated endpoint. Decorated endpoints will be require the presence and (to a degree) correctness of rendering def-related query parameters or the client will receive a 400 error with a description of how to fix the request. Tries to manage some of the issues raised in #440. For backwards compatibility, two new endpoints /webgateway/render_image_rdef and /webgateway/render_image_region_rdef are created and the new decorator applied to them. Old endpoints have no behavior change.

[kkoz]

Discussed the handling of endpoints without rendering query parameters. In order to preserve the functionality while preventing the caching issue, we are going to have endpoints where query parameters are not supplied get the current rendering settings, then redirect to the same endpoint but with rendering query parameters fully supplied. This way, the user will not experience caching issues, and the url will describe what rendering settings the user is seeing.

[sbesson]

Assuming it's not too cumbersome, the redirection layer feels like an interesting approach that preserves some backwards-compatibility while internally construct query parameters inline with our recommendations.

As we work through this use case, this might be an opportunity to review https://omero.readthedocs.io/en/stable/developers/Web/WebGateway.html#rendering-settings and define more specifically what "current rendering settings" means. In particular, in a multi-user scenario where both the viewer is not the image owner and has her/his own rendering settings, which ones are expected to be considered as current?

[will-moore]

This is working pretty nicely for me. Redirects from render_image/ID/ etc to render_image/ID/?c=... etc looks good. 👍

[will-moore]

I remembered that I am actually using the /render_image/ID/ url (without rendering parameters) in https://github.com/will-moore/parade-crossfilter/ so I checked how this PR affects performance, since that app tends to load a lot of images...

Unfortunately, with this PR, every redirect adds quite a bit of time to the loading of images (this is testing from home on dev web-server so times are slower anyway, but the effect is the same):

without this PR:

with this PR:

So it still feels like this is a significantly worse experience for users of that functionality, and we should still support this functionality if users want it (and are happy to accept any potential caching issues)

[will-moore]

There's a bunch of failing tests that are likely due to this PR at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/1367/

[sbesson]

At a glance, most of the integration tests are failing because the expectation was to receive a 200 HTTP error code previously instead of 302 right?

I am a bit more concerned by the performance overhead associated with the redirection layer. How easy is it to profile the code to identify where the extra time is spend?

[will-moore]

I think the redirect is slower simply because it makes 2 calls to load the image instead of 1. In both calls the rendering engine has to be initialised to load the rendering settings. The actual loading of the jpeg appears slower in my screenshots above, but I don't think that it's a real difference - more likely due to the variable bandwidth of home internet.

[will-moore]

These checks are causing "Split View Figure" to fail, since it uses URLs like: render_image/ID/?c=1

We could fix this by supplying the full rendering settings, but I think we should probably just remove the Split View Figure from the webclient. This really pre-dates OMERO.figure and now it's not really needed. cc @jburel

[jburel]

@will-moore: removing it will make sense but note that we will need to have a similar change in insight First step will be to deprecate it

[will-moore]

@jburel It's already broken! If we deprecate it, we should release a webclient with it working (but deprecated). That would delay this PR. Or we fix it - but that's not worth it. Or we just replace the Split-View figure dialog with "Use OMERO.figure" message.

[kkoz]

@will-moore @jburel I discussed with @knabar and we think the best way to move forward here is to create new endpoints for render_image and render_image_region. These would have the rdef query validation decorators while the old endpoints would be as they were but would be marked as deprecated. We would recommend that going forward people use the new endpoints. This way there won't be any redirection performance issues to worry about, and applications currently using the old endpoints will be unaffected. We would then migrate clients to the new endpoints. Would like your thoughts or objections.

[will-moore]

@kkoz Is this simply to deal with the Split-View figure issue? I would prefer not to have render_image2 and deprecate render_image just for this, since that will create a lot of work to migrate everything. I don't think Split-View figure has been used much - it's very old and we've promoted OMERO.figure as the way to make figures for a long time. If we really can't remove Split-View figure in the next release, then create render_image_unsafe/ID/ which allows ?c=1 and mark that endpoint as deprecated.

[kkoz]

@will-moore No this has nothing to do with the Split-View figure issue. Sorry I should have mentioned that. It is more in response to the points made in our discussion this morning, in particular to preserve the behavior of the endpoint with no query parameters provided. I would still propose that we update the docs to suggest always providing the query parameters, but existing clients which want to use the "default settings" endpoint will still be able to do so without experiencing any changes.

[will-moore]

"while the old endpoints would be as they were but would be marked as deprecated" - do you mean you want to mark the current behaviour allowing /render_image/ID/ as deprecated? I still don't think that we should remove this behaviour (for all the reasons we discussed). This would mean we don't need new endpoints and don't need to migrate apps (same for other users). Certainly agree to updating the docs to describe the behaviour of this endpoint with appropriate warnings.

[will-moore]

I think if we follow the policy of "No feature removal without first deprecating" to apply to Split-View figure (as @jburel mentioned above), then the same should apply to the removal of support for render_image/ID/?c=1 which is what's breaking the Split-View figure.

In which case we need a PR to deprecate both of those features and release that before removing them?

[kkoz]

@will-moore I'd like to make sure I understand what you think the final behavior should be after all the deprecations and feature removal is complete. Let me know if the below is correct:

• We have render_image and render_image_region endpoints (no new endpoints)
• Both endpoints support either no rendering query parameters at all or a complete set (including windows and colors for all channels)

Therefore render_image/ID would be a valid endpoint but render_image/ID/?c=1 would not.

[will-moore]

Yes, that's exactly what I would want. 👍

[knabar]

@will-moore @jburel

After some additional discussion, I don’t think this last proposal on its own will do exactly what we’re looking for.

One of the main goals is for the client to be able to cache basically indefinitely for best possible performance, which is not possible if the endpoint response depends on anything other than the given parameters.

Extending the proposal to get there though should be simple and not impact any users of the current endpoints, by adding two new endpoints that wrap the current endpoints to add new behavior:

• Only support full set of rendering parameters
• Add caching headers to enable unlimited caching

Since the new endpoints would just wrap the current ones, there should be a minimal amount of new code needed, without any code duplication.

I don’t see any need to remove the old endpoints, and if there are advantages to deprecate the partial rendering parameters support, we can still do that, or just leave the endpoints as they are indefinitely.

[kkoz]

I added the new endpoints to give us a concrete discussion point.

[will-moore]

Looks good. Hopefully that should fix the failing integration tests too...

[will-moore]

We're down to just 5 failing tests now: https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/1375/testReport/ All are render_image_region, with 'tile': '0,0,0' or '0,0,0,512,512' or 'region': '0,0,10,10' or '0,0,512,512'.

[will-moore]

As discussed today - if we want to remove support for render_image/ID/?c=1 etc in future, then we could mark that as deprecated in the next release. This would be best to have in separate PR - and can be referred to in the Changelog etc. to raise awareness. The same goes for Split-View figure functionality.

[will-moore]

Description looks good, thanks. Good to merge for me!