-
-
Notifications
You must be signed in to change notification settings - Fork 956
Pluggable sandbox handler architecture for content viewer plugins #15036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
9fccc76
e327111
acb15ba
cb79952
25a58b8
5a4d2a8
2a8985d
30fd3f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,9 +107,11 @@ js-dist | |
| storage/* | ||
| kolibri/content/content_db/*.sqlite3 | ||
| kolibri/core/content/contentschema/migrations/* | ||
| # Check in h5p & bloom specific files | ||
| !kolibri/core/content/static/h5p/ | ||
| !kolibri/core/content/static/bloom/ | ||
| # Check in h5p & bloom specific files in their respective plugins | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. nitpick: the output directory still isn't ignored, and this hunk doesn't match its commit message.
|
||
| !kolibri/plugins/h5p_viewer/static/h5p/ | ||
| !kolibri/plugins/bloompub_viewer/static/bloom/ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion:
One line next to the existing h5p/bloom entries closes both. |
||
| # H5P PHP library unpacked by build-h5p; only its build output is checked in | ||
| kolibri/plugins/h5p_viewer/h5p_build/vendor/ | ||
|
|
||
| # virtual environment | ||
| venv/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,31 +2,39 @@ | |
| Kolibri Content hooks | ||
| --------------------- | ||
|
|
||
| Hooks for managing the display and rendering of content. | ||
| Hooks for managing the display and viewing of content. | ||
| """ | ||
|
|
||
| import json | ||
| import logging | ||
| from abc import abstractmethod | ||
|
|
||
| from django.conf import settings | ||
| from django.core.serializers.json import DjangoJSONEncoder | ||
| from django.utils.safestring import mark_safe | ||
| from importlib_resources import files | ||
| from le_utils.constants import file_formats | ||
| from le_utils.constants import format_presets | ||
|
|
||
| from kolibri.core.content.utils.paths import zip_content_static_root | ||
| from kolibri.core.utils.urls import join_url | ||
| from kolibri.core.webpack.hooks import WebpackBundleHook | ||
| from kolibri.core.webpack.hooks import WebpackError | ||
| from kolibri.core.webpack.hooks import WebpackInclusionMixin | ||
| from kolibri.plugins.hooks import define_hook | ||
| from kolibri.plugins.hooks import KolibriHook | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @define_hook | ||
| class ContentRendererHook(WebpackBundleHook, WebpackInclusionMixin): | ||
| class ContentViewerHook(WebpackBundleHook, WebpackInclusionMixin): | ||
| """ | ||
| An inheritable hook that allows special behaviour for a frontend module that defines | ||
| a content renderer. | ||
| a content viewer. | ||
| """ | ||
|
|
||
| #: Set tuple of format presets that this content renderer can handle | ||
| #: Set tuple of format presets that this content viewer can handle | ||
| @property | ||
| @abstractmethod | ||
| def presets(self): | ||
|
|
@@ -63,28 +71,36 @@ def html(cls): | |
| tags.append(hook.template_html()) | ||
| return mark_safe("\n".join(tags)) | ||
|
|
||
| def template_html(self): | ||
| @property | ||
| def viewer_data(self): | ||
| """ | ||
| Generates template tags containing data to register a content renderer. | ||
| Data registering this content viewer with the frontend. | ||
|
|
||
| :returns: HTML of a template tags to insert into a page. | ||
| :returns: dict serialized into this viewer's template tag. | ||
| """ | ||
| # Note, while most plugins use sorted chunks to filter by text direction | ||
| # content renderers do not, as they may need to have styling for a different | ||
| # content viewers do not, as they may need to have styling for a different | ||
| # text direction than the interface due to the text direction of content | ||
| urls = [chunk["url"] for chunk in self.bundle] | ||
| return { | ||
| "urls": [chunk["url"] for chunk in self.bundle], | ||
| "presets": self.presets, | ||
| "css_selectors": self.all_css_selectors(), | ||
| } | ||
|
|
||
| def template_html(self): | ||
| """ | ||
| Generates template tags containing data to register a content viewer. | ||
|
|
||
| :returns: HTML of a template tags to insert into a page. | ||
| """ | ||
| tags = ( | ||
| self.frontend_message_tag() | ||
| + self.plugin_data_tag() | ||
| + [ | ||
| '<template data-viewer="{bundle}">{data}</template>'.format( | ||
| bundle=self.unique_id, | ||
| data=json.dumps( | ||
| { | ||
| "urls": urls, | ||
| "presets": self.presets, | ||
| "css_selectors": self.all_css_selectors(), | ||
| }, | ||
| self.viewer_data, | ||
| separators=(",", ":"), | ||
| ensure_ascii=False, | ||
| cls=DjangoJSONEncoder, | ||
|
|
@@ -95,6 +111,120 @@ def template_html(self): | |
| return mark_safe("\n".join(tags)) | ||
|
|
||
|
|
||
| # Backwards compatibility alias | ||
| ContentRendererHook = ContentViewerHook | ||
|
|
||
|
|
||
| @define_hook | ||
| class SandboxedContentViewerHook(ContentViewerHook): | ||
| """ | ||
| A content viewer that uses the Kolibri sandbox with a dynamically loaded handler. | ||
|
|
||
| Subclasses must define: | ||
| - bundle_id: The main viewer bundle ID (inherited from WebpackBundleHook) | ||
| - presets: Tuple of format presets this viewer handles (inherited from ContentViewerHook) | ||
| - sandbox_handler_id: The bundle ID of the sandbox handler | ||
|
|
||
| The sandbox handler is built separately with no Kolibri externals and loaded | ||
| dynamically into the sandbox iframe at runtime. | ||
| """ | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def sandbox_handler_id(self): | ||
| """ | ||
| Bundle ID of the sandbox handler. | ||
| This should match a bundle defined in buildConfig.js with sandbox_handler: true | ||
| """ | ||
| pass | ||
|
|
||
| @property | ||
| def sandbox_static_path(self): | ||
| """ | ||
| Returns the filesystem path to the plugin's static directory. | ||
| """ | ||
| return str(files(self._module_path).joinpath("static")) | ||
|
|
||
| @classmethod | ||
| def get_sandbox_static_paths(cls): | ||
| """ | ||
| Returns a list of filesystem paths to static directories | ||
| that should be mounted on the sandbox server. | ||
|
|
||
| Includes: | ||
| - Core content static directory (kolibri/core/content/static) | ||
| - Plugin static directories for each registered sandbox handler | ||
| """ | ||
| core_static_path = str(files("kolibri.core.content").joinpath("static")) | ||
| return [core_static_path] + [ | ||
| hook.sandbox_static_path for hook in cls.registered_hooks | ||
| ] | ||
|
|
||
| @property | ||
| def sandbox_handler_unique_id(self): | ||
| """Full unique ID for the sandbox handler bundle.""" | ||
| return "{}.{}".format(self._module_path, self.sandbox_handler_id) | ||
|
|
||
| def _get_sandbox_handler_stats(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion: stats are re-read and re-parsed from disk on every page render. The parent caches the equivalent read ( (The unconditional |
||
| """Load stats file for the sandbox handler bundle.""" | ||
| developer_mode = getattr(settings, "DEVELOPER_MODE", False) | ||
| if hasattr(self, "_cached_sandbox_handler_stats") and not developer_mode: | ||
| return self._cached_sandbox_handler_stats | ||
|
|
||
| try: | ||
| stats = json.loads( | ||
| files(self._module_path) | ||
| .joinpath("build") | ||
| .joinpath("{}_stats.json".format(self.sandbox_handler_unique_id)) | ||
| .read_text() | ||
| ) | ||
| except OSError as e: | ||
| raise WebpackError( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. blocking: This raises Beyond CI, any packaged build that ships a registered Scope the fail-loud to development, matching the original suggestion: raise under |
||
| "Error accessing sandbox handler stats file '{}': {}".format( | ||
| self.sandbox_handler_unique_id, e | ||
| ) | ||
| ) | ||
|
|
||
| self._cached_sandbox_handler_stats = stats | ||
| return stats | ||
|
|
||
| @property | ||
| def sandbox_handler_url(self): | ||
| """URL to the built sandbox handler JavaScript file.""" | ||
| stats = self._get_sandbox_handler_stats() | ||
| chunks = stats.get("chunks", {}).get(self.sandbox_handler_unique_id, []) | ||
|
|
||
| for chunk in chunks: | ||
| name = chunk.get("name", "") | ||
| if name.endswith(".js"): | ||
| relpath = "{}/{}".format(self.sandbox_handler_unique_id, name) | ||
| if getattr(settings, "DEVELOPER_MODE", False): | ||
| url = chunk.get("publicPath") | ||
| if url and not url.startswith("auto"): | ||
| return url | ||
| # The handler <script> is loaded inside the sandbox iframe, which | ||
| # is served from the alternate (zip content) origin. Serve the | ||
| # handler from that origin's static root — where alt_wsgi mounts | ||
| # the plugin static dirs — not the main-origin STATIC_URL, which | ||
| # 404s when resolved against the iframe's origin. | ||
| return join_url(zip_content_static_root(), relpath) | ||
|
|
||
| return None | ||
|
|
||
| @property | ||
| def viewer_data(self): | ||
| """ | ||
| Extends the base payload with the sandbox handler URL, when built. | ||
| """ | ||
| # `define_hook` rebuilds the class through KolibriHookMeta, so the class | ||
| # zero-argument `super()` closes over is not in the instance's MRO. | ||
| data = super(SandboxedContentViewerHook, self).viewer_data | ||
| handler_url = self.sandbox_handler_url | ||
| if handler_url: | ||
| data["sandboxHandlerUrl"] = handler_url | ||
| return data | ||
|
|
||
|
|
||
| @define_hook | ||
| class ContentNodeDisplayHook(KolibriHook): | ||
| """ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved — addressed in the current code.
nitpick: the artifact is uploaded on every caller, but only the release consumes it.
build_whl.ymlis called fromrelease_kolibri.yml,pr_build_kolibri.ymlandwarm_build_cache.yml. The rationale in the step comment above — surfacing a breakage in PR CI rather than mid-release — is served by the collect step; the upload only matters for the release path. Gating it (if: github.event_name == 'release', or an input from the caller) keeps the early-warning property without storing the tree on every PR push and everydevelopbuild.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree - making sure the full collect + upload works is important. Leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood — deferring to your call. Withdrawing the nitpick.