feat(plugins): extend RpcDriver for BLOB, materialized views, and type mappings - #576
feat(plugins): extend RpcDriver for BLOB, materialized views, and type mappings#576aesslinger wants to merge 5 commits into
Conversation
Extend the RpcDriver to forward save_blob_to_file and fetch_blob_as_data_url to plugin processes via JSON-RPC. Plugins that implement these methods can now handle binary data export/preview. Plugins that do not implement them receive a graceful fallback via is_method_not_found (same pattern as routines, triggers).
Extend the RpcDriver to forward get_materialized_views, get_materialized_view_columns, get_materialized_view_definition, and refresh_materialized_view to plugin processes via JSON-RPC. Plugins that declare materialized_views capability can now serve these queries. Plugins without support receive graceful fallbacks via is_method_not_found (empty vec or unsupported error).
Add an optional type_mappings field to PluginManifest and ConfigManifest that maps generic inferred types (e.g. DATETIME, JSON) to driver-specific types (e.g. TIMESTAMP, JSONB). The RpcDriver now overrides map_inferred_type to consult these static mappings at lookup time. This avoids the need for an async RPC call in a synchronous trait method. Built-in drivers continue to use their direct trait overrides and declare empty mappings. Existing plugins without type_mappings are unaffected (serde default is an empty map, passthrough behavior is preserved).
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summary (commit a95a781)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a95a781)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Reviewed by laguna-s-2.1:free · Input: 649.5K · Output: 47.4K · Cached: 1.1M |
debba
left a comment
There was a problem hiding this comment.
Hey @aesslinger, thanks a lot for this one. I checked out the branch, read through the whole diff, and ran everything locally. This is really solid work: the fallback behavior is carefully aligned with the trait defaults, the tests verify both the exact payloads sent over RPC and the -32601 fallback paths, and the decision to resolve map_inferred_type() locally through the manifest (since the trait method is synchronous and can't issue an RPC call) is exactly the right call. I also confirmed the forwarding is actually reachable, since plugins can declare materialized_views: true in their capabilities. Full suite is green on the branch for me: 1051 passed, 0 failed.
I also built a small test plugin implementing the new methods (plus a "legacy" variant that returns -32601) and drove it over JSON-RPC stdio with the exact payloads the RpcDriver sends. Everything behaves as advertised, including the graceful degradation for old plugins. Nice job on backward compatibility.
A few things I'd like to see addressed before merge, none of them about the code itself:
-
plugins/manifest.schema.jsonneeds the new field. The schema hasadditionalProperties: falseat the top level, so a plugin declaringtype_mappingsin its manifest will currently fail validation in the registry tooling. Adding the property to the schema should be enough. -
PLUGIN_GUIDE.mdshould document the new surface. The six new RPC methods and thetype_mappingsmanifest field aren't mentioned yet. Since this PR is the prerequisite for the PostgreSQL plugin migration, the guide is the public contract plugin authors will rely on. -
The description doesn't match the code for
save_blob_to_file. The PR says "plugins return base64-encoded data; the host handles file I/O", but the implementation forwardsfile_pathto the plugin and discards the result, so it's the plugin that writes the file. That's a perfectly fine design given the plugin runs on the same machine, but the description (and ideally the guide) should state the actual contract so plugin authors implement the right thing.
Two small nits, take them or leave them: get_materialized_view_columns is the only new method without a fallback test, and the TS PluginManifest interface in src/types/plugins.ts could gain an optional type_mappings field for completeness (the frontend doesn't consume it today, so no urgency). Also a tiny note on the description: it mentions 21 new tests, but I count 13, the other 8 in that filter are pre-existing.
Overall: approve once the schema and docs are updated. Really happy to see the plugin RPC surface reaching parity, this unblocks a lot.
1. manifest.schema.json: add type_mappings field (fixes registry validation for plugins declaring this field) 2. PLUGIN_GUIDE.md: document all 6 new RPC methods (materialized views, BLOB operations) and the type_mappings manifest field with examples and contracts 3. Correct BLOB contract documentation: the plugin writes the file directly (it receives file_path), not the host 4. Add missing fallback test for get_materialized_view_columns (was the only new method without a -32601 fallback test) 5. Add type_mappings to TS PluginManifest interface in src/types/plugins.ts for frontend type completeness 6. Test count: 22 plugin driver unit tests (was 21, +1 new fallback)
|
Hey @debba, thanks for the thorough review and for actually spinning up a test plugin to verify the behavior — that's awesome. All 6 items addressed in the latest push: 1. 2.
3. Fixed the BLOB contract description — Both the PR description and the PLUGIN_GUIDE now correctly state that the plugin receives 4. Added the missing fallback test — 5. TS interface updated — 6. Test count corrected — 22 plugin driver unit tests total (13 new in this PR + 9 pre-existing that were in the filter). Updated the PR description. Let me know if there's anything else you'd like adjusted. |
|
LGTM, @NewtTheWolf is there something related to tabularium? |
Extends the plugin RPC adapter to forward three method categories that were previously unsupported, enabling full feature parity for plugin drivers.
Ref #16
Changes
1. BLOB Methods Forwarded
save_blob_to_fileandfetch_blob_as_data_urlare now forwarded to plugin drivers via JSON-RPC. The plugin receives the file path and is responsible for writing the file directly (since it runs on the same machine as the host). Plugins that don't implement these methods return-32601and the host falls back to the existing "not supported" error (no behavioral change for existing plugins).2. Materialized View Methods Forwarded
get_materialized_views,get_materialized_view_columns,get_materialized_view_definition, andrefresh_materialized_vieware now forwarded. Same fallback pattern as above.3.
type_mappingsManifest FieldAdds an optional
type_mappings: HashMap<String, String>field toPluginManifest. TheRpcDriveruses this to resolvemap_inferred_type()locally (this method is synchronous and cannot issue an RPC call). Example: a PostgreSQL plugin declares{"DATETIME": "TIMESTAMP", "JSON": "JSONB"}.Documentation Updates
plugins/manifest.schema.json:type_mappingsfield added (fixes validation for plugins using it)plugins/PLUGIN_GUIDE.md: All 6 new RPC methods andtype_mappingsfully documented with params, results, and contractssrc/types/plugins.ts:type_mappingsadded to TSPluginManifestinterfaceContext
These are prerequisite changes for the PostgreSQL plugin migration initiative. Without them, a plugin driver cannot achieve full feature parity with the built-in PostgreSQL driver for BLOB handling, materialized view management, and type inference during paste/import operations.
Testing
cargo test)pnpm tsc --noEmit)-32601fallback)