-
Notifications
You must be signed in to change notification settings - Fork 3
Enable automatic plugin registration in middleware #930
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: main
Are you sure you want to change the base?
Changes from 16 commits
0c9c396
77af043
82a6996
12330aa
a6aaed5
8eca04c
a7dd05b
8340f71
24aec9b
5d798b8
de58fee
f182b93
588e255
7395044
ca6da37
ed77e66
ad6b212
10225e1
03b6fea
8dd9ab9
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 |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| #include <string> | ||
| #include <vector> | ||
| #include <cstddef> | ||
| #include <set> | ||
|
|
||
| #include <unordered_map> | ||
|
|
||
|
|
@@ -429,6 +430,7 @@ | |
| namespace parser | ||
| { | ||
| class Command; | ||
| typedef std::shared_ptr<Command> command_ptr; | ||
|
Check warning on line 433 in native/c/extend/plugin.hpp
|
||
| } // namespace parser | ||
|
|
||
| namespace plugin | ||
|
|
@@ -826,36 +828,30 @@ | |
|
|
||
| void print(const char *s) const | ||
| { | ||
| if (m_output_stream && s) | ||
| if (s) | ||
| { | ||
| m_output_stream->write(s, std::strlen(s)); | ||
| output_stream().write(s, std::strlen(s)); | ||
|
Comment on lines
+831
to
+833
Member
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. Thanks for fixing #929 🙏 |
||
| } | ||
| } | ||
|
|
||
| void println(const char *s) const | ||
| { | ||
| print(s); | ||
| if (m_output_stream) | ||
| { | ||
| m_output_stream->put('\n'); | ||
| } | ||
| output_stream().put('\n'); | ||
| } | ||
|
|
||
| void err(const char *e) const | ||
| { | ||
| if (m_error_stream && e) | ||
| if (e) | ||
| { | ||
| m_error_stream->write(e, std::strlen(e)); | ||
| error_stream().write(e, std::strlen(e)); | ||
| } | ||
| } | ||
|
|
||
| void errln(const char *e) const | ||
| { | ||
| err(e); | ||
| if (m_error_stream) | ||
| { | ||
| m_error_stream->put('\n'); | ||
| } | ||
| error_stream().put('\n'); | ||
| } | ||
|
|
||
| void to_err(const std::stringstream &sstr) | ||
|
|
@@ -1133,6 +1129,7 @@ | |
| const CommandDefaultValue *defaultValue) = 0; | ||
| virtual void set_handler(CommandHandle command, CommandHandler handler) = 0; | ||
| virtual void add_subcommand(CommandHandle parent, CommandHandle child) = 0; | ||
| virtual void add_to_server(CommandHandle command) = 0; | ||
| }; | ||
|
|
||
| virtual ~CommandProviderImpl() | ||
|
|
@@ -1200,6 +1197,11 @@ | |
| return m_plugins; | ||
| } | ||
|
|
||
| const std::set<parser::command_ptr> &get_server_commands() const | ||
| { | ||
| return m_server_commands; | ||
| } | ||
|
|
||
| PluginManager(const PluginManager &) = delete; | ||
| PluginManager &operator=(const PluginManager &) = delete; | ||
|
|
||
|
|
@@ -1210,6 +1212,7 @@ | |
| void discard_command_providers_from(std::size_t start_index); | ||
|
|
||
| std::vector<std::unique_ptr<CommandProvider>> m_command_providers; | ||
| std::set<parser::command_ptr> m_server_commands; | ||
| std::vector<LoadedPlugin> m_plugins; | ||
| PluginMetadata m_pending_metadata; | ||
| bool m_metadata_pending; | ||
|
|
@@ -1268,9 +1271,8 @@ | |
| return false; | ||
| } | ||
|
|
||
| return std::any_of(m_plugins.begin(), m_plugins.end(), [&name](const auto &plugin) { | ||
| return plugin.metadata.display_name == name; | ||
| }); | ||
| return std::any_of(m_plugins.begin(), m_plugins.end(), [&name](const auto &plugin) | ||
| { return plugin.metadata.display_name == name; }); | ||
| } | ||
|
|
||
| inline void PluginManager::discard_command_providers_from(std::size_t start_index) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| /** | ||
| * This program and the accompanying materials are made available under the terms of the | ||
| * Eclipse Public License v2.0 which accompanies this distribution, and is available at | ||
| * https://www.eclipse.org/legal/epl-v20.html | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Copyright Contributors to the Zowe Project. | ||
| * | ||
| */ | ||
|
|
||
| #include "plugin_bridge.hpp" | ||
| #include "dispatcher.hpp" | ||
| #include "builder.hpp" | ||
| #include "logger.hpp" | ||
| #include <string> | ||
| #include <cctype> | ||
| #include <algorithm> | ||
|
|
||
| namespace plugin | ||
| { | ||
|
|
||
| // Forward declaration for recursion | ||
| static void traverse_and_register_impl(parser::Command *cmd, std::string &path_prefix, CommandDispatcher &dispatcher, int depth); | ||
|
|
||
| /** | ||
| * @brief Recursively traverse command tree and register to middleware | ||
| * | ||
| * @param cmd The command to process | ||
| * @param path_prefix The command path built so far (e.g., "sample.hello") | ||
| * @param dispatcher The dispatcher to register commands to | ||
| */ | ||
| static void traverse_and_register(parser::Command *cmd, const std::string &path_prefix, CommandDispatcher &dispatcher) | ||
| { | ||
| // Limit recursion depth to prevent stack issues on z/OS | ||
| std::string mutable_path = path_prefix; | ||
| traverse_and_register_impl(cmd, mutable_path, dispatcher, 0); | ||
| } | ||
|
|
||
| static void traverse_and_register_impl(parser::Command *cmd, std::string &path_prefix, CommandDispatcher &dispatcher, int depth) | ||
|
Check failure on line 40 in native/c/server/plugin_bridge.cpp
|
||
| { | ||
| if (!cmd || depth > 10) // Prevent deep recursion on z/OS | ||
| return; | ||
|
Comment on lines
+43
to
+44
Member
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. This number seems a bit arbitrary. I recall seeing (on a recent PR) that we allow up to 64 steps in a recursive function. 🤔 Have you seen problems past 10 levels? And also, is it correct to assume that a level os basically a command group? |
||
|
|
||
| // Get handler for this command | ||
| typedef plugin::CommandProviderImpl::CommandRegistrationContext::CommandHandler CommandHandler; | ||
|
Check warning on line 46 in native/c/server/plugin_bridge.cpp
|
||
| CommandHandler handler = cmd->get_handler(); | ||
|
|
||
| // Check if this command has subcommands | ||
| const std::map<std::string, parser::command_ptr> &subcommands = cmd->get_commands(); | ||
|
|
||
| if (handler != nullptr) | ||
| { | ||
| // This command has a handler, register it with dot notation | ||
| std::string rpc_command_name = path_prefix; | ||
| std::replace(rpc_command_name.begin(), rpc_command_name.end(), ' ', '.'); | ||
|
|
||
| try | ||
| { | ||
| LOG_DEBUG("Registering plugin command: %s (from path: %s)", rpc_command_name.c_str(), path_prefix.c_str()); | ||
|
|
||
| CommandBuilder builder(handler); | ||
|
|
||
| // For plugin commands, arguments from JSON are passed in camelCase by the middleware. | ||
| // Plugins typically register CLI-style argument names (kebab-case) using add_keyword_arg. | ||
| // We dynamically infer renames from the command's declared arguments. | ||
| for (const auto &arg : cmd->get_args()) | ||
| { | ||
| // Help flags and automatically generated 'no-' flags are CLI-specific | ||
| // and shouldn't be exposed as separate JSON-RPC parameters. | ||
| if (arg.is_help_flag || arg.name.rfind("no-", 0) == 0) | ||
|
Member
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. This seems to be enforcing an implicit rule that I don't believe we have documented. Wouldn't this prevent plug-ins from having a property that starts with |
||
| continue; | ||
|
|
||
| // If the arg name has hyphens, map its camelCase equivalent | ||
| // to the kebab-case name expected by the handler. | ||
| if (arg.name.find('-') != std::string::npos) | ||
| { | ||
| std::string camel_name; | ||
| bool capitalize_next = false; | ||
| for (char c : arg.name) | ||
|
Check failure on line 80 in native/c/server/plugin_bridge.cpp
|
||
| { | ||
| if (c == '-') | ||
| { | ||
| capitalize_next = true; | ||
| } | ||
| else | ||
| { | ||
| camel_name += capitalize_next ? static_cast<char>(std::toupper(c)) : c; | ||
| capitalize_next = false; | ||
| } | ||
| } | ||
|
Comment on lines
+77
to
+92
Member
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. Can we turn this for loop into a utility function to convert from kebab to camel case? |
||
| builder.rename_arg(camel_name, arg.name); | ||
| } | ||
|
|
||
| // Also apply default values if provided | ||
| if (!arg.default_value.is_none()) | ||
| { | ||
| if (arg.default_value.is_bool()) | ||
|
Check failure on line 98 in native/c/server/plugin_bridge.cpp
|
||
| builder.set_default(arg.name, *arg.default_value.get_bool()); | ||
| else if (arg.default_value.is_int()) | ||
| builder.set_default(arg.name, *arg.default_value.get_int()); | ||
| else if (arg.default_value.is_double()) | ||
| builder.set_default(arg.name, *arg.default_value.get_double()); | ||
| else if (arg.default_value.is_string()) | ||
| builder.set_default(arg.name, *arg.default_value.get_string()); | ||
|
Comment on lines
+99
to
+106
Member
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. nit: personal preference:
|
||
| } | ||
| } | ||
|
|
||
| bool success = dispatcher.register_command(rpc_command_name, builder); | ||
|
|
||
| if (!success) | ||
| { | ||
| LOG_ERROR("Failed to register plugin command: %s", rpc_command_name.c_str()); | ||
| } | ||
| } | ||
| catch (const std::exception &e) | ||
|
Check warning on line 116 in native/c/server/plugin_bridge.cpp
|
||
| { | ||
| LOG_ERROR("Exception registering plugin command %s: %s", rpc_command_name.c_str(), e.what()); | ||
| } | ||
| catch (...) | ||
|
Check warning on line 120 in native/c/server/plugin_bridge.cpp
|
||
| { | ||
| LOG_ERROR("Unknown exception registering plugin command: %s", rpc_command_name.c_str()); | ||
| } | ||
|
Comment on lines
+117
to
+124
Member
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. nit: And we can print |
||
| } | ||
|
|
||
| // Recursively process subcommands | ||
| // Build subcommand path and recurse - avoid creating many temporary strings | ||
| size_t original_length = path_prefix.length(); | ||
| for (std::map<std::string, parser::command_ptr>::const_iterator it = subcommands.begin(); | ||
|
Check warning on line 129 in native/c/server/plugin_bridge.cpp
|
||
| it != subcommands.end(); ++it) | ||
| { | ||
| if (it->second) // Null check for safety | ||
| { | ||
| // Append to existing string instead of creating new ones | ||
| path_prefix += "."; | ||
| path_prefix += it->first; | ||
|
Comment on lines
+135
to
+137
Member
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. nit: Just thinking about memory and such, but this is pretty minimal that it probably doesn't matter much 😋 I'm thinking that if we are more memory conscious, we could increase the recursion depth 😋 |
||
|
|
||
| traverse_and_register_impl(it->second.get(), path_prefix, dispatcher, depth + 1); | ||
|
|
||
| // Restore original path for next sibling | ||
| path_prefix.resize(original_length); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void register_commands_with_server(PluginManager &pm, CommandDispatcher &dispatcher) | ||
|
t1m0thyj marked this conversation as resolved.
Outdated
|
||
| { | ||
| const std::set<parser::command_ptr> &server_commands = pm.get_server_commands(); | ||
| if (!server_commands.empty()) | ||
| { | ||
| LOG_DEBUG("Registering plugin commands to middleware"); | ||
| } | ||
|
|
||
| // Traverse top-level commands and register them to middleware | ||
| for (const auto &command : server_commands) | ||
| { | ||
| LOG_DEBUG("Registering top-level plugin command: %s", command->get_name().c_str()); | ||
| traverse_and_register(command.get(), command->get_name(), dispatcher); | ||
| } | ||
| } | ||
|
|
||
| } // namespace plugin | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /** | ||
| * This program and the accompanying materials are made available under the terms of the | ||
| * Eclipse Public License v2.0 which accompanies this distribution, and is available at | ||
| * https://www.eclipse.org/legal/epl-v20.html | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Copyright Contributors to the Zowe Project. | ||
| * | ||
| */ | ||
|
|
||
| #ifndef PLUGIN_BRIDGE_HPP | ||
| #define PLUGIN_BRIDGE_HPP | ||
|
|
||
| #include "../extend/plugin.hpp" | ||
| #include "../parser.hpp" | ||
|
|
||
| // Forward declarations | ||
| class CommandDispatcher; | ||
|
|
||
| namespace plugin | ||
| { | ||
|
|
||
| /** | ||
| * @brief Register all plugin commands to the middleware dispatcher | ||
| * | ||
| * This function takes a PluginManager that has already loaded plugins, | ||
| * and registers all their commands to the middleware CommandDispatcher | ||
| * as RPC methods using dot notation (e.g., "sample.hello"). | ||
| * | ||
| * @param pm The PluginManager containing loaded plugins | ||
| * @param dispatcher The CommandDispatcher to register commands to | ||
| */ | ||
| void register_commands_with_server(plugin::PluginManager &pm, CommandDispatcher &dispatcher); | ||
|
|
||
| } // namespace plugin | ||
|
|
||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,7 +159,6 @@ void register_uss_commands(CommandDispatcher &dispatcher) | |
| dispatcher.register_command("deleteFile", | ||
| create_uss_builder(uss::handle_uss_delete) | ||
| .validate<DeleteFileRequest, DeleteFileResponse>()); | ||
| // {"id": 1, "jsonrpc": "2.0", "method": "moveFile", "params": {"source": "source-path", "target": "target-path", "force": false}} | ||
|
Member
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. Thanks for removing. Should we have some general (JSDoc-like) documentation on how each registered command is supposed to be called from the server's perspective? |
||
| dispatcher.register_command("moveFile", | ||
| create_uss_builder(uss::handle_uss_move) | ||
| .validate<MoveFileRequest, MoveFileResponse>() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.