search

ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
16 stars 29 forks source link

Allow plugins to use settings utilities #513

Closed chris-allan closed 7 months ago

chris-allan commented 8 months ago

There are various settings utilities included in the omeroweb.settings module that would be useful to use in plugins. Unfortunately, because of import order and plugin settings processing that's made impossible if they are in the same package. The LeaveUnset functionality is of particular interest. This moves them into omeroweb.utils so they can be if desired.

chris-allan commented 8 months ago

Since plugin.settings are processed during the first import of omeroweb.settings, yes, kind of. It's not that it affects the order it's just not possible at all due to the calling context and our use of __import__ in process_custom_settings(). Similar to having a modules a and b which both import things from each other where you would get something like:

ImportError: cannot import name '...' from partially initialized module 'a' (most likely due to a circular import)

I was also finding myself re-implementing parse_boolean() in almost every plugin for the same reasons.

will-moore commented 8 months ago

Same for iviewer: https://github.com/ome/omero-iviewer/blob/aac33c0b897744fe0cf12cd6ce4500f65c7db64f/plugin/omero_iviewer/iviewer_settings.py#L23

chris-allan commented 8 months ago

why are these plugins not having the circular dependency mentioned in https://github.com/ome/omero-web/pull/513#issuecomment-1814231554 ?

Because it's not strictly a circular dependency but rather the calling context during process_custom_settings(). The semantics are weird, I can get into them if you want but honestly I wasn't for writing a whole essay here on how the processing of settings in omeroweb.settings works and what will or will not work with which implementation details. It even depends on which server type you are using.

removing these functions from omeroweb.settings will cause backwards incompatible changes for all plugins importing them.

No it will not as we're importing all the functions that were moved to omeroweb.utils. For example, from omeroweb.settings import parse_boolean will still work with the changes proposed here.

chris-allan commented 8 months ago

Same for iviewer: https://github.com/ome/omero-iviewer/blob/aac33c0b897744fe0cf12cd6ce4500f65c7db64f/plugin/omero_iviewer/iviewer_settings.py#L23

Yes, none of these examples, including those from @sbesson above, utilize the built-in CUSTOM_SETTINGS_MAPPINGS feature. There has been no need for plugin developers to perform explicit settings inclusion for a very long time but these do. I expect a bunch of plugins have been added over the years by the copy and paste method where the legacy features were used so we are where we are. The aforementioned plugins even use Django 1.x style urls.py.

sbesson commented 8 months ago

For completeness, CUSTOM_SETTINGS_MAPPINGS is the recommended way for OMERO.web app developers to declare their own settings - https://omero.readthedocs.io/en/stable/developers/Web/CreateApp.html#app-settings so there is definitely value in fixing these imports.

@will-moore I found no OME web app using this settings layout. I assume as a proof of concept, it should be possible to update either omero-signup and/or idr-gallery settings to use CUSTOM_SETTINGS_MAPPINGS. I suspect that without this change, the omeroweb.settings import will fail as described by @chris-allan in https://github.com/ome/omero-web/pull/513#issuecomment-1814231554 but with this OMERO.web PR included, the settings should be functional.