Skip to content
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

feat: add config_get and tests #1

Closed
wants to merge 27 commits into from
Closed

feat: add config_get and tests #1

wants to merge 27 commits into from

Conversation

mhmd-azeez
Copy link
Contributor

No description provided.

@mhmd-azeez mhmd-azeez requested a review from bhelx December 10, 2024 11:44
@@ -11,11 +11,14 @@ export function callImpl(input: CallToolRequest): CallToolResult {
if (!name) {
throw new Error("Argument `name` must be provided")
}

const greeter = config_get('name') || "anonymous"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, didn't really finish this conversation: https://github.com/dylibso/mcp.run/pull/68#discussion_r1878277435

but the type system has no way of knowing if this is ever null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fortunately i think js treats empty string as falsey here

@mhmd-azeez mhmd-azeez marked this pull request as draft December 10, 2024 16:22
@mhmd-azeez
Copy link
Contributor Author

It seems like none of hte bindgens used in this repo can handle nullable output:

  • TS:
/**
 * Get the configuration for the tool
 *
 * @param {string} input - The config key
 * @returns {string | null} The config value for the given key, or empty if not found
 */
export function config_get(input: string): string | null {
  const mem = Memory.fromString(input as string);

  const ptr = hostFunctions.config_get(mem.offset);

  return Memory.find(ptr).readJsonObject();
}
Panic at call: Uncaught TypeError: cannot read property 'readJsonObject' of undefined
    at config_get (script.js:128)
    at callImpl (script.js:137)
    at call (script.js:168)
  • Rust:
/// config_get Get the configuration for the tool
/// It takes input of String (The config key)
/// And it returns an output Option<String> (The config value for the given key, or empty if not found)
#[allow(unused)]
pub(crate) fn config_get(input: String) -> std::result::Result<Option<String>, extism_pdk::Error> {
    let res = unsafe { raw_imports::config_get(input)? };

    let extism_pdk::Json(res) = res;

    Ok(res)
}
Panic at call: EOF while parsing a value at line 1 column 0.
  • C++:
EXTISM_IMPORT_USER("config_get")
extern extism::imports::RawHandle config_get(extism::imports::RawHandle);

} // namespace imports

std::expected<std::string, Error> config_get(std::string_view input) {
  auto &encoded = input;
  auto in_handle = extism::UniqueHandle<char>::from(encoded);
  if (!in_handle) {
    return std::unexpected(Error::extism);
  }
  auto out_raw = imports::config_get(*in_handle);
  if (!out_raw) {
    return std::unexpected(Error::host_null); // <-- fails here
  }
  extism::UniqueHandle<char> out_handle(out_raw);
  auto out_string = out_handle.string();
  if (!out_string.size()) {
    return std::unexpected(Error::not_json);
  }
  return jsoncons::decode_json<std::string>(std::move(out_string));
}

@mhmd-azeez mhmd-azeez marked this pull request as ready for review December 11, 2024 16:44
@mhmd-azeez mhmd-azeez changed the title feat: greet now uses config feat: add config_get and tests Dec 11, 2024
@mhmd-azeez mhmd-azeez requested a review from nilslice December 11, 2024 17:31
@mhmd-azeez
Copy link
Contributor Author

Since nullable json output is not very well supported currently, should we go back to contentType: text/plain for the output too?

@mhmd-azeez
Copy link
Contributor Author

Also, should we rename config because of this?

image

for dir in servlets/*/; do
cd "$dir"
echo "Testing $dir"
xtp plugin test --allow-host api.fxratesapi.com --allow-host getxtp.com --log-level warn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to consider just doing--allow-host "*" for these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how will this work for file paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xtp test doesnt have configuration for allowedPaths, so we cant test them yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use the Extism CLI too in a pinch

@mhmd-azeez
Copy link
Contributor Author

mhmd-azeez commented Dec 15, 2024

closing in favor of #9

@mhmd-azeez mhmd-azeez closed this Dec 15, 2024
func mock_input() uint64

//go:export config_get
func config_get(key uint64) uint64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did @bhelx get rid of this from the schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the prod schema no longer has the config_get import

@evacchi evacchi mentioned this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants