feat(metagen)!: hostcall transport #982
Conversation
📝 WalkthroughWalkthroughThe changes update environment configuration keys and version numbers; modify workspace definitions and dependency management across Cargo.toml files and build scripts; and update target architectures (e.g., switching from wasm32-wasi to wasm32-wasip1). Documentation is revised for clarity in generator and client API descriptions. In examples and source code, multiple language-specific GraphQL transport implementations and query graph functionalities have been restructured, including replacing older type wrappers with new fdk implementations and integrating hostcall mechanisms. Numerous test files have also been updated to align with these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant QG as QueryGraph
participant Trans as Transports Module
participant GT as GraphQLTransport (e.g. HostcallTransport)
participant HC as Hostcall Service
App->>QG: Build query node from user inputs
QG->>Trans: Request transport instance (graphql/ graphql_sync)
Trans->>GT: Prepare and send GraphQL request
GT->>HC: Execute host call / query
HC-->>GT: Return parsed response
GT-->>Trans: Deliver response data
Trans-->>QG: Pass back final result
QG-->>App: Final query result returned
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Yohe-Am
force-pushed
the
feat/hostcall-tport
branch
from
4376027 to
0067562
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (2)
src/metagen/src/shared/types.rs (1)
105-122: 💡 Verification agent🧩 Analysis chain
Add tests to ensure backward compatibility with existing callers
The
TypeRenderer::newmethod now requires a third parameterprerendered_set, which is a breaking change. This change is well implemented and supports the neededHostcallTransportfunctionality, but it would be beneficial to ensure all callers have been updated.Consider adding tests that verify the behavior with both empty and populated
prerendered_setvalues to ensure the initialization works correctly in all scenarios.
🏁 Script executed:
#!/bin/bash # Check for all callers of TypeRenderer::new to ensure they've been updated rg "TypeRenderer::new" --type rust -A 2 -B 2Length of output: 5448
Action Required: Add Comprehensive Backward Compatibility Tests for TypeRenderer::new
The new constructor now requires the third parameter,
prerendered_set, which is a breaking change. While the implementation itself looks clean, our verification shows that there are multiple call sites (e.g. in client_ts, fdk_ts, client_rs, etc.) whereTypeRenderer::newis invoked. To ensure that all callers have been correctly updated and to catch any inadvertent regressions, please add tests that:
- Verify Behavior with an Empty
prerendered_set: Confirm that when no pre-rendered names are provided, thename_memofield is correctly initialized (likely as an empty collection).- Verify Behavior with a Populated
prerendered_set: Ensure that non-empty inputs correctly transform intoRenderedName::Nameentries, maintaining the expected association between integer keys and names.- Cross-check Caller Consistency: Given that call sites appear in modules like
client_ts,client_py,client_rs,fdk_ts,fdk_rs, andfdk_py, please verify that they are all updated to provide a valid third parameter.Addressing these points will help secure backward compatibility and prevent runtime issues that might arise from inconsistencies between the new constructor signature and existing usages.
tests/metagen/typegraphs/sample/py/client.py (1)
579-681: 🛠️ Refactor suggestionBase transport class with typed GraphQL building and file handling.
Overall, the pattern is clean, but be mindful of mutable defaults in function parameters (e.g.,files: Dict[str, File] = {}) as flagged by your static analysis. Changing them toNoneand initializing inside the method prevents potential shared-state bugs.- def fetch( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: GraphQLTransportOptions | None, - files: typing.Dict[str, File] = {}, - ) -> typing.Any: + def fetch( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: GraphQLTransportOptions | None, + files: typing.Optional[typing.Dict[str, File]] = None, + ) -> typing.Any: + if files is None: + files = {}🧰 Tools
🪛 Ruff (0.8.2)
621-621: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
680-680: Do not use mutable data structures for argument defaults
(B006)
🧹 Nitpick comments (69)
tests/metagen/typegraphs/sample/rs/main.rs (1)
111-133: Upgrading union arguments from partial to complete inputs.
By enforcingRootCompositeArgsFnInputforscalar_union,composite_union, andmixed_unioncalls, the logic is safer. Double-check that all references to union calls are updated similarly.tests/metagen/typegraphs/identities/ts/fdk.ts (3)
253-351:FileExtractorhandles file mapping in GraphQL variables.
The logic thoroughly detects and validates uploaded files. Watch for potential large file handling performance.🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
587-665: GraphQL response handling.
Robust error-checking for non-JSON responses. Ensure you handle compressed or chunked responses if relevant to your environment.
667-807:GraphQLTransportandPreparedRequestclasses cover typical GQL flows.
Well-organized approach for queries, mutations, and prepared queries. Keep an eye on memory overhead for largedocstrings in repeated usage.tests/metagen/typegraphs/sample/rs/client.rs (1)
262-265:RootGetUserFnInputstruct is empty.
Confirm whether any logic or arguments are expected. If so, consider removing or filling this struct to avoid confusion.tests/runtimes/wasm_reflected/rust/src/lib.rs (1)
28-28: Inconsistent string conversion methodsThe code uses both
.into()(line 28) and.to_string()(line 38) for similar string conversions.For better consistency, standardize on one approach:
- name: "Entity A".into(), + name: "Entity A".to_string(),Or if you prefer:
- name: "Entity B".to_string(), + name: "Entity B".into(),Either approach is valid, but being consistent improves readability and maintainability.
Also applies to: 38-38
src/metagen-client-rs/Cargo.toml (1)
23-26: Remove commented-out feature sectionThere's a commented-out feature section that appears to be superseded by the new implementation.
-# [features] -# default = ["sync"] -# sync = ["reqwest/blocking"] -# async = ["dep:futures", "dep:tokio-util"]Removing commented-out code improves readability and maintainability.
examples/typegraphs/metagen/rs/build.sh (3)
11-11: Verify compatibility with target architecture changeThe target has been updated from
wasm32-wasitowasm32-wasip1. This is a good modernization step as WASIP1 provides improved features, but it requires users to have compatible toolchains.Consider adding a comment explaining the reason for the target change and any prerequisites users might need.
-TARGET=wasm32-wasip1 +# Using wasip1 instead of wasi as it provides improved compatibility with the hostcall transport +TARGET=wasm32-wasip1
15-18: Update paths to use variables for better maintainabilityThe script now uses hard-coded relative paths with many parent directories. This makes the script brittle if the directory structure changes.
+ROOT_DIR="../../../../" wasm-tools component new \ - ../../../../target/$TARGET/debug/$CRATE_NAME.wasm \ - -o ../../../../target/rust-component.wasm \ + "${ROOT_DIR}target/$TARGET/debug/$CRATE_NAME.wasm" \ + -o "${ROOT_DIR}target/rust-component.wasm" \ --adapt wasi_snapshot_preview1=$ADAPTOR
21-21: Use consistent variable for pathsSimilar to the previous comment, using a variable for the root directory would make this command more maintainable.
-cp ../../../../target/rust-component.wasm ../rust.wasm +cp "${ROOT_DIR}target/rust-component.wasm" ../rust.wasmsrc/typegate/src/runtimes/substantial.ts (1)
121-130: Well implemented hostcall context with proper authenticationThe addition of the
hostcallCtxobject with authentication token is a good security practice. This implementation properly integrates the authentication into the hostcall transport system.Consider adding a comment explaining the purpose of this context object for future developers.
const token = await InternalAuth.emit(typegate.cryptoKeys); +// Create hostcall context with authentication for secure transport communication const hostcallCtx = { authToken: token, typegate, typegraphUrl: new URL( `internal+hostcall+subs://typegate/${params.typegraphName}`, ), }; const agent = new Agent(hostcallCtx, backend, queue, agentConfig);src/metagen/src/fdk_py/static/fdk.py (1)
12-23: Ctx class implementation looks good, but contains redundantpassstatement.The
Ctxclass properly stores the binding, query graph, and host transport objects as required for the HostcallTransport implementation. However, there's a redundantpassstatement at line 20 after the variable assignments.def __init__( self, binding: "HostcallBinding", qg: "QueryGraph", host: "HostcallTransport" ): self.gql = binding self.qg = qg self.host = host - passsrc/typegate/src/runtimes/python.ts (1)
146-153: Modifieddeinitmethod signature while maintaining Promise returnThe
deinitmethod has been changed from async to sync, but still returns a Promise for compatibility:
- Removed the
asynckeyword from the method signature- Removed
awaitfrom the worker manager call as it's now synchronous- Added explicit
return Promise.resolve()to maintain the Promise return typeThis change harmonizes the cleanup process with other runtime implementations while maintaining backward compatibility.
Consider adding a comment explaining why this method returns a Promise despite not being marked as async. This would help future developers understand the design decision.
src/typegate/src/runtimes/substantial/deno_context.ts (1)
8-8: Commented out unused test configurationThe testBaseUrl constant has been commented out, suggesting it's no longer needed with the new transport implementation.
Consider removing commented-out code entirely rather than just commenting it out, as it can lead to confusion in the future.
src/metagen/src/utils.rs (2)
31-89: Well-structured implementation for directive-based code generationThe
processed_writefunction is a well-designed implementation for conditional code generation with clear directive handling. The use of static lazy regex compilation is efficient, and the function correctly handles nested directives with a stack approach.Consider adding a unit test to verify the behavior with complex nested directives, especially for edge cases like empty flags or multiple negations.
There's a minor formatting issue in the error message generation - if the first unclosed directive is at line 0, the error message will start with a comma. You could fix this with:
- stack - .into_iter() - .map(|(line, _)| line) - .fold(String::new(), |acc, cur| { format!("{acc}, {cur}") }) + stack + .into_iter() + .map(|(line, _)| line.to_string()) + .collect::<Vec<_>>() + .join(", ")
91-139: Efficient pattern-based line collection implementationThe
collect_at_first_instancefunction efficiently handles pattern matching with clear separation of concerns in the different processing stages. Using a HashSet to track matches is a good optimization.One potential improvement would be to respect the original line endings from the input string rather than always using
\nfor the join operation.Consider preserving the original line endings with something like:
- output_lines.join("\n") + if input.contains("\r\n") { + output_lines.join("\r\n") + } else { + output_lines.join("\n") + }src/metagen/src/fdk_rs/static/fdk.rs (1)
102-113: Fix typo in commentThe stubs section includes a typo in the comment.
- // these are stubs to items that cum from client.rs + // these are stubs to items that come from client.rssrc/metagen-client-rs/src/selection.rs (2)
625-628: Document the rationale for keeping or removing NotUnionSelectionThe comment indicates uncertainty about why the
NotUnionSelectiontrait abstraction was added. It would be helpful to document the decision process more clearly.Consider adding more detailed comments explaining the abstraction's purpose or adding a TODO to investigate further:
// FIXME: I don't remember why I added this abstraction // I think it was breaking the generated graphql but // seems to be gone now + // TODO: Investigate if this abstraction is still needed or can be safely removed. + // If removing, verify with tests that graphql generation still works correctly. pub trait NotUnionSelection {}
756-758: Document decisions on macro implementationThe comment exchange about converting to a proc macro vs keeping as a regular macro includes a performance consideration. It would be helpful to document this decision more formally.
Consider adding a more structured comment explaining the tradeoffs:
- // TODO(Natoandro): convert to proc_macro - // REPLY(Yohe): compile time hits might be a good reason to keep this macro + // TODO: Consider converting to proc_macro + // Performance note: Keeping as a regular macro may be preferable for compile-time + // performance, especially since this is called frequently in the codebase.src/metagen/src/client_rs/static/client.rs (3)
4-5: Consider adding documentation for the newtransportsmodule.
This new module consolidates transport-related functionality, which is beneficial from an organizational standpoint. However, adding doc comments (e.g.,///) explaining usage patterns and supported environment scenarios (WASM vs. native) could improve maintainability.
10-11: Consider clarifying transport initialization.
graphqlis straightforward, though a brief inline comment about howty_to_gql_ty_mapis used to configure the transport could further clarify context for new contributors.
24-29: Document the hostcall transport creation.
While the usage ofArcfor concurrency is appropriate, a doc comment clarifying howhostcallis expected to be defined and invoked would improve discoverability for future maintainers.src/metagen/src/client_ts/mod.rs (2)
116-120: Ensure correctness of the newrender_clientinvocation.
Passinghostcall: falseexplicitly is fine for the current scenario. If you anticipate changing hostcall behavior dynamically, consider exposing it as a configuration or command-line argument.
260-260: Empty array parameter is acceptable.
Passing an empty array for theTypeRendereris non-problematic. Just ensure you update it if future transformation hooks or middlewares are introduced.src/typegate/src/runtimes/wit_wire/hostcall.ts (4)
10-14: Consider adding usage notes or disclaimers for sensitive data.
HostCallCtxcontains anauthToken, which might be sensitive. Ensure that it is never logged in plain text or sent to unintended recipients.
16-29: Validateop_nameagainst an explicit whitelist.
Currently, the code checks only for"gql"and defaults to throwing an error. Consider validatingop_nameagainst a more explicit set of supported operations to avoid unexpected behaviors or security gaps.
30-52: Replace ad-hoc console statements with the existing logger & fix typo.
- The
console.log({err}, "XXXX")log in line 30 might be accidental debug output and could be removed or replaced withlogger.debug.- There is a typo in line 48: "Unpexpected" → "Unexpected".
Proposed fix:
- console.log({err}, "XXXX") + logger.debug("Error details: {}", err)- message: `Unpexpected error: ${Deno.inspect(err)}`, + message: `Unexpected error: ${Deno.inspect(err)}`,
54-117: Handle optional or missingvariablesgracefully.
Thegqlfunction currently assumesvariablesare always present. Consider using a default (e.g.,{}) if the user omitsvariablesin the request body. This will minimize risk of null/undefined errors downstream.src/metagen/src/fdk_rs/mod.rs (2)
41-42: Document the purpose ofexclude_client.
It's unclear at first glance why the client is optionally excluded. Add a doc comment describing when and whyexclude_clientshould be set. This helps future maintainers understand the config parameter.
260-304: Avoid potential future merges of user-managed sections.
gen_staticmerges dynamic code with a static template, which can risk overwriting user modifications. Consider segmenting user-editable code into a separate file or region to reduce accidental overwrites.src/typegate/src/worker_utils.ts (1)
6-20: Guard against memory leaks and concurrency issues.
#pendingHostCallscan grow unbounded if calls remain unresolved. Ensure that each call eventually resolves or rejects. For additional safety, consider adding a timeout mechanism or a maximum size.src/metagen/src/fdk_rs/types.rs (2)
59-61: Conditional serde attribute for unknown fields.The conditional code for
#[serde(deny_unknown_fields)]provides flexibility in handling unknown fields during deserialization. However, using a commented-out code pattern might lead to confusion.Consider implementing this more explicitly by actually writing the attribute when needed rather than commenting it out:
- if !additional_props { - // writeln!(dest, "#[serde(deny_unknown_fields)]")?; - } + if !additional_props { + // We're allowing unknown fields + } else { + writeln!(dest, "#[serde(deny_unknown_fields)]")?; + }Alternatively, if this is intentionally commented out as you work through implementation details:
- if !additional_props { - // writeln!(dest, "#[serde(deny_unknown_fields)]")?; - } + // TODO: Implement unknown fields handling + // if !additional_props { + // writeln!(dest, "#[serde(deny_unknown_fields)]")?; + // }
83-83: Commented out serde tag attribute in render_enum.Similar to the previous comment, the code includes a commented-out line for enum serialization configuration. This may lead to confusion about whether this feature is intentionally disabled or just a work in progress.
Consider adding a TODO comment to clarify the intention:
- // writeln!(dest, r#"#[serde(tag = "__typename")]"#)?; + // TODO: Enable typename tagging when needed + // writeln!(dest, r#"#[serde(tag = "__typename")]"#)?;tests/metagen/metagen_test.ts (1)
514-550: Added new test case for proxy primitives.The new test case expands coverage for primitive data types with the proxy functionality, strengthening the test suite.
However, there appears to be duplication between this test case and the "primtives" test case above (lines 436-471). Consider refactoring to reduce code duplication.
// Create a reusable function or constant for the common query and variables const primitiveQuery = `query ($data: primitives) { data: prefix_primitives( data: $data ) { str enum uuid email ean json uri date datetime int float boolean } }`; const primitiveVars = { data: { str: "bytes", enum: "tree", uuid: "a963f88a-52f2-46b0-9279-ed2910ac2ca5", email: "[email protected]", ean: "0799439112766", json: JSON.stringify({ foo: "bar" }), uri: "https://metatype.dev", date: "2024-12-24", datetime: new Date().toISOString(), int: 1, float: 1.0, boolean: true, }, } as Record<string, JSONValue>; // Then use these in both test casestests/metagen/typegraphs/sample/ts/client.ts (1)
875-890: Consider using namespace instead of static classThe
Transportsclass only contains static members, which is flagged by static analysis. Consider using a namespace or plain functions instead to align with TypeScript best practices.- export class Transports { - /** - * Get the {@link GraphQLTransport} for the typegraph. - */ - static graphql( - qg: _QueryGraphBase, - addr: URL | string, - options?: GraphQlTransportOptions, - ) { - return new GraphQLTransport( - new URL(addr), - options ?? {}, - qg.typeNameMapGql, - ); - } - } + export namespace Transports { + /** + * Get the {@link GraphQLTransport} for the typegraph. + */ + export function graphql( + qg: _QueryGraphBase, + addr: URL | string, + options?: GraphQlTransportOptions, + ) { + return new GraphQLTransport( + new URL(addr), + options ?? {}, + qg.typeNameMapGql, + ); + } + }🧰 Tools
🪛 Biome (1.9.4)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/metagen/src/client_py/mod.rs (1)
124-126: AddGenClientPyOptsstructDefining a dedicated struct for client options improves clarity. You might consider adding a default implementation or doc comment if you plan to expand these options further.
src/metagen/src/client_rs/mod.rs (2)
151-153: AddGenClientRsOptsDefining a new struct for
hostcallconfiguration is straightforward. Consider adding doc comments or default handling for clarity.
171-171: Separate data and return typesSeparating
data_typesandreturn_typesis a nice improvement for managing partial vs. non-partial structures. If duplication grows, consider unifying the logic.tests/metagen/typegraphs/identities/py/fdk.py (2)
41-41: Usebool(...)directly instead of conditional expressionOn line 41, the expression
select_all = True if sub_flags is not None and sub_flags.select_all else Falsecan be replaced withselect_all = bool(sub_flags and sub_flags.select_all), which is more concise and avoids the explicitTrue/Falseternary.- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
41-41: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
784-784: Raise exception with a causeWhen re-raising exceptions in Python, consider including the original error (
err) as the cause. This helps maintain the exception chain for better debugging.-except urllib.error.URLError as err: - raise Exception(f"URL error: {err.reason}") +except urllib.error.URLError as err: + raise Exception(f"URL error: {err.reason}") from err🧰 Tools
🪛 Ruff (0.8.2)
784-784: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/metagen/typegraphs/sample/ts_upload/client.ts (1)
211-213: Avoid redeclaring or shadowing type parameter 'O'The type alias at lines 211-213 may inadvertently shadow a type parameter declared elsewhere. This can lead to confusion or unexpected TypeScript behavior. Consider renaming it to ensure clarity.
🧰 Tools
🪛 Biome (1.9.4)
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
src/metagen-client-rs/src/graphql.rs (1)
20-24: Remove or justify commented-out codeLines 20-24 contain structs and an enum commented out. Stale commented code can confuse contributors. If these items are truly deprecated, consider removing them entirely. Otherwise, add an explanatory comment describing why they're kept.
src/metagen/src/client_ts/static/mod.ts (4)
27-31: Consider minor formatting improvements for readability.
Having theforloop header spread over multiple lines may reduce readability. Collapsing it could be more concise:-for ( - const [instanceName, instanceSelection] of Object.entries( - nodeInstances, - ) -) { +for (const [instanceName, instanceSelection] of Object.entries(nodeInstances)) {
208-209: Potential lint: avoid redeclaring type parameter 'O'.
A static analysis tool complains that 'O' might be redeclared. Although it is syntactically valid in TypeScript to useinfer O, consider renaming to avoid overshadowing or confusion.-type SelectNodeOut<T> = T extends QueryNode<infer O> | MutationNode<infer O> ? O +type SelectNodeOut<T> = T extends QueryNode<infer R> | MutationNode<infer R> ? R : never;🧰 Tools
🪛 Biome (1.9.4)
[error] 208-208: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
488-508: Naming collision for 'subNodes' variable.
Inside the map callback,([variantTy, subNodes]) => ...may overshadow the outersubNodes. Consider renaming the callback parameter to reduce confusion.- .map(([variantTy, subNodes]) => { + .map(([variantTy, variantSubNodes]) => { let gqlTy = typeToGqlTypeMap[variantTy]; ... return `... on ${gqlTy} {${ variantSubNodes.map((node) => ...
916-928: Static-only class 'Transports' triggers lint warning.
The class has only static methods. If no instance-level state is used, consider converting it to plain functions to align with best practices.-export class Transports { - static graphql(...) { ... } - static hostcall(...) { ... } -} +export function graphql(...) { ... } +export function hostcall(...) { ... }examples/typegraphs/metagen/ts/fdk.ts (2)
266-266: Prefer optional chaining over&&checks.
Static analysis suggests using optional chaining.-if (path[0] && path[0].startsWith("." + key)) { +if (path[0]?.startsWith("." + key)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
942-969: Static-only classTransportstriggers lint warnings.
As with the first file, turning this into utility functions is recommended.-export class Transports { - static graphql(...) { ... } - static hostcall(...) { ... } -} +export function graphql(...) { ... } +export function hostcall(...) { ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 942-969: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
examples/typegraphs/metagen/rs/fdk.rs (1)
137-141: Building context withqgandhostis a neat approach.
Instantiating aQueryGraphon each request may be efficient enough for your usage. Consider caching if performance becomes a concern.src/metagen/src/fdk_py/mod.rs (2)
70-94: Efficient code generation.
The approach of writing out comments and templates line by line is clear. However, if performance becomes a concern for large files, consider buffering chunks or using a more structured template engine.
138-141: Regex usage for reordering or splitting content.
The lazy static approach works well for a single-target pattern. If additional import manipulation or code rearrangement is needed, consider a more flexible approach or clearly document these behaviors for maintainability.tests/metagen/typegraphs/sample/py/client.py (1)
739-773:GraphQLTransportUrlibimplementation and exceptions.
The HTTP request flow is clear. Consider raising the URLError exception with an explicit cause:- except urllib.error.URLError as err: - raise Exception(f"URL error: {err.reason}") + except urllib.error.URLError as err: + raise Exception(f"URL error: {err.reason}") from errThis preserves the traceback chain.
🧰 Tools
🪛 Ruff (0.8.2)
744-744: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
772-772: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
src/metagen-client-rs/src/common.rs (2)
73-180:register_path_to_filesandselect_node_to_gqlmethods.
Good job filtering paths relevant to a particular variable while building the query arguments. The repeated usage ofwrite!is fine, though if performance or readability becomes a concern with very large queries, consider using a templating approach.
293-459: Error handling and placeholder resolution.
Overall, the error enums and placeholder logic are well structured. Consider avoidingpanic!in non-test code (invalid path segment type) to ensure more graceful handling if unexpected data arises.- _ => panic!("invalid path segment type"), + _ => return Err(D::Error::custom("invalid path segment type")),tests/internal/py/fdk.py (1)
30-41: Handlingselection_to_nodesand theselect_alllogic.Detected usage of:
select_all = True if sub_flags is not None and sub_flags.select_all else FalsePer static analysis (SIM210), prefer a simpler boolean expression:
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags and sub_flags.select_all)This improves readability and avoids the extra conditional expression.
🧰 Tools
🪛 Ruff (0.8.2)
41-41: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
examples/typegraphs/metagen/py/fdk.py (3)
30-41:selection_to_nodesrepeated pattern withselect_all.As in the other file, consider simplifying:
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags and sub_flags.select_all)for improved clarity.
🧰 Tools
🪛 Ruff (0.8.2)
41-41: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
527-527: Reusing loop variable name (node) overshadowing.Rename the inner loop variable to avoid collisions, such as
child_node.🧰 Tools
🪛 Ruff (0.8.2)
527-527: Loop control variable
nodeoverrides iterable it iterates(B020)
784-784: Exception scope.When re-raising within an
exceptblock, do:raise Exception("...") from errto preserve the original traceback context.
🧰 Tools
🪛 Ruff (0.8.2)
784-784: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/runtimes/wasm_wire/rust/fdk.rs (2)
472-483:query_graphfunction logic is correct.
Storing typed mappings in a sharedArcis a good design choice, minimizing data copies. Consider ensuring unit tests verify these mappings are up to date with all relevant types.
486-572: Extensive methods onQueryGraphare well-structured.
Here are a few suggestions to ensure robustness:
- Confirm each node’s arguments match their defined meta definitions.
- Consider adding unit tests covering each new node method (e.g.
add,range,record, etc.) to verify correct AST or GraphQL generation.src/metagen/src/client_py/static/client.py (4)
4-10: Remove unused # metagen directives or clarify usage.
If line 5 and line 7 (# metagen-genif-not HOSTCALL/# metagen-skip) are not relevant to normal operation, consider documenting or removing them to avoid confusion.
741-776:fetchmethod logic inGraphQLTransportUrlib
Overall, this looks good for synchronous requests. Two suggestions:
- Use
raise ... from errin theURLErrorcatch block to preserve the original exception context.- Confirm that all HTTP status codes (>200) are properly surfaced for debugging.
except urllib.error.URLError as err: - raise Exception(f"URL error: {err.reason}") + raise Exception(f"URL error: {err.reason}") from err🧰 Tools
🪛 Ruff (0.8.2)
747-747: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
775-775: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
832-919:HostcallTransportsupports in-process calls without HTTP.
Check if there's any plan to handle timeouts or resilience features for large or slow vended queries. Otherwise, the abstraction is well-designed for direct bindings.🧰 Tools
🪛 Ruff (0.8.2)
855-855: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
925-975:PreparedRequestclass
This robust approach to query building and argument resolution is appreciated. For consistency, consider verifying placeholders at the earliest possible stage to fail fast when arguments are missing.src/metagen-client-rs/src/hostcall.rs (3)
11-20:HostcallTransportstruct
Struct layout is straightforward. Consider doc comments to explain usage of thebindingfunction pointer.
30-64:fetchmethod
Good approach to serializing the request. Confirm that large payloads won't lead to performance bottlenecks if everything is synchronous.
87-106:mutationandmutation_with_opts
Likewise, these no-async-limits approach might be fine for smaller calls. If large responses or concurrency are expected, consider concurrency patterns or streaming solutions.tests/metagen/typegraphs/sample/py_upload/client.py (2)
674-681: Abstract method signature observation.
UsingGraphQLTransportOptions | Noneis valid in Python 3.10+ for indicating “optional type.” Ensure your runtime environment supports this syntax. Also, returningtyping.Anycan be acceptable here as an abstract placeholder, but consider a more specific return type if you want stricter type checking.🧰 Tools
🪛 Ruff (0.8.2)
680-680: Do not use mutable data structures for argument defaults
(B006)
772-772: Preserve the original traceback in exceptions.
When raising a new exception in theexcept urllib.error.URLError as err:block, consider chaining it viaraise Exception(...) from errto retain error context.- raise Exception(f"URL error: {err.reason}") + raise Exception(f"URL error: {err.reason}") from err🧰 Tools
🪛 Ruff (0.8.2)
772-772: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.ghjk/deno.lockis excluded by!**/*.lockCargo.lockis excluded by!**/*.lockexamples/typegraphs/metagen/Cargo.lockis excluded by!**/*.locktests/runtimes/wasm_wire/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
.ghjk/lock.json(8 hunks)Cargo.toml(4 hunks)docs/metatype.dev/docs/guides/wasm-functions/index.mdx(1 hunks)docs/metatype.dev/docs/reference/metagen/index.mdx(2 hunks)docs/metatype.dev/docs/reference/typegraph/client/index.mdx(5 hunks)examples/typegraphs/metagen-py.py(1 hunks)examples/typegraphs/metagen-py.ts(1 hunks)examples/typegraphs/metagen/Cargo.toml(0 hunks)examples/typegraphs/metagen/py/fdk.py(1 hunks)examples/typegraphs/metagen/py/remix.py(1 hunks)examples/typegraphs/metagen/py/remix_types.py(0 hunks)examples/typegraphs/metagen/rs/Cargo.toml(1 hunks)examples/typegraphs/metagen/rs/build.sh(1 hunks)examples/typegraphs/metagen/rs/fdk.rs(6 hunks)examples/typegraphs/metagen/ts/fdk.ts(1 hunks)rust-toolchain.toml(1 hunks)src/meta-cli/src/typegraph/rpc/mod.rs(1 hunks)src/metagen-client-rs/Cargo.toml(1 hunks)src/metagen-client-rs/src/common.rs(1 hunks)src/metagen-client-rs/src/files.rs(5 hunks)src/metagen-client-rs/src/graphql.rs(22 hunks)src/metagen-client-rs/src/hostcall.rs(1 hunks)src/metagen-client-rs/src/lib.rs(2 hunks)src/metagen-client-rs/src/nodes.rs(1 hunks)src/metagen-client-rs/src/selection.rs(7 hunks)src/metagen/fixtures/client_rs/main.rs(5 hunks)src/metagen/fixtures/client_ts/main.ts(1 hunks)src/metagen/src/client_py/mod.rs(6 hunks)src/metagen/src/client_py/static/client.py(7 hunks)src/metagen/src/client_py/types.rs(1 hunks)src/metagen/src/client_rs/mod.rs(7 hunks)src/metagen/src/client_rs/selections.rs(1 hunks)src/metagen/src/client_rs/static/client.rs(1 hunks)src/metagen/src/client_ts/mod.rs(5 hunks)src/metagen/src/client_ts/static/mod.ts(13 hunks)src/metagen/src/fdk_py/mod.rs(3 hunks)src/metagen/src/fdk_py/static/fdk.py(1 hunks)src/metagen/src/fdk_py/static/main.py.jinja(0 hunks)src/metagen/src/fdk_py/static/struct.py.jinja(0 hunks)src/metagen/src/fdk_py/static/types.py.jinja(0 hunks)src/metagen/src/fdk_py/types.rs(0 hunks)src/metagen/src/fdk_py/utils.rs(0 hunks)src/metagen/src/fdk_rs/mod.rs(7 hunks)src/metagen/src/fdk_rs/static/Cargo.toml(1 hunks)src/metagen/src/fdk_rs/static/fdk.rs(3 hunks)src/metagen/src/fdk_rs/stubs.rs(1 hunks)src/metagen/src/fdk_rs/types.rs(4 hunks)src/metagen/src/fdk_ts/mod.rs(4 hunks)src/metagen/src/shared/types.rs(1 hunks)src/metagen/src/tests/fixtures.rs(2 hunks)src/metagen/src/utils.rs(2 hunks)src/pyrt_wit_wire/main.py(4 hunks)src/typegate/src/config/shared.ts(0 hunks)src/typegate/src/engine/computation_engine.ts(1 hunks)src/typegate/src/runtimes/deno/deno.ts(7 hunks)src/typegate/src/runtimes/deno/types.ts(1 hunks)src/typegate/src/runtimes/deno/worker.ts(2 hunks)src/typegate/src/runtimes/deno/worker_manager.ts(3 hunks)src/typegate/src/runtimes/python.ts(2 hunks)src/typegate/src/runtimes/substantial.ts(4 hunks)src/typegate/src/runtimes/substantial/agent.ts(8 hunks)src/typegate/src/runtimes/substantial/deno_context.ts(3 hunks)src/typegate/src/runtimes/substantial/types.ts(2 hunks)src/typegate/src/runtimes/substantial/worker.ts(2 hunks)src/typegate/src/runtimes/wasm/worker.ts(1 hunks)src/typegate/src/runtimes/wasm/worker_manager.ts(1 hunks)src/typegate/src/runtimes/wit_wire/hostcall.ts(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts(1 hunks)src/typegate/src/services/auth/mod.ts(2 hunks)src/typegate/src/services/auth/protocols/oauth2.ts(2 hunks)src/typegate/src/worker_utils.ts(1 hunks)tests/internal/py/fdk.py(1 hunks)tests/internal/py/logic.py(1 hunks)tests/internal/py/logic_types.py(0 hunks)tests/metagen/metagen_test.ts(7 hunks)tests/metagen/typegraphs/identities.py(3 hunks)tests/metagen/typegraphs/identities/py/fdk.py(1 hunks)tests/metagen/typegraphs/identities/py/handlers.py(1 hunks)tests/metagen/typegraphs/identities/py/handlers_types.py(0 hunks)tests/metagen/typegraphs/identities/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/identities/rs/build.sh(1 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs(7 hunks)tests/metagen/typegraphs/identities/rs/lib.rs(3 hunks)tests/metagen/typegraphs/identities/ts/fdk.ts(2 hunks)tests/metagen/typegraphs/identities/ts/handlers.ts(2 hunks)tests/metagen/typegraphs/sample/py/client.py(7 hunks)tests/metagen/typegraphs/sample/py/main.py(1 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(9 hunks)tests/metagen/typegraphs/sample/py_upload/main.py(1 hunks)tests/metagen/typegraphs/sample/rs/client.rs(15 hunks)tests/metagen/typegraphs/sample/rs/main.rs(6 hunks)tests/metagen/typegraphs/sample/rs_upload/client.rs(3 hunks)tests/metagen/typegraphs/sample/rs_upload/main.rs(3 hunks)tests/metagen/typegraphs/sample/ts/client.ts(13 hunks)tests/metagen/typegraphs/sample/ts/main.ts(2 hunks)tests/metagen/typegraphs/sample/ts_upload/client.ts(13 hunks)tests/metagen/typegraphs/sample/ts_upload/main.ts(1 hunks)tests/runtimes/python/py_fail/dep_fail.py(1 hunks)tests/runtimes/python/py_fail/hello_fail.py(1 hunks)tests/runtimes/python/python_test.ts(6 hunks)tests/runtimes/substantial/common.ts(0 hunks)tests/runtimes/wasm_reflected/rust/Cargo.toml(1 hunks)tests/runtimes/wasm_reflected/rust/build.sh(1 hunks)tests/runtimes/wasm_reflected/rust/src/lib.rs(3 hunks)tests/runtimes/wasm_wire/rust/Cargo.toml(1 hunks)tests/runtimes/wasm_wire/rust/build.sh(1 hunks)tests/runtimes/wasm_wire/rust/fdk.rs(6 hunks)
⛔ Files not processed due to max files limit (5)
- tests/runtimes/wasm_wire/rust/lib.rs
- tools/compose/compose.base.yml
- tools/tasks/dev.ts
- tools/tasks/install.ts
- tools/tasks/test.ts
💤 Files with no reviewable changes (11)
- src/metagen/src/fdk_py/static/struct.py.jinja
- src/typegate/src/config/shared.ts
- tests/runtimes/substantial/common.ts
- examples/typegraphs/metagen/Cargo.toml
- src/metagen/src/fdk_py/static/main.py.jinja
- tests/internal/py/logic_types.py
- src/metagen/src/fdk_py/static/types.py.jinja
- src/metagen/src/fdk_py/types.rs
- src/metagen/src/fdk_py/utils.rs
- examples/typegraphs/metagen/py/remix_types.py
- tests/metagen/typegraphs/identities/py/handlers_types.py
🧰 Additional context used
🪛 Biome (1.9.4)
tests/metagen/typegraphs/sample/ts/client.ts
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/metagen/typegraphs/sample/ts_upload/client.ts
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/metagen/src/client_ts/static/mod.ts
[error] 208-208: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 915-944: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
examples/typegraphs/metagen/ts/fdk.ts
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 942-969: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
tests/metagen/typegraphs/identities/ts/fdk.ts
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 942-969: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
🪛 Ruff (0.8.2)
tests/metagen/typegraphs/identities/py/fdk.py
41-41: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
527-527: Loop control variable node overrides iterable it iterates
(B020)
633-633: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
692-692: Do not use mutable data structures for argument defaults
(B006)
756-756: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
784-784: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
862-862: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/metagen/src/client_py/static/client.py
683-683: Do not use mutable data structures for argument defaults
(B006)
747-747: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
775-775: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
855-855: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/metagen/typegraphs/sample/py/client.py
680-680: Do not use mutable data structures for argument defaults
(B006)
744-744: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
772-772: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
examples/typegraphs/metagen/py/fdk.py
41-41: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
527-527: Loop control variable node overrides iterable it iterates
(B020)
633-633: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
692-692: Do not use mutable data structures for argument defaults
(B006)
756-756: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
784-784: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
862-862: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/internal/py/fdk.py
41-41: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
527-527: Loop control variable node overrides iterable it iterates
(B020)
633-633: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
692-692: Do not use mutable data structures for argument defaults
(B006)
756-756: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
784-784: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
862-862: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/metagen/typegraphs/sample/py_upload/client.py
680-680: Do not use mutable data structures for argument defaults
(B006)
744-744: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
772-772: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (300)
src/typegate/src/engine/computation_engine.ts (1)
191-195: Cosmetic Formatting ImprovementThe updated type assertion in the
registerResultmethod now includes a space—changing((l ?? {})as Record<string, unknown>)[field]to((l ?? {}) as Record<string, unknown>)[field]. This improves readability without altering the logic.rust-toolchain.toml (1)
5-5: Updated WASM Target Configuration
The targets field now includes"wasm32-wasip1". This update aligns with the recent shift in target architecture. Please ensure this new target is supported by all related tooling and build configurations.docs/metatype.dev/docs/guides/wasm-functions/index.mdx (1)
120-122: Update TARGET Variable in Build Snippet
The shell snippet now setsTARGET=wasm32-wasip1, which is consistent with the changes in the toolchain configuration. Verify that all documentation references and subsequent build instructions reflect this updated target appropriately.tests/metagen/typegraphs/identities/rs/build.sh (1)
8-14: Revised Build Script for Identities FDK
The build script now specifiesTARGET=wasm32-wasip1and updates the WASM file path to referenceidentities_fdk.wasm. This appears consistent with the overall target migration. Please double-check that the referenced file names and paths (e.g.,identities_fdk.wasm) conform to the Cargo project outputs and naming conventions.src/meta-cli/src/typegraph/rpc/mod.rs (1)
23-26:❓ Verification inconclusive
Verify Enum Variant Reordering in RpcCall
The modifications indicate a reordering of theRpcCallenum variants:
- The AI summary specifies that the
Core(core::RpcCall)variant should appear above theAws(aws::RpcCall)variant.- Currently, the ordering is:
Aws(aws::RpcCall)Core(core::RpcCall)Runtimes(runtimes::RpcCall)Utils(utils::RpcCall)Please confirm if this ordering is intentional. In untagged enums, the variant order can affect deserialization behavior. If the intended order is indeed to have
CoreprecedeAws, then the variant ordering should be revised accordingly.
Below is a shell script to verify how the
RpcCallenum is declared—including any serde attributes—and to inspect its variant ordering:Once these results are available, we can see if the enum is annotated (for example, with
#[serde(untagged)]), which would mean that the ordering of variants (currently:Aws(aws::RpcCall), Core(core::RpcCall), Runtimes(runtimes::RpcCall), Utils(utils::RpcCall),) can impact deserialization behavior. The AI summary suggests that the
Core(core::RpcCall)variant should precedeAws(aws::RpcCall), so please confirm if this discrepancy is intentional. If the enum is, in fact, untagged and the order affects deserialization as expected, a reordering might be required.Please verify and confirm the intended order of these enum variants.
🏁 Script executed:
#!/bin/bash echo "------ Printing first 50 lines of src/meta-cli/src/typegraph/rpc/mod.rs ------" sed -n '1,50p' src/meta-cli/src/typegraph/rpc/mod.rs echo "" echo "------ Searching for serde attributes in the file ------" grep -R "#\[serde" src/meta-cli/src/typegraph/rpc/mod.rsLength of output: 1166
Confirm RpcCall Enum Variant Order
The code currently defines the untagged enum variants in the following order:
Aws(aws::RpcCall)Core(core::RpcCall)Runtimes(runtimes::RpcCall)Utils(utils::RpcCall)Since the enum uses
#[serde(untagged)], the order in which variants are listed directly impacts deserialization. The AI summary noted that theCore(core::RpcCall)variant should precedeAws(aws::RpcCall), which is not the case here.Please verify if this ordering is intentional. If the intent is indeed to prioritize the
Corevariant overAws, then the enum should be reordered accordingly to avoid potential deserialization issues.tests/runtimes/wasm_wire/rust/build.sh (1)
10-10: Consistency Check for WASM Wire Build Script
There are several notable updates in this script:
- The WASM file path now references
wasm_wire_rust.wasm(lines 14–16), and the copy command (line 19) has been updated accordingly.- However, the target remains set to
wasm32-unknown-unknown(line 10) unlike the other scripts that now usewasm32-wasip1.
Please confirm whether the use ofwasm32-unknown-unknownis intentional for the wasm_wire runtime or if it should be updated for consistency with the other modules.Also applies to: 13-16, 19-19
src/metagen/src/tests/fixtures.rs (2)
21-27: Good change to ownership modelThe refactoring from deserializing directly into
Vec<Arc<Typegraph>>toVec<Typegraph>and then wrapping withArc::new()is a good practice. This approach provides better control over memory allocation and avoids unnecessary reference counting during the deserialization process.
124-130: Consistent ownership model applied correctlyThe same refactoring approach has been correctly applied to this function as well, maintaining consistency with the changes in
test_typegraph_1(). This ensures a uniform approach to handlingTypegraphobjects throughout the codebase.src/pyrt_wit_wire/main.py (3)
36-47: Explicit import declarations for runtime dependenciesThese imports are correctly marked with
ruff: noqa: F401since they're not directly used in this file but are required by client_py at runtime. This approach ensures all necessary modules are available when the code is componentized.
59-62: Well-implemented functional approach for GraphQL queriesThe new
gql_fnfunction replaces the previous object-oriented approach, providing a more streamlined way to handle GraphQL queries. This functional approach is cleaner and aligns with the hostcall transport architecture mentioned in the PR objectives.
105-105: Updated handler to use the new functional approachThis change correctly updates the
ErasedHandler.handlemethod to pass the newgql_fnfunction instead of aCtxinstance, completing the transition to the new functional approach for GraphQL queries.docs/metatype.dev/docs/reference/metagen/index.mdx (2)
173-175: Documentation updated to reflect new capabilitiesThe documentation has been properly updated to reflect the new functionality. The TODO has been replaced with clear instructions about using
stubbed_runtimes, and information about theHostcallTransportimplementation has been added. This aligns with the PR objectives of implementingHostcallTransportfor different frameworks.
218-219: Consistent documentation for fdk_rs transportThe documentation for
fdk_rshas been appropriately updated with the same pattern, mentioning the client_rs based typegraph client and the specialHostcallTransportimplementation. This maintains consistency across different framework documentation..ghjk/lock.json (7)
980-980: Update of environment configuration for test-codegen.The
envKeyfor the test-codegen task has been updated from the previous value to "bciqgfkhptprs54tg3wl45acxctzc2lu4v2xlh3slk6w4m5g5ctysv4a". This is part of the system-wide updates to support the new HostcallTransport implementation.
1132-1132: Update of environment configuration for build-tgraph-py.The
envKeyfor the build-tgraph-py task has been updated to "bciqpei32hkwk37chtbyin65e3pvaeawn6cq3ytsihyxo54ixft7yyjq". This change aligns with the PR objective to reimplementfdk_pyand consolidate code generation.
1142-1142: Update of environment configuration for build-tgraph-rpc.The
envKeyfor the build-tgraph-rpc task has been updated to "bciqn7hev7ej5zg7cixajsoe5kpouah36nmzt2o7vsxnxgl6tou4z7cy". This supports the transition to the new transport construction framework.
1147-1147: Update of environment configuration for build-tgraph-ts.The
envKeyfor the build-tgraph-ts task has been updated to "bciqdxfpgv43gmtbphf7wdom7n6wv3xvhrtexnqrokaok6ljbccma5by", supporting the changes infdk_tstransport construction mentioned in the PR objectives.
1155-1155: Update of environment configuration for build-tgraph-ts-node.The
envKeyfor the build-tgraph-ts-node task has been updated to match build-tgraph-ts, ensuring consistency across related TypeScript builds.
1160-1160: Update of environment configuration for build-tgraph-core.The
envKeyfor the build-tgraph-core task has been updated to "bciqg4j54df6d7tkfqx5g6fwnhuznqtrveerazjqzrtykguzhefcljpa". This provides the foundation for the hostcall transport implementation.
1889-1889: Update of TYPEGRAPH_VERSION environment variable.The version has been incremented from "0.0.3" to "0.0.4", reflecting the breaking changes in the API due to the new HostcallTransport implementation.
tests/metagen/typegraphs/sample/rs_upload/main.rs (7)
12-13: Updated API initialization with new transport architecture.The code now:
- Explicitly types
addrasUrlfor better clarity- Uses the new factory function
query_graph()instead ofQueryGraph::newThis aligns with the PR objective of transitioning transport construction from a method on
QueryGraphto a set of functions in thetransportsmodule.
17-17: Moved transport creation to the transports module.Using
transports::graphql_sync(&api1, addr.clone())instead ofapi1.graphql_sync()reflects the architectural change described in the PR objectives, where transport construction is now handled by dedicated functions instead of methods onQueryGraph.
19-23: Updated to use non-optional structs for file uploads.The code now uses
types::RootUploadFnInputwith a requiredfilefield instead of a partial struct with optional fields. This improves type safety and requires developers to provide all necessary parameters.
28-31: Updated upload_many to use non-optional input structure.The code now uses
types::RootUploadManyFnInputwith a requiredfilesfield. This is a more type-safe approach that clearly communicates which fields are mandatory.
52-52: Updated async transport creation to use the transports module.Similar to the sync version, the async GraphQL client is now created using
client::transports::graphql(&api1, addr)instead of a method onapi1, maintaining consistency with the new architecture.
55-60: Updated file upload argument preparation for async API.The code now uses a consistent approach with required fields for the prepared mutation, matching the synchronous implementation and providing better type safety.
85-88: Updated upload_many mutation to use the new required field structure.The async version of upload_many now uses the same
types::RootUploadManyFnInputstructure with required fields as the synchronous implementation, ensuring consistency across the API.tests/metagen/typegraphs/sample/rs_upload/client.rs (6)
7-18: Added new transports module with dedicated factory functions.This new module centralizes transport creation logic with two key functions:
graphql()for async transportgraphql_sync()for synchronous transportThis implements the PR objective of moving transport construction from methods on
QueryGraphto separate functions, making the architecture more modular and easier to extend.
91-94: Updated RootUploadFnInput to use required fields.The struct now has a required
filefield (no longer wrapped inOption), which improves type safety by ensuring this mandatory field is always provided.
95-95: Added explicit RootUploadFnOutput type definition.The function return type is now clearly defined as a boolean, improving code clarity and documentation.
99-102: Updated RootUploadManyFnInput to use required fields.Similar to RootUploadFnInput, this struct now has a required
filesfield, ensuring type safety and clarity regarding which parameters are mandatory.
109-130: Added factory function for QueryGraph creation.This new
query_graph()function centralizes the creation logic for QueryGraph instances, providing a consistent way to initialize them with the necessary type mappings. This is an important part of the new transport architecture.
134-135: Updated method signatures to use new input types.The
uploadandupload_manymethods now accept the new input types with required fields, completing the migration to a more type-safe API. This is consistent with the PR objective of enhancing the handling of GraphQL queries and mutations.Also applies to: 152-153
tests/metagen/typegraphs/sample/rs/main.rs (9)
13-14: Use ofquery_graph()aligns with the new approach.
The adoption ofquery_graph()instead ofQueryGraph::new(...)appears consistent with the refactoring strategy.
18-18: Replaced direct method call withgraphql_synctransport.
Invokingclient::transports::graphql_sync(&api1, addr.clone())is consistent with the new architecture separating transport creation from theQueryGraph.
52-55: Confirm fullPostpayload correctness.
Here,id,slug, andtitleare being set as empty strings for slug and title. Ensure that the downstream logic can handle empty values.
69-69: Adoption ofgraphqltransport.
Similar tographql_sync, this is consistent with the separation of transport logic fromQueryGraph.
96-99: FullPostobject passed toscalar_args.
All fields are now required, which strengthens type safety. Confirm no undesired side effects from using an empty slug/title.
102-104: Composite arguments now mandatory.
Ensures anidis always provided tocomposite_args. This improves clarity and prevents partial data scenarios.
171-174: Mutation call with a completePostobject.
Once again, confirm that an empty slug/title is acceptable in your domain logic.
180-190: From partial to fully requiredRootIdentityFnInput.
This makes thenumargument mandatory. Verify that no backward compatibility issues arise, especially if older code expected optional fields.
39-43:❓ Verification inconclusive
Switching from partial to full
Postand composite arguments.
By requiringtypes::PostandRootCompositeArgsFnInput, all necessary fields must be explicitly set. Ensure all calls supplying these arguments pass valid, non-empty strings or relevant data.Would you like to run a quick search to confirm no old references to
PostPartialor partial inputs remain?
🏁 Script executed:
#!/bin/bash rg -A 4 PostPartial rg -A 4 RootCompositeArgsFnInputPartialLength of output: 3021
Full type usage verified in main.rs – review legacy partial types in client.rs
The changes in
tests/metagen/typegraphs/sample/rs/main.rs(lines 39–43) correctly switch from using partial types to requiring full inputs (types::PostandRootCompositeArgsFnInput), ensuring that all necessary fields are provided explicitly. However, a repository search indicates that legacy references—particularly, the definition and usage ofPostPartial—remain intests/metagen/typegraphs/sample/rs/client.rs.
- Action Items:
- Confirm that the legacy
PostPartialtype intests/metagen/typegraphs/sample/rs/client.rsis either intentionally retained for backward compatibility or should be updated/removed.- Verify that all calls supplying the new full types guarantee valid, non-empty strings or the expected data as per the new requirements.
Please review the usage in
client.rsand update or document accordingly if the old partial types are no longer desirable.tests/metagen/typegraphs/identities/ts/fdk.ts (5)
36-207: Implementation of_selectionToNodeSetfunction.
The design properly checks node presence, arguments, and aliases. Consider verifying all error messages for clarity.
209-245: Introduction ofQueryNode,MutationNode, and supporting types.
These classes systematically wrap node metadata. The use of private fields (#inner) prevents accidental misuse.🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
353-585: Comprehensive GQL building and fetching.
This portion includes variable substitution, query document construction, and handling file uploads. The structure appears solid.
809-841:HostcallTransport: custom transport approach.
It raises an error for file uploads, which is a valid approach if your environment doesn't support them.
975-1522: Extensive node metadata definitions.
The typed approach for composites, cycles, and proxies is well-structured. Ensure thorough test coverage due to the complexity.tests/metagen/typegraphs/sample/rs/client.rs (5)
7-19: Newtransportsmodule for GraphQL transport creation.
Splitting transports fromQueryGraphclarifies responsibilities and prevents collisions with the WASM environment.
270-280:PostandUserunify mandatory fields.
The new definitions enforce a fully required set of PostgreSQL-like fields. This helps ensure data consistency between server and client.
293-297:RootCompositeUnionFnOutputpartial definitions replaced.
The code now references the new enumerations forPostandUser. Verify that any legacy references to the partial variants remain updated.
331-332: Newreturn_typesmodule.
This reorganizes partial data types under a separate namespace, further clarifying the difference between full objects and partial variants.
441-444:query_graphfunction returns aQueryGraphwith typed mappings.
This ensures consistency across the codebase, especially for “UserIdStringUuid” and “StringE1a43.” Continue to confirm all references are correct.examples/typegraphs/metagen-py.py (1)
31-31: Updated dependency path aligns with the PR's objectivesThe dependency path has been updated from
./metagen/py/remix_types.pyto./metagen/py/fdk.py, which is consistent with the PR objective of consolidating allfdk_pycode generation intofdk.py. This change is part of the larger refactoring effort for the hostcall transport implementation.tests/runtimes/python/py_fail/hello_fail.py (1)
3-3: Added Ruff linter suppression for test failure fileAdding
# ruff: noqais appropriate for this test file since it intentionally contains code errors (the undefined variableundefind) for testing failure cases. This prevents unnecessary linting warnings in a file that's designed to demonstrate failures.tests/runtimes/python/py_fail/dep_fail.py (2)
3-3: Added Ruff linter suppression for test failure fileAdding
# ruff: noqais appropriate here as this file intentionally contains a syntax error in the function definition to test failure cases. This prevents unnecessary linting warnings in a file designed to demonstrate failures.
5-6: Function definition contains syntax error (intentional for testing)The function definition has a syntax error where the colon is misplaced after the type annotation instead of after the parameter list. This appears to be intentional since this is a test file for dependency failures.
🧰 Tools
🪛 Ruff (0.8.2)
5-5: SyntaxError: Expected ',', found ':'
5-6: SyntaxError: Expected ')', found newline
src/metagen/src/fdk_rs/stubs.rs (1)
168-168: Added new configuration option for excluding client codeThe new
exclude_client: Some(true)field added toFdkRustGenConfigimplements part of the hostcall transport feature by controlling whether client-related code is included in the generated output. This aligns with the PR objective of transitioning transport construction fromQueryGraphto functions in thetransportsmodule.examples/typegraphs/metagen-py.ts (1)
30-30: Dependency change aligns with FDK consolidationThis change correctly updates the dependency from the previous type-specific file to the new consolidated
fdk.pymodule, which aligns with the PR objective of reimplementingfdk_pyand consolidating all code generation intofdk.py.src/typegate/src/runtimes/wasm/worker_manager.ts (1)
14-15: Improved module organization for hostcall functionalityThe refactoring to import hostcall-related functionality from a dedicated module (
hostcall.ts) rather than the general module improves code organization and modularity. This change supports the PR's focus on implementing HostcallTransport by better structuring the related code.tests/metagen/typegraphs/sample/py_upload/main.py (1)
7-7: Transport creation refactored to dedicated Transports classThis change properly implements the architectural shift described in the PR objectives, moving transport construction from a method on
QueryGraphto a static method in theTransportsclass. This improves separation of concerns and aligns with modern software design principles.The implementation correctly:
- Adds the necessary import for the
Transportsclass- Changes the client initialization to use
Transports.graphql_sync(api, url)instead of a method on the api objectAlso applies to: 13-13
tests/metagen/typegraphs/sample/ts_upload/main.ts (2)
4-4: Clean import update for transport refactoringThe import statement has been updated to include the new
Transportsclass, which aligns with the PR objective of transitioning transport construction to a dedicated module.
9-9: Transport construction moved to static methodThe GraphQL client instantiation has been refactored from a direct method call on
qgto using the staticTransports.graphqlmethod. This change follows the architectural shift described in the PR objectives, where transport construction forfdk_tsis now a static method within theTransportsclass.src/metagen-client-rs/src/nodes.rs (1)
8-8: Module reorganization for PathToInputFilesThe import path for
PathToInputFileshas been changed from thefilesmodule to thecommonmodule, indicating a structural reorganization to improve code organization. This aligns with the PR's focus on reimplementation and restructuring.examples/typegraphs/metagen/rs/Cargo.toml (2)
17-17: Added workspace dependency for metagen-clientThe addition of
metagen-client.workspace = trueproperly integrates this package with the workspace configuration, which is a good practice for managing related crates within a monorepo.
19-25: Release profile settings commented outThe release profile optimizations have been commented out and the comment updated to indicate these are optional settings. This change might be to allow workspace-level settings to take precedence or to provide flexibility in build configurations.
src/metagen/fixtures/client_ts/main.ts (2)
4-4: Updated import for Transports moduleThe import statement now includes the
Transportsclass, which is consistent with the architectural changes described in the PR objectives.
8-10: Refactored GraphQL client instantiationThe GraphQL client instantiation has been updated to use the static
Transports.graphqlmethod, passing theapi1instance as a parameter. This change is part of the broader refactoring described in the PR, moving transport construction to dedicated modules or classes.tests/runtimes/wasm_reflected/rust/build.sh (2)
7-7: Update wasm-tools command to use new path structureThe path for the WebAssembly component has been updated to reflect the new package naming structure (
wasm_reflected_rustinstead ofrust), which aligns with the changes in Cargo.toml that renamed the package from "rust" to "wasm_reflected_rust".
10-10: Update copy command to use new file pathThe copy command now correctly references the new file path that matches the updated component name in the previous wasm-tools command.
tests/metagen/typegraphs/identities/ts/handlers.ts (2)
14-14: Add required imports for the new transport implementationThe additions to the import statement are necessary to support the new hostcall transport functionality.
44-51: Implement hostcall transport for TS primitivesThis implementation adds the
proxy_primitiveshandler function that demonstrates the hostcall transport pattern. It correctly:
- Creates a new QueryGraph instance
- Obtains a hostcall transport via the static Transports.hostcall method
- Uses the transport to execute a query to the tsPrimitives endpoint
This aligns with the PR objectives related to implementing HostcallTransport for fdk_ts.
src/metagen/src/client_py/types.rs (2)
32-38: Improve TypedDict property rendering to handle optional typesThis change enhances the rendering of TypedDict properties by properly differentiating optional properties (using
typing.NotRequired) from required ones. This approach is more idiomatic to Python's typing system and provides better type information to Python static type checkers.
40-40: Remove total=False argument from TypedDictThe removal of
total=Falseargument is appropriate since the optionality of properties is now handled individually usingtyping.NotRequiredfor each optional property. This results in a more accurate type representation.src/metagen/src/client_rs/selections.rs (1)
81-83: Add phantom field for empty union selection structsThis addition ensures type safety for empty union selection structs by including a
PhantomData<ATy>field. Without this field, the generic parameterATywould be unused in empty structs, potentially causing compiler warnings or errors in Rust.This is a common pattern in Rust for preserving type parameters that aren't directly used in fields but are semantically important for the type.
src/metagen/src/fdk_ts/mod.rs (4)
42-43: Config enhancement for client exclusionThis new
exclude_clientfield provides a way to opt out of client generation when only type declarations are needed. This aligns with the hostcall transport feature described in the PR objectives.
92-100: Good implementation of conditional client renderingThe conditional logic properly implements the
exclude_clientfeature, usingunwrap_or_default()to safely handle the Option. This ensures backward compatibility while allowing new functionality.
199-199: Empty vector passed to TypeRendererThe TypeRenderer is now initialized with an empty array, which seems to be part of the new hostcall transport implementation.
225-225: Test configuration is correctSetting
exclude_clienttoNonein the test ensures backward compatibility is maintained, as the default behavior will be used.tests/metagen/typegraphs/sample/py/main.py (2)
10-10: New Transports import addedThis import supports the architectural change described in the PR objectives, where transport construction is now handled by dedicated modules.
17-17: GraphQL client creation refactored to use TransportsThis change aligns with the PR objective of moving transport construction from a method on
QueryGraphto functions in thetransportsmodule or a static method in theTransportsclass.examples/typegraphs/metagen/py/remix.py (3)
1-1: Updated imports to use new handler patternThe import now includes
handler_remix_trackandCtx, aligning with the new patterns for decorators mentioned in the PR objectives.
6-7: Updated function signature to match new decorator patternThe function decorator now follows the
handler_{fn_name}format and the function accepts a secondCtxparameter as mentioned in the PR objectives.
8-13: Return format changed to dictionary with snake_case keysThe implementation now returns a dictionary with snake_case keys (
release_time,mp3_url) rather than camelCase.tests/internal/py/logic.py (2)
4-4: Updated imports for new handler patternImports now include
handler_remote_sumandRootSumFnInputfrom.fdk, which supports the new decorator pattern mentioned in the PR objectives.
8-9: Function updated to use new decorator and typed inputThe function is now decorated with
@handler_remote_sumand accepts a typed inputRootSumFnInputinstead of a generic dictionary, aligning with the new patterns described in the PR objectives.src/typegate/src/runtimes/substantial/worker.ts (4)
4-4: Import added for HostcallPumpThe import of
HostcallPumpfrom worker_utils.ts supports the new hostcall transport mechanism.
10-10: New HostcallPump instance createdCreating the pump at module level ensures it's available throughout the worker's lifecycle.
12-16: Effective message handling for HOSTCALL_RESPGood implementation of early return pattern for hostcall responses, keeping the code clean and focused.
39-51: Well-structured context initialization with hostcall handlerThe implementation properly integrates the hostcall mechanism into the existing Context structure, allowing for seamless communication between the worker and host environment.
tests/metagen/typegraphs/identities/rs/lib.rs (3)
4-5: Clippy allowance added for large_enum_variantThis suppresses warnings for large enum variants, which is reasonable if the enums are used in a context where size optimization isn't critical.
8-8: Added metagen_client prelude importThis import provides access to the essential types and functions from the metagen_client crate, which is needed for the new HostcallTransport functionality.
19-19: New handler registration for RsProxyPrimitivesThis registers a new handler for the proxy primitives, enabling the new transport mechanism to be used with the existing Mat structure.
tests/runtimes/wasm_reflected/rust/src/lib.rs (1)
4-5: Clippy allowances addedThe code now suppresses warnings for too_many_arguments and approx_constant, which is reasonable for test code.
src/metagen-client-rs/Cargo.toml (3)
6-9: Well-structured feature flags addedThe addition of feature flags with GraphQL as the default is a good practice for modular functionality.
13-13: Made reqwest dependency optionalMaking reqwest optional aligns with the feature flag approach, ensuring users only include what they need.
20-21: Target-specific dependency configurationGood use of conditional dependencies to exclude tokio-util from WebAssembly targets where it's not needed.
src/typegate/src/runtimes/substantial.ts (1)
473-478: Improved error message formatting for better readabilityThe error message formatting has been improved to make the list of available workflows more readable. This is a good user experience enhancement.
tests/runtimes/python/python_test.ts (3)
361-363: Good formatting improvement for long test namesBreaking the long test name across multiple lines improves readability without affecting functionality.
426-428: Good formatting improvement for long test namesSimilar to the previous case, breaking the long test name across multiple lines improves readability.
6-10:❓ Verification inconclusive
Verify import path for WitWireHandle
There's a potential issue with the import paths. Line 6 imports
WitWireHandlefrommod.tswhile line 10 imports other related items fromhostcall.ts. This could lead to inconsistency if both files export the same entities.Ensure that
WitWireHandleis imported from the correct location and that there's no duplication of exports between these modules.
🏁 Script executed:
#!/bin/bash # Check if WitWireHandle is exported from both mod.ts and hostcall.ts echo "Checking mod.ts exports:" rg "export.*WitWireHandle" "@metatype/typegate/runtimes/wit_wire/mod.ts" echo "Checking hostcall.ts exports:" rg "export.*WitWireHandle" "@metatype/typegate/runtimes/wit_wire/hostcall.ts"Length of output: 451
Action Required: Verify Import Paths for WitWireHandle
The verification script did not find the files at the specified paths, which means we couldn’t confirm whether
WitWireHandleis exported from both modules or if this is an intended alias-based import (e.g., via an import map or TypeScript configuration).
- Please double-check that the module resolution for
"@metatype/typegate/runtimes/wit_wire"in your configuration (such as tsconfig.json or a Deno import map) correctly maps to the intended files.- If these files are supposed to be local, ensure that both
mod.tsandhostcall.tsexist in the correct locations.- Confirm that there’s no duplication of the
WitWireHandleexport between these modules.Additional manual verification of the module paths is necessary to avoid any future inconsistencies or conflicts.
src/metagen/src/fdk_py/static/fdk.py (1)
1-9: Well-structured type definitions for HostcallTransport implementation.The type aliases are clearly defined using Python's typing module. This provides a solid foundation for the hostcall transport mechanism in the Python FDK.
src/typegate/src/services/auth/mod.ts (2)
75-86: Good standardization on usingundefinedinstead ofnull.The change from
Protocol | nulltoProtocol | undefinedand the corresponding adjustments to fallback values standardize how the absence of an auth protocol is represented, which aligns with TypeScript best practices.
96-102: Improved error handling with explicit error logging.The addition of the
errordestructuring and the conditional logging enhances the system's observability by explicitly reporting token middleware errors.src/typegate/src/runtimes/substantial/types.ts (2)
14-25: Good extension of WorkflowMessage type to support hostcall responses.The WorkflowMessage type has been properly expanded to a union type that includes the new HOSTCALL_RESP message type, providing the necessary structure for hostcall responses.
56-57: WorkflowEvent type properly extended for hostcall events.The addition of the HOSTCALL event type to the WorkflowEvent union enables the system to handle hostcall requests, which is essential for the HostcallTransport implementation.
src/typegate/src/runtimes/deno/types.ts (2)
11-19: DenoMessage type properly extended to support hostcall responses.Similar to the WorkflowMessage type, the DenoMessage type has been expanded to a union type that includes the HOSTCALL_RESP message type. The implementation is consistent with the changes in the substantial runtime.
24-24: DenoEvent type properly extended for hostcall events.The addition of the HOSTCALL event type to the DenoEvent union maintains consistency with the WorkflowEvent type in the substantial runtime, ensuring a uniform approach to hostcall handling across runtimes.
src/typegate/src/services/auth/protocols/oauth2.ts (2)
168-168: Enhanced method declaration withoverridekeywordAdding the
overridekeyword explicitly indicates that this method is intended to override the method from the parentProtocolclass. This is a good practice as it:
- Makes the inheritance relationship clearer
- Provides compile-time checking if the method signature doesn't match the parent
- Improves code maintainability for future developers
284-287: Improved error logging for OAuth token refresh failuresThe enhanced error logging captures both the error and client data for better debugging context. This will help with troubleshooting token refresh issues in production environments.
Is the format string
"XXX error refreshing oauth token {}"intentional? TheXXXprefix is often used in temporary debugging code and might not be suitable for production. Consider checking if this is the intended log message format.tests/metagen/typegraphs/identities/rs/Cargo.toml (1)
1-11: Package restructuring aligned with workspace configurationThe changes to the package definition and dependencies align this crate with the workspace configuration:
- Updated package name from
metagen_identities_fdktoidentities_fdk- Upgraded version from
0.0.1to0.5.1-rc.0- Simplified dependency management using workspace references
This brings consistency with other similar restructuring across the project and makes dependency management more centralized.
src/metagen-client-rs/src/lib.rs (2)
4-12: Enhanced module organization with feature flagsThe changes to the module declarations improve the library's organization by:
- Making key modules (
args,files,graphql) publicly accessible- Using conditional compilation with feature flags for
filesandgraphqlmodules- Adding a new
commonmodule to share functionality- Clearly defining the relationship with the
hostcallmoduleThis modular approach improves separation of concerns and allows consumers to include only the features they need.
29-39: Updated prelude exports to match module visibility changesThe prelude module has been updated to correctly expose the new module organization:
- Added
commonmodule exports- Applied feature flags consistently to ensure conditional compilation works properly
- Changed from
reqwest::Urltourl::Urlto align with other dependency changesThese changes ensure the prelude remains in sync with the module structure.
src/typegate/src/runtimes/python.ts (1)
130-132: Updated URL protocol to support hostcall transportThe URL protocol has been changed from
internal+witwire://tointernal+hostcall+witwire://, which integrates with the new hostcall transport mechanism introduced in this PR. This change enables the PythonRuntime to leverage the new hostcall functionality for communication.tests/runtimes/wasm_reflected/rust/Cargo.toml (2)
2-4: Configuration standardization looks goodThe package renaming to "wasm_reflected_rust" and migration to workspace-level version and edition settings improves consistency and simplifies maintenance across the codebase.
10-10: Dependency management improvementMoving the wit-bindgen dependency to use workspace-level versioning is a good practice that will help maintain consistent dependency versions across the project.
tests/metagen/typegraphs/identities.py (4)
129-130: Dependency path updated correctlyThe update of all import paths from handlers_types.py to fdk.py aligns with the PR objective of consolidating code generation into fdk.py.
Also applies to: 136-137, 143-144, 150-151
153-159: New Python proxy primitives implementationThe addition of py_proxy_primitives correctly implements the hostcall transport mechanism for Python, maintaining consistency with the existing pattern while extending functionality.
190-196: TypeScript proxy primitives implementationThe addition of ts_proxy_primitives properly implements the hostcall transport mechanism for TypeScript, following the same pattern as the Python implementation.
219-223: Rust proxy primitives implementationThe addition of rs_proxy_primitives completes the implementation of hostcall transport across all three target frameworks as specified in the PR objectives.
src/typegate/src/runtimes/deno/worker.ts (4)
5-6: Updated imports for hostcall functionalityThe import changes correctly bring in the necessary components for the hostcall transport implementation, replacing the removed make_internal with the new HostcallPump class.
8-8: Initialization of HostcallPumpThe initialization of a HostcallPump instance properly prepares the environment for handling hostcalls.
11-14: Added hostcall response handlingThe new condition correctly handles HOSTCALL_RESP messages by delegating to the hostcallPump handler and returning early, ensuring proper separation of concerns.
33-42: Implemented hostcall transport mechanismThe implementation of hostcallPump.newHandler properly replaces the previous make_internal function, creating a handler that posts HOSTCALL messages with the required information.
The satisfies DenoEvent type check ensures type safety for the message structure.
tests/metagen/typegraphs/sample/ts/main.ts (3)
4-4: Updated import to include TransportsThe import has been correctly updated to include the Transports class, aligning with the PR objective of transitioning transport construction to a static method.
8-11: Updated GraphQL client initializationThe client initialization has been properly updated to use Transports.graphql instead of api1.graphql, reflecting the architectural change described in the PR objectives.
161-161: Updated prepared query and mutation implementationThe prepared query and mutation implementations have been correctly updated to align with the new transport mechanism while maintaining their functionality.
Also applies to: 166-166
docs/metatype.dev/docs/reference/typegraph/client/index.mdx (7)
35-35: Good simplification of QueryGraph descriptionThe description now focuses on query builders for root functions, which aligns well with the architectural change moving transport functionality to a separate namespace.
52-52: Updated Rust example to use factory function patternThe new
query_graph()function approach instead of the constructor pattern is more idiomatic for Rust and aligns with the PR's objective to transition transport construction to thetransportsmodule.
60-60: Clear explanation of the new Transport namespaceThe updated documentation clearly explains that transport construction is now handled by functions in the
Transportsnamespace, which matches the implementation changes described in the PR objectives.
71-77: Well-documented new HostcallTransportThe documentation provides a clear explanation of the new
HostcallTransportfunctionality, including its purpose and supported features, which is the main focus of this PR.
90-92: Updated Python example with new transport APIThe example now correctly uses
Transports.graphql_sync(qg, url)instead of calling the method directly on the query graph, which aligns with the API changes mentioned in the PR objectives.
117-119: Updated TypeScript example with new transport APIThe example now correctly uses
Transports.graphql(api1, url)instead of calling the method directly on the query graph, which is consistent with the API changes described in the PR objectives.
142-143: Updated Rust example for new transport APIThe example now correctly uses the factory function
query_graph()and the standalonetransports::graphql_syncfunction, which aligns with the migration notes in the PR objectives stating that transport construction has moved to thetransportsmodule.src/typegate/src/runtimes/deno/worker_manager.ts (3)
13-13: Added imports for hostcall functionalityCorrectly imported the
hostcallfunction andHostCallCtxtype, which are essential for implementing the HostcallTransport feature described in the PR objectives.
41-44: Updated constructor to support hostcall contextThe constructor now accepts a
hostcallCtxparameter, enabling the worker manager to handle host calls between the runtime and typegate.
74-95: Added hostcall event handlingImplemented support for the "HOSTCALL" event type, which allows the worker to make host calls to the typegate and respond with the results. This is a critical component for the HostcallTransport feature described in the PR objectives.
src/typegate/src/runtimes/substantial/deno_context.ts (4)
4-4: Updated import for HostcallPumpAdded import for the
HostcallPumptype which is used in the updated Context constructor signature.
15-18: Updated Context constructor for hostcall integrationChanged the Context constructor to accept a GraphQL handler from HostcallPump instead of a TaskContext, which aligns with the PR's objective to implement hostcall transport functionality.
423-436: Minor formatting improvementsImproved consistency in spacing around the switch statement and console methods, making the code more readable.
444-444: Improved error handlingChanged
catch (err)tocatch (_)since the error is not used, which is a good practice to indicate intentional ignoring of the error.src/metagen/src/fdk_rs/static/Cargo.toml (1)
10-14: Updated dependencies for workspace configurationThe changes transition from explicit version constraints to workspace-level dependency management, which improves consistency across the project. The addition of
metagen-clientsupports the new HostcallTransport implementation for Rust.src/metagen/src/fdk_rs/static/fdk.rs (3)
34-38: Appropriate Default implementation for MatBuilderAdding the
Defaultimplementation forMatBuilderenhances usability by allowingMatBuilder::default()to work. This is a common and recommended pattern when a type has a clear default state.
83-91: Conditionally integrating HostcallTransport in the Router handle methodThe changes correctly integrate the new hostcall transport functionality using conditional code generation with directives. This allows the code to be generated with or without hostcall support depending on the flags.
115-120: Expanded Ctx struct with conditional fieldsThe
Ctxstruct now includes additional fields when theHOSTCALLflag is enabled, allowing for proper integration with the hostcall transport system.src/metagen-client-rs/src/selection.rs (5)
660-663: Consistent changes to trait boundsSimilar to the previous comment, the trait bounds are also commented out here. Make sure these changes are consistent and well-tested.
785-791: Good addition of a simplified trait implementationAdding the simpler pattern for
impl_union_selection_traitsis a good improvement, providing a more concise option for types that don't need complex variant handling.
810-816: Consistent implementation of Selection traitAdding the
Selectiontrait implementation in the macro ensures consistency with other selection types and makes it easier to use the union selection types.
643-646:❓ Verification inconclusive
Review commented-out trait bounds
The commented-out trait bounds (
SelT: NotUnionSelection) might have been there for a specific reason to prevent invalid states. Since you're unsure about their purpose, it would be prudent to verify that removing these constraints doesn't allow invalid selections.
🏁 Script executed:
#!/bin/bash # Look for test failures or related issues in the git history git log -p -- src/metagen-client-rs/src/selection.rs | grep -A 10 -B 10 "NotUnionSelection" # Check for tests that might validate this behavior rg --type rust "UnionMember.*NotUnionSelection" "src/metagen-client-rs/tests/"Length of output: 147
Manual Verification Needed: Confirm that removing
SelT: NotUnionSelectiondoes not introduce invalid states
- The initial log search and test lookup did not produce any evidence regarding the impact of omitting
SelT: NotUnionSelection.- No direct references or tests were found that validate the constraint’s role in preventing invalid selections.
- Please manually verify edge cases associated with composite selections to ensure that the removal of this trait bound does not permit undesired states.
81-81:❓ Verification inconclusive
Change in function visibility may impact external consumers
Changing the visibility of
selection_to_select_nodefrompubtopub(crate)appropriately restricts this function to the crate's internal implementation details, which is good for API design. However, this could potentially break external code that was directly using this function.
🏁 Script executed:
#!/bin/bash # Check if this function is used outside of its crate rg --type rust "selection_to_select_node" --files-without-match "src/metagen-client-rs/src/selection.rs"Length of output: 102
Attention: Verify potential external usage of
selection_to_select_node
- Automated searches (using
rgwith exclusion of the file) did not reveal any references toselection_to_select_nodeoutside of its own file.- No
pub usere-exports were detected in the repository, suggesting that the function is not being exposed for external consumption.- While these findings indicate it is likely safe to restrict its visibility to
pub(crate), please manually verify (or perform additional searches) to ensure that no external crates or modules rely on this function.src/typegate/src/runtimes/substantial/agent.ts (3)
15-15: Adding hostcall imports for transport implementationThe import of
hostcallandHostCallCtxis necessary for the implementation of hostcall transport functionality, aligning with the PR objectives.
321-340: Well-implemented hostcall event handlingThe hostcall event handler is well-structured with proper error handling. It correctly processes hostcall requests and sends the response back through the worker manager.
The implementation aligns well with the existing pattern of handling other event types and includes appropriate error handling by catching exceptions and including them in the response.
47-52:✅ Verification successful
Constructor parameter addition for HostcallTransport integration
Adding the
hostcallCtxparameter to the constructor is the right approach for making this context available to the class methods. Making it public is appropriate since it needs to be accessed by methods like#eventResultHandlerFor.
🏁 Script executed:
#!/bin/bash # Check if all instances where Agent is constructed now include the hostcallCtx parameter rg --type typescript "new Agent\(" -A 5 src/typegate/Length of output: 90
Action: Verify Agent Constructor Usage with Updated Search Command
The addition of the
hostcallCtxparameter and exposing it publicly in the constructor is correctly addressing the HostcallTransport integration. To ensure that every instantiation ofAgentin TypeScript files incorporates this new parameter, please verify manually using an updated search command. Note that the previous command failed due to an unrecognized file type; try the following:
- Replace the unsupported
--type typescriptflag with the recognized-t tsflag.- Re-run the command in the repository root to inspect constructions of
Agent.Once you confirm that all
Agentconstructions now include the new parameter, we can consider the change approved.
🏁 Script executed:
#!/bin/bash # Verify that Agent instances in TypeScript files include the hostcallCtx parameter rg -t ts "new Agent\(" -A 5 src/typegate/Length of output: 609
Agent constructor update verified and approved
The updated constructor properly includes
hostcallCtxas the first parameter, and our verification confirms that all instances (e.g., insrc/typegate/src/runtimes/substantial.ts) are correctly passing this argument. No further changes are required for handling the HostcallTransport integration.src/metagen/src/client_rs/static/client.rs (4)
7-9: No concerns; prefer consistent environment gating.
These comments clearly mark environment-specific code conditions.
20-22: No immediate issues found.
Environment gating for theHOSTCALLscenario is well-documented and helps ensure that logic aligns with the correct runtime.
30-31: End-of-module definitions look consistent.
No additional feedback; the structural choices here are clear.
14-16:❓ Verification inconclusive
Validate synchronization approach for non-WASM targets.
graphql_syncis conditionally compiled fornot(target_family = "wasm"). Ensure that all synchronous needs (e.g., thread usage, blocking requests) are tested to avoid runtime stalls.
🏁 Script executed:
#!/bin/bash # Check if there's any usage of this function across non-WASM contexts to confirm proper usage. rg -A 3 'graphql_sync'Length of output: 5783
Action: Confirm robust testing for synchronous behavior in non-WASM targets.
Thegraphql_syncfunction is conditionally compiled and used in various test and sample modules across both Rust and Python. Please verify that its synchronous execution—especially regarding thread usage and blocking network requests—is thoroughly exercised in your non-WASM test suites to prevent any runtime stalls. If additional tests for blocking operations or thread safety are missing, add them to ensure the implementation behaves as expected.src/typegate/src/runtimes/wit_wire/mod.ts (1)
6-8:✅ Verification successful
Confirm the removal of logger references.
These logging imports and instances are commented out, indicating they are no longer needed. Verify if debugging or error logging is still required in other parts of the code.
🏁 Script executed:
#!/bin/bash # Scan the codebase for usage of the removed logger to ensure there's no remaining references. rg 'getLogger|logger'Length of output: 34166
Logger removal confirmed in wit_wire module
The commented logger imports and instantiation insrc/typegate/src/runtimes/wit_wire/mod.tshave been effectively removed. A repository-wide scan verifies that no active logger references remain in this file. Meanwhile, other parts of the codebase continue to use logging for debugging and error reporting as needed, so this localized removal does not impact overall functionality.src/metagen/src/client_ts/mod.rs (4)
122-124: Opt-in hostcall configuration is a neat approach.
Creating a dedicated options struct (GenClientTsOpts) is a good way to add or expand new flags later.
240-246: Conditional writing logic looks correct.
processed_writetoggles parts of the static template based onhostcall. This approach is maintainable and clarifies how to handle different build scenarios.
285-285: Consistent usage of empty array for selections.
Similar to line 260, maintaining consistent patterns around no-op transformations helps keep code uniform.
126-131:✅ Verification successful
Verify usage of
render_clientacross the codebase.
This function delegates torender_staticand then performs type rendering. Confirm references to ensure any previous direct calls torender_staticnow go throughrender_clientwhen needed.
🏁 Script executed:
#!/bin/bash # Check if there are direct calls to `render_static` outside `render_client`. rg 'render_static'Length of output: 579
Usage of
render_staticis appropriately encapsulated.I verified that in all modules (client_ts, client_rs, and client_py) the only invocations of
render_staticoccur within their respectiverender_clientfunctions. There are no direct calls torender_staticoutside these functions, confirming that any previous direct calls now correctly go throughrender_client.src/typegate/src/runtimes/wit_wire/hostcall.ts (1)
1-9: Kudos on adding this file and implementing requested functionalities!
No issues found in this introductory section. The license header and logger initialization look correct.src/metagen/src/fdk_rs/mod.rs (4)
218-223: Check naming consistency for therender_clientcall.
Ensure that the usage ofrender_clientis consistent with the newly introducedexclude_clientoption. If the option is meant to skip client generation altogether, confirm it doesn't conflict with any partial usage of client code.
310-361: Confirm local vs. remote dependency resolution formetagen-client.
- Debug mode uses a local path in
../metagen-client-rsvia#[cfg(debug_assertions)].- Non-debug mode references GitHub with a pinned tag.
This is generally good, but be sure that the local path is correct for all development machines and that the pinned version is properly updated in sync with your releases.
188-190:❓ Verification inconclusive
Verify the code generated when
exclude_clientistrue.
You currently branch to generate only thetypesmodule. Ensure the final crate still compiles without the client code present. Adding a quick test scenario for anexclude_clientsetup might be helpful.
🏁 Script executed:
#!/bin/bash # After generating code with exclude_client=true, check if it compiles cargo checkLength of output: 109
Action Required: Manually Verify Cargo Compilation with
exclude_clientEnabledThe automated verification script failed because the
cargocommand was not found in the current environment. Please runcargo checkon a system with Cargo installed to ensure that the crate compiles correctly whenexclude_clientis set totrue. Additionally, consider adding a quick test scenario or CI job that specifically tests theexclude_clientconfiguration to catch any issues arising from the omission of the client code.
- Ensure that the build environment includes Cargo.
- Confirm that only the
typesmodule is generated whenexclude_clientis enabled and that no compiler errors are introduced.
152-155:✅ Verification successful
Check for consistent naming and behavior when including/excluding the client.
Here, you're passing!self.config.exclude_client.unwrap_or_default()to determinehostcall. Confirm that in the rest of the codebase,exclude_client = truealways retains the same effect (disable the client).You can run this script to grep for other references to
exclude_client:
🏁 Script executed:
#!/bin/bash rg 'exclude_client'Length of output: 1024
Consistent client exclusion verified.
The grep results confirm that the implementation of theexclude_clientflag is consistent across the codebase. In modules like fdk_ts and fdk_py, the flag is checked withif config.exclude_client.unwrap_or_default(), while in fdk_rs the negation (!self.config.exclude_client.unwrap_or_default()) is applied when passing the value togen_cargo_toml—this inversion correctly converts the flag into the expected parameter (e.g., for enabling hostcall). In all cases, settingexclude_clientto true consistently disables the client as intended.Cargo.toml (4)
4-4: New workspace member added for Rust examples.The addition of the
"examples/typegraphs/metagen/rs"directory to the workspace members enhances the project structure by including Rust examples for metagen.
21-25: Added test directories to workspace members.The addition of test directories to workspace members is beneficial for ensuring that the test code follows the same development standards and can be easily managed together with the main codebase.
43-43: Disabled default features for metagen-client.Setting
default-features = falsefor the metagen-client dependency allows for more precise control over which features are enabled, potentially reducing the build size and compilation time.
141-141: Added wit-bindgen dependency.The addition of the wit-bindgen dependency is essential for WebAssembly Interface Type (WIT) support, which is needed for the hostcall transport implementation mentioned in the PR objectives.
src/metagen/src/fdk_rs/types.rs (2)
56-56: Added additional_props parameter to render_struct method.The new parameter allows for more flexible handling of additional properties in struct serialization.
210-210: Updated render_struct call with additional_props parameter.The call to render_struct now properly passes the additional_props parameter from the ObjectTypeData, ensuring consistent behavior.
tests/metagen/metagen_test.ts (5)
52-52: Added directory creation to ensure test reliability.Adding
Deno.mkdirwith the recursive option ensures the directory exists before attempting to write files, improving test reliability.
72-72: Fixed typo in comment.Corrected "enclose the generated create" to "enclose the generated crate" for better clarity.
89-89: Added environment variable for metagen client testing.Setting
METAGEN_CLIENT_RS_TEST = "1"enables specific test behavior for the metagen client, which aligns with the PR objectives of implementing the HostcallTransport.
286-288: Improved test structure using object with name property.Restructuring the test to use an object with a name property follows a more structured and maintainable pattern.
498-500: Added FIXME comment for known issue.The comment clearly documents a known issue with ambiguous unions/either in the fdk_rs implementation, which helps developers understand potential problems when working with this code.
tests/runtimes/wasm_wire/rust/Cargo.toml (2)
1-3: Updated package configuration to use workspace settings.Renaming the package to "wasm_wire_rust" and using workspace configuration for edition and version improves consistency across the project and better reflects the package's purpose.
10-14: Updated dependencies to use workspace configuration.Using workspace-level configuration for dependencies ensures consistent versions across the project and simplifies dependency management. Adding the metagen-client dependency aligns with the PR's focus on implementing the HostcallTransport.
src/typegate/src/runtimes/deno/deno.ts (7)
36-37: Good refactoring of functionsThe simplification of the
identityandinternal_policyfunctions to more concise arrow function expressions improves code readability while maintaining the same functionality.Also applies to: 42-43
104-107: Cleaner handling of outjection secretsThe simplification of the
matSecretsassignment using a ternary operator improves code clarity by making the condition and result more explicit.
108-110: Improved iteration over matSecretsUsing direct iteration over
matSecretsinstead of array indices is more idiomatic and reduces potential off-by-one errors.
166-174: Well-structured hostcall context implementationThe implementation of the
hostcallCtxobject properly encapsulates the required context data for hostcall transport (auth token, typegate instance, and typegraph URL) in a clean, maintainable way. This aligns with the PR objective of implementing theHostcallTransport.
178-178: Proper integration of hostcall contextThe
hostcallCtxis correctly passed to theWorkerManagerconstructor, ensuring the worker has access to the necessary context for hostcall operations.
192-194: Simplified deinit methodThe removal of the explicit return type annotation from the
deinitmethod while maintaining the resolved promise return is a good simplification.
260-261: Improved naming convention for unused parametersPrefixing unused parameters with underscore (
_verbose,_pulseCount) follows TypeScript conventions for indicating parameters that are intentionally unused.tests/metagen/typegraphs/sample/ts/client.ts (6)
26-34: Improved code formattingThe reformatting of the
nodeInstancesinitialization and the subsequentforloop improves readability while maintaining functionality.
211-218: Enhanced type definitionsThe reformatted type definitions for
SelectNodeOut<T>andQueryOut<T>are now more concise and readable while maintaining the same functionality.🧰 Tools
🪛 Biome (1.9.4)
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
645-650: Access modifier change enables inheritanceChanging the
#requestprivate method toprotected async requestenables subclasses to override or extend this functionality, which aligns with the hostcall transport implementation goals.
684-689: Improved ternary expression formattingThe reformatted ternary expressions for object creation are more consistent and readable while maintaining the same functionality.
Also applies to: 720-725
693-693: Updated method accessThe method calls have been correctly updated to use
this.requestinstead ofthis.#request, reflecting the change from private to protected access.Also applies to: 729-729
892-894: Class moved to ensure proper module structureMoving the
_QueryGraphBaseclass definition to the end of the file maintains a clean separation of concerns with the transport functionality.src/metagen/fixtures/client_rs/main.rs (4)
13-14: Updated client initializationThe client initialization has been updated to use
query_graph()instead ofQueryGraph::new(), aligning with the PR objective of transitioning transport construction from a method onQueryGraphto functions in thetransportsmodule.
18-18: Updated GraphQL client instantiationUsing
client::transports::graphql_sync(&api1, addr.clone())instead of the previous method chain approach properly implements the new transport pattern.
39-44: Enhanced type safety with complete structsThe code now uses complete struct types (
types::Post,types::RootCompositeArgsFnInput) instead of partial ones, improving type safety and making the intent clearer.Also applies to: 52-56, 96-104
69-69: Consistent use of transport moduleThe async version of the GraphQL client is now instantiated using
client::transports::graphql(&api1, addr), maintaining consistency with the sync version and properly implementing the new transport pattern.src/metagen-client-rs/src/files.rs (4)
4-4: Updated imports for better organizationUpdating the import to use
crate::common::*reflects the reorganization of code, which improves the module structure.
12-19: Proper platform-specific code handlingAdding
#[cfg(not(target_family = "wasm"))]attributes ensures that path-related functionality is only compiled for non-WASM targets, and marking unused variants with#[allow(dead_code)]prevents compiler warnings.
57-64: Enhanced file creation flexibilityThe addition of
from_pathandfrom_async_readermethods with appropriate conditional compilation provides more flexibility for creatingFileinstances on different platforms.Also applies to: 82-93
108-109: Consistent conditional compilationThe conditional compilation for
FileData::Pathhandling in bothTryFromimplementation andinto_reqwest_partmethod ensures consistent behavior across different target platforms.Also applies to: 114-115, 153-154
src/metagen/src/client_py/mod.rs (4)
7-8: Expose modules publiclySwitching from
mod types;andmod utils;topub mod types;andpub mod utils;makes these modules accessible externally. This is fine as long as you intend for them to be part of the public API.
118-122: Introducerender_clientcall withGenClientPyOptsUsing
GenClientPyOpts { hostcall: false }to control behavior is a neat approach. The logic in these lines seems consistent with the new rendering pattern.
236-240:❓ Verification inconclusive
Process
HOSTCALLparameter inrender_staticConditional processing based on
hostcallis sensible. Confirm that test coverage includes scenarios with bothtrueandfalsevalues.
🏁 Script executed:
#!/bin/bash # Search for any test usage or references to the 'hostcall' parameter. rg 'hostcall' --glob '*test*'Length of output: 1047
Action Required: Confirm full test coverage for both
hostcallcasesThe function
render_staticinsrc/metagen/src/client_py/mod.rsconditionally processes thehostcallparameter. Our search shows that several tests referencehostcall(e.g., intests/runtimes/wasm_wire/wasm_wire_test.ts,tests/runtimes/python/python_test.ts,tests/internal/internal_test.ts, and the metagen snapshots). However, it isn’t immediately clear that there are explicit tests covering bothhostcall == trueandhostcall == falsecases. Please verify that your test suite explicitly exercises both branches, and add additional tests as needed.
128-133:✅ Verification successful
New function
render_clientThe function signature and call to
render_staticappear consistent. Ensure other references across the codebase are updated to match usage ofrender_client.
🏁 Script executed:
#!/bin/bash # Verify all references to render_client in the repository to ensure consistent usage rg 'render_client' -A 3Length of output: 4058
Consistent usage of
render_clientconfirmed across the codebase.
The verification shows that the new function signature insrc/metagen/src/client_py/mod.rsis being used uniformly in all language-specific modules (client_py, client_rs, and client_ts) as well as in the FDK modules. No further adjustments are required.src/metagen/src/client_rs/mod.rs (8)
145-150: Adoptrender_clientwithGenClientRsOptsCalling
render_clientwithhostcall: falsealigns with your new approach. The overall flow here is approved.
155-160: Implementrender_clientfor RustThis function closely mirrors the Python counterpart. The calls to
render_staticand manifest generation look good.
179-182: Createquery_graphfunctionDefining the
QueryGraphmore explicitly helps keep the code structured. Implementation looks fine.
189-189: Add tuple mapping for GQL typesThe code snippet adding
("{ty_name}".into(), "{gql_ty}".into())for each type has no apparent issues.
206-207: Closequery_graphfunction outputThese closing lines finalize the generated string. Looks consistent with typical Rust code generation patterns.
216-221: Conditionally wrap composite out typesAutomatically prefixing composite types with
return_types::is a clean approach to differentiate them.
304-304: Unwrap onRc::into_innerUsing
unwrap()may panic if data is unexpectedly shared. Consider handling potential errors or verifying the single-owner assumption.
308-314: Render static content based onhostcallThis approach allows toggling hostcall logic in generated clients. Ensure integration tests cover both configurations.
tests/metagen/typegraphs/identities/py/handlers.py (6)
4-17: Refactor imports and decoratorsSwitching to
handler_*decorators and adjusting imports withfdkreferences align with the new naming scheme. No immediate issues.
20-22:@handler_primitiveswith dictionary accessUsing
inp["data"]is fine as long asinpis guaranteed to have"data". Double-check any test coverage to prevent KeyError.
25-27:@handler_compositesusageSame pattern with dictionary indexing. It’s consistent with the new approach.
30-32:@handler_cyclesusageNo issues here; straightforward change to the decorator and indexing approach.
35-38:@handler_simple_cyclesdecoratorThe dictionary-based return logic stays consistent. No concerns.
40-44: Newproxy_primitivesfunctionInvoking
ctx.host.querywith selection flags is a valuable extension. Verify that the proxied query is validated in your test suite.src/metagen-client-rs/src/graphql.rs (1)
1-755: Overall implementation looks solidThe refactoring around request building and improved error handling is well-structured. The new approach using
GqlRequestBuilderand dedicated blocking/non-blocking client structures appears consistent with the rest of the codebase. No major concurrency or security concerns stand out, and the usage ofArcfor placeholders is appropriate for sharing immutable data.src/metagen/src/client_ts/static/mod.ts (13)
23-25: Use of conditional alias resolution is clear.
The logic to handle both Alias and non-Alias paths by mapping them intonodeInstancesis straightforward and maintainable.
210-215: Generic conditional type is well-structured.
This conditional type forQueryOutis a helpful approach to unify node results.
442-442: Ensure stable naming & references in function signature.
Confirmed that the function signature forconvertQueryNodeGqlis coherent; no issues found.
448-450: Clarify node identity logic.
Using a ternary to decide if the nodeName matches instanceName is neat. Be mindful of potential collisions if any future logic modifies these names.
642-642: Visibility change to protected is valid.
Makingrequestprotected ensures subclasses can override or extend while restricting direct external calls.
681-681: Nice approach for single vs multiple queries.
UsingisNode ? { value: ... } : Object.fromEntries(...)elegantly handles single vs. multi-node queries in one pass.
682-686: Mapping query entries is concise.
This is a clear pattern for multi-node scenarios, ensuring each node’sinneris extracted.
714-714: Ensuring consistent detection withinstanceof MutationNode.
No issues found. This logic is straightforward.
718-722: Maintaining correct keys in the query object.
UsingObject.entriesensures we preserve each unique query key. Implementation looks good.
726-726: Long variable name clarifies meaning.
resultis generic, but the context clarifies usage.
775-807: New HostcallTransport class stands out.
This implementation neatly extendsGraphQLTransportto handle no-URL scenarios. The explicit error for file uploads is a helpful guard.
808-808: Ending brace of class.
No content to comment.
481-486:❓ Verification inconclusive
Functional approach to subNodes is clear.
Using.mapto recursively build subnode strings is straightforward. Ensure to handle optional or empty arrays without errors in upstream logic.Would you like to verify if any empty subNodes scenario might appear? If so, run:
and check if subNodes is guaranteed non-empty before calling.
--- 🏁 Script executed: ```shell #!/bin/bash rg -A5 'convertQueryNodeGql'Length of output: 8989
Review Update: Verify Upstream Handling of Optional/Empty
subNodesThe functional approach using
.mapand.join(" ")to build the subnode strings is clear and consistent with similar patterns seen elsewhere in the codebase. However, the current implementation (lines 481–486 insrc/metagen/src/client_ts/static/mod.ts) does not include an explicit check for an empty or undefinedsubNodesarray. Given that an empty array would simply yield an empty string, it’s important to confirm that the surrounding logic safely handles this case.
- Verify that upstream logic guarantees that
subNodesis always defined and, if empty, that an empty string output is acceptable.- If there’s any possibility that an empty array might lead to unexpected behavior or syntax issues, consider adding a safeguard.
examples/typegraphs/metagen/ts/fdk.ts (16)
36-45: Function_selectionToNodeSetintroduced for node processing.
This function’s signature and docstrings are clear. Consider documenting arguments more thoroughly if you expect external usage.
47-106: Argument objects enforcement.
You're enforcing argument objects with checks liketypeof arg != "object"orarg === null. This is robust. Ensure that your error messages remain user-friendly if this is part of a public API.
108-196: Subnodes and union variants logic is well-structured.
The approach to handle subNodes vs. variants is well-defined. Checking forunreachablescenarios fosters reliability.
198-207: Proper error handling for leftover foundNodes.
Throwing an error when unexpected nodes are found helps prevent silent mistakes.
208-213: Type definitions forSubNodesandSelectNode.
Clear and consistent definitions that reflect the structure.
214-219: Optional properties inSelectNodeare appropriate.
Ensures usage is flexible for subNodes or files.
221-293: FileExtractor implementation.
Your approach for dynamic file extraction is robust. Ensure performance remains acceptable for large arrays or deeply nested structures.🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
360-373: Alias class logic.
Creating multiple aliases is a valuable feature. This approach is consistent with your overall pattern.
447-459: PlaceholderValue pattern is well-designed.
This ensures placeholders are easily identifiable and replaced. The usage of.key()is clear.
467-475: GraphQlTransportOptions typed for clarity.
Allowing a customfetchfunction fosters testability. Good practice.
587-665:fetchGqlfunction looks robust.
Your logic for file uploads is well-structured, and the fallback to JSON request body is correct.
670-772:GraphQLTransportclass provides complete query/mutation lifecycle.
Protectedrequestmethod is well-scoped, allowing clean extension.
809-842:HostcallTransportextends the base class.
Explicitly rejecting file uploads clarifies capability constraints. No issues found.
844-925:PreparedRequestpattern.
This pattern fosters reusability and reduces overhead by pre-building GraphQL strings. The variable resolving approach is robust.
929-939:ErrorPolyfilltyping.
No issues; it looks entirely consistent with the transport’s error-throwing mechanism.
477-547:✅ Verification successful
convertQueryNodeGqlhandles arguments and subNodes thoroughly.
The recursion is comprehensive. Confirm that your type maps remain synchronized with all variants.Do you want to verify that all variant strings exist in
typeToGqlTypeMapacross the code? If so, you can run:--- 🏁 Script executed: ```shell #!/bin/bash rg -A10 'typeToGqlTypeMap'Length of output: 55023
SYNCHRONIZED TYPEMAP CHECK CONFIRMED
The recursive structure in
convertQueryNodeGqlis solid, and the error handling for missing variant mappings (i.e. throwing an error when a variant’s GraphQL type isn’t found intypeToGqlTypeMap) is implemented consistently across the codebase. The search results confirm that all usages oftypeToGqlTypeMap—especially within variant processing—follow the same pattern. Just remain vigilant to updatetypeToGqlTypeMapwhenever new variants are introduced.examples/typegraphs/metagen/rs/fdk.rs (6)
88-92: Add Default implementation for MatBuilder.
This is convenient for quickly instantiatingMatBuilder.
153-156: Context struct extended withqgandhost.
Allows easy access to the GraphQL transport capabilities.
235-249:transports::hostcallfunction introduced.
Methodically constructs aHostcallTransportfrom the localquery_graph. Straightforward.
255-259:QueryGraphstruct for storing type mappings.
No issues; this shapes up the system’s GraphQL type map.
343-352:query_graph()function supplies core type map.
This method is minimal and direct.
354-367:remixmethod on QueryGraph matches updated NodeMetas.
Good separation of query-building from actual request dispatch.src/metagen/src/fdk_py/mod.rs (3)
4-5: New imports look good.
96-114: Stubbed function generation is solid.
The logic that registers stubbed materializer implementations is straightforward. Ensure that new stubbed runtimes are updated consistently in all relevant places to avoid missing coverage.
232-292: End-to-end test coverage is appreciated.
These tests demonstrate practical usage and help validate the generated Python module. As a next step, you might add more specialized negative or edge-case tests to ensure robust coverage.tests/metagen/typegraphs/sample/py/client.py (5)
15-15: Abstract base class integration.
Introducing theABCandabstractmethodis a good design move to enforce consistent transport behavior across implementations.
779-787:prepare_queryoverloads.
Nice use of Python overloading for typed clarity. The final returned type is well-defined, ensuring consistent usage in calling code.
797-824: Prepared request builder.
Returning a typedPreparedRequestfosters a clean interface for parameterizing queries. This pattern is easy to extend if more specialized request configuration is introduced.
829-829:PreparedRequestclass introduced.
Providing a separate class for handling the dry-run node logic and argument resolution is a great way to keep concerns separated and manageable.
891-901: Staticgraphql_syncmethod.
Linking directly toGraphQLTransportUrlibis consistent with the rest of the transport patterns. Ensure that any additional synchronous or asynchronous transport variants get integrated the same way for uniform usage.src/metagen-client-rs/src/common.rs (4)
1-2: License headers.
Including explicit licensing is a good practice and ensures clarity for contributors.
44-50:GqlRequeststruct introduction.
The struct’s fields cover all aspects of a GraphQL request, including placeholders and file paths. This design accommodates both JSON-based and multipart requests effectively.
190-223:buildmethod for final GQL request.
Structuring the doc with typed variables is a robust approach. Careful usage ofOk(...)and custom errors ensures manageable debugging for invalid typegraph references.
238-291:handle_responsefunction.
The stepwise approach to parse, check errors, and reconstruct the node-indexed data is clear. The explicit mismatch error for missingnode{idx}fields helps with diagnosing partial or malformed responses.tests/metagen/typegraphs/identities/rs/fdk.rs (8)
88-93: ImplementingDefaulttrait forMatBuilderlooks good.The
impl Defaultblock cleanly delegates toSelf::new(). This is consistent with Rust best practices and simplifies code usage.
137-141: New context initialization inhandlemethod.Creating a
QueryGraphand passing it intoCtxwithhost: transports::hostcall(&qg)is a neat encapsulation. No immediate issues. Just ensure thatquery_graph()has minimal overhead and is free from data races if used concurrently.
152-156: Introduction ofCtxstruct.Defining the
Ctxstruct with both theqgandhostfields improves clarity when passing context around. This can make code more readable and maintainable by grouping all relevant data in one place.
235-236: New imports for PhantomData and prelude.Using
PhantomDatais useful for zero-sized type markers, and importing frommetagen_client::preludeindicates usage of shared definitions. Both lines look fine and do not raise concerns.
238-257:transportsmodule.Introducing multiple functions for different transport mechanisms (
graphql,graphql_sync,hostcall) modularizes the code and clarifies the intended usage in various environments. Ensure that any error statuses or exceptions at the transport level are handled consistently in higher layers.
259-266: CloningQueryGraph.The
#[derive(Clone)]onQueryGraphallows creating copies of the type mapping with ease. This can be beneficial for concurrency models. Just watch out for large memory usage ifty_to_gql_ty_mapgrows large and is cloned frequently.
908-924:query_graphfunction definition.Providing a shared function for constructing
QueryGraphcentralizes the relevant mappings. This ensures consistent usage across the codebase. The typed keys and the straightforwardArcusage help with thread-safety.
927-1107: New methods forQueryGraph(py_xxx, ts_xxx, rs_xxx).The parameterized builder pattern with typed arguments (
NodeArgs<...>) and typed returns, e.g.UnselectedNode<...>, appears well-structured. It ensures compile-time checks and simpler expansions in the future. Make sure that any panicking or misconfiguration in nested calls is properly handled upstream.tests/internal/py/fdk.py (2)
1-16: General file header and imports.File-level docstrings and typed imports look good. Imports are broadly scoped but appear necessary for the GraphQL transport functionalities, HTTP support, file handling, and type hints.
18-28: Definition ofCtxclass.The constructor references a
HostcallBinding, aQueryGraph, and aHostcallTransport, all of which are injected as dependencies. This is a clean design for context injection in Python, keeping responsibilities well isolated.examples/typegraphs/metagen/py/fdk.py (1)
1-16: File header and imports.Similar to the previous Python file, the imports and standard library usage look appropriate for networking, IO, and advanced type hints.
tests/runtimes/wasm_wire/rust/fdk.rs (6)
88-93: Implementation ofDefaultforMatBuilderlooks good.
No correctness or performance issues are apparent. This straightforward approach helps maintain consistency by delegating toSelf::new().
137-141: Ensure consistent error handling for hostcall transport creation.
The code creatingCtxwithqgand thehosttransport looks correct. However, be sure to confirm that any errors arising intransports::hostcallare handled gracefully upstream if needed.
152-156: NewCtxstruct fields appear correct.
Exposing both theQueryGraphandHostcallTransportis consistent with the PR objectives of enabling advanced query operations.
235-249: Validate side effects of the newhostcallfunction.
The function returns aHostcallTransportreferencingsuper::hostcallwithout explicit error handling. Confirm that any potential runtime errors insuper::hostcallwon't cause unexpected behavior.
251-254: Commented placeholders.
These lines primarily introduce doc-like comments or placeholders. No further action required.
255-258: Definition ofQueryGraphis concise and clear.
Cloning thety_to_gql_ty_mapis sensible for safe concurrency.src/metagen/src/client_py/static/client.py (2)
18-18: Abstract base import is fine.
No further concerns for correctness or maintainability.
777-830:prepare_queryandprepare_mutationmethods
Providing pre-built queries is helpful for reusability. Ensure any placeholders in arguments are tested thoroughly to avoid runtime selection errors.src/metagen-client-rs/src/hostcall.rs (11)
1-3: File header and licensing
Acknowledged. No issues detected.
4-10: Imports and references
These references look correct for bridging the new binding approach inHostcallTransport.
22-28:HostcallTransport::new
Constructor ensures consistent assignment. No concerns or improvements noted.
66-86:queryandquery_with_opts
Both methods delegate relatively cleanly. Just ensure that the default opts do not hide important hostcall configuration if advanced usage is needed.
108-128:prepare_query&prepare_query_with_opts
Preparation logic is consistent with the rest of the code. Check for usage in Python bridging to ensure the same partial-binding approach is tested.
129-148:prepare_mutation&prepare_mutation_with_opts
Similar to query preparation. Looks good, no immediate concerns.
150-160:PreparedRequestHostcallstruct
Clear definition and alignment with the new approach.
162-240: Implementation ofperform()
Deserialization logic is well-handled, returning structured errors. This is essential for debugging and user feedback.
241-256: ManualCloneimplementation
This is necessary becauseDocmight not beClone. Implementation is correct.
258-311:HostcallError
Good variety of errors. Ensure that future expansions (like network timeouts or file upload issues) remain consistent with this approach.
312-324:impl From<GraphQLRequestError>
Conversion logic effectively covers all relevant error variants.tests/metagen/typegraphs/sample/py_upload/client.py (4)
15-15: No concerns on the added imports.
ImportingABCandabstractmethodis a clear indication that the code now uses abstract classes.
579-589: Abstract base class looks good.
This base class formalizes the transport interface. It adds clarity and enforces the contract for any subclass needing to implement the required methods.
779-799: Enhanced method signatures and return types are consistent.
The updated signatures returningPreparedRequest[...]instances and the union-typed overloads are well-structured. Using string-based forward references is a commonly accepted pattern for generic return types.Also applies to: 806-826
891-901: Transport factory logic is clear and straightforward.
The static factory method centralizes the construction ofGraphQLTransportUrlib, improving maintainability. No concerns here.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
tests/metagen/metagen_test.ts (1)
514-550: Typo in test case name and potential duplicate test.There's a typo in the test case name "proxy primtives" (should be "proxy primitives"). Also, this test case appears very similar to the existing "primtives" test case on line 436.
- name: "proxy primtives", + name: "proxy primitives",Additionally, verify if this is intended to be a distinct test from the "primtives" test case, as they contain identical data and GraphQL queries. If they serve different purposes, consider adding a comment explaining the distinction.
src/metagen-client-rs/src/common.rs (5)
19-21: Remove or clarify commented-out code
The code snippet is partially commented out and might be obsolete. Consider removing it for cleanup or adding a comment explaining why it's retained.-// fn path_segment_as_prop(segment: &str) -> Option<&str> { -// segment.strip_prefix('.') -// }
90-180: Consider splitting theselect_node_to_gqlfunction
This function handles complex branching logic for query building. Splitting it into smaller, more maintainable methods or using helper functions can improve readability and reduce complexity.
238-280: Refine error handling for edge cases
Currently, the code handles only 2xx HTTP status codes as success. Consider handling additional status scenarios such as redirections (3xx) or request-timeouts (408), if applicable, or at least log them for better observability.
281-345: Fix spelling mistakes in error descriptions
"recieived" is misspelled. Trivial fix to standardize the user-facing messages.- /// GraphQL errors recieived + /// GraphQL errors received ... - /// Http error codes recieived + /// Http error codes received
411-418: Assess coverage forPrepareRequestError
Ensure all error variants inPrepareRequestErrorare tested, especially file upload and placeholder resolution paths, to maintain confidence in the error-handling logic.src/metagen/src/utils.rs (5)
4-5: Add documentation for the imported module.The import
use crate::interlude::*;brings in symbols from the interlude module, but the asterisk import makes it unclear what symbols are being imported. Consider documenting what key elements are being used from this module.
37-43: Consider extracting regex patterns as named constants.The regex patterns are defined as static variables, but the names could be more descriptive to better indicate their purpose. Additionally, consider consolidating the regex documentation with the pattern definition for easier maintenance.
- static SKIP: once_cell::sync::Lazy<regex::Regex> = - once_cell::sync::Lazy::new(|| regex::Regex::new(r"metagen-skip").unwrap()); - static IF_START: once_cell::sync::Lazy<regex::Regex> = once_cell::sync::Lazy::new(|| { - regex::Regex::new(r"metagen-genif(?<not>-not)?\s+(?<name>\S+)").unwrap() - }); - static IF_END: once_cell::sync::Lazy<regex::Regex> = - once_cell::sync::Lazy::new(|| regex::Regex::new(r"metagen-endif").unwrap()); + /// Regex that matches the skip directive: "metagen-skip" + static SKIP_DIRECTIVE: once_cell::sync::Lazy<regex::Regex> = + once_cell::sync::Lazy::new(|| regex::Regex::new(r"metagen-skip").unwrap()); + + /// Regex that matches conditional start directives: + /// - "metagen-genif <flag>" (include if flag is true) + /// - "metagen-genif-not <flag>" (include if flag is false) + static CONDITIONAL_START_DIRECTIVE: once_cell::sync::Lazy<regex::Regex> = once_cell::sync::Lazy::new(|| { + regex::Regex::new(r"metagen-genif(?<not>-not)?\s+(?<name>\S+)").unwrap() + }); + + /// Regex that matches the end of a conditional block: "metagen-endif" + static CONDITIONAL_END_DIRECTIVE: once_cell::sync::Lazy<regex::Regex> = + once_cell::sync::Lazy::new(|| regex::Regex::new(r"metagen-endif").unwrap());And update references in the code:
- if SKIP.is_match(line) { + if SKIP_DIRECTIVE.is_match(line) {- if let Some(caps) = IF_START.captures(line) { + if let Some(caps) = CONDITIONAL_START_DIRECTIVE.captures(line) {- } else if IF_END.is_match(line) { + } else if CONDITIONAL_END_DIRECTIVE.is_match(line) {
61-62: Add a comment explaining the condition logic.The logic for evaluating conditions with the
notmodifier is concise but might not be immediately clear to all developers. Consider adding a clarifying comment.let condition = flags.get(flag).copied().unwrap_or(false); + // If "not" is present, invert the condition let condition = condition ^ not.is_some();
104-106: Consider adding an early return comment.The early return logic is correct, but a comment would help clarify its purpose - returning the original input unchanged when no matches are found.
let Some(first) = matches.next() else { + // No matches found, return the original input unchanged return input.to_string(); };
124-136: Simplify the post-non-matches logic.The logic for identifying non-matching lines after the first match is correct but somewhat complex. Consider adding a comment or refactoring to make it more readable.
- // Add non-matching lines after first match - let post_non_matches = lines[first + 1..] - .iter() - .enumerate() - .filter_map(|(rel_i, line)| { - let original_i = first + 1 + rel_i; - if matches_set.contains(&original_i) { - None - } else { - Some(*line) - } - }); + // Add non-matching lines that appear after the first match + let post_non_matches = lines[first + 1..] + .iter() + .enumerate() + .filter_map(|(rel_i, line)| { + let original_i = first + 1 + rel_i; + // Only include lines that weren't already added as matches + (!matches_set.contains(&original_i)).then_some(*line) + });docs/metatype.dev/docs/reference/metagen/index.mdx (1)
171-175: Ensure Consistent Punctuation in fdk_ts Extra Keys Table
The table for thefdk_tsgenerator’s extra configuration keys (specifically the description forexclude_client) looks good overall. However, a static analysis hint indicates that the description on line 174 may be missing a punctuation mark at the end. Consider appending a period to enhance readability and consistency.🧰 Tools
🪛 LanguageTool
[uncategorized] ~174-~174: A punctuation mark might be missing here.
Context: ...o not include the typegraph client fromclient_ts. | ### `fdk_p...(AI_EN_LECTOR_MISSING_PUNCTUATION)
src/typegate/src/runtimes/wit_wire/hostcall.ts (1)
16-51: Good error handling pattern but potential security concern in error detailsThe hostcall function has good structure with proper try/catch blocks, but it might expose too much information in error messages. Consider sanitizing sensitive data in error causes before throwing.
} catch (err) { logger.error("error on wit_wire hostcall {}", err); if (err instanceof Error) { throw { message: err.message, cause: err.cause, - ...(typeof err.cause == "object" && err + ...(typeof err.cause == "object" && err && !containsSensitiveData(err.cause) ? { // deno-lint-ignore no-explicit-any code: (err.cause as any).code ?? "unexpected_err", } : { code: "unexpected_err", }), };And add a helper function to sanitize error data:
function containsSensitiveData(cause: any): boolean { // Check for tokens, credentials, or other sensitive data return false; // Implement appropriate checks }src/metagen/src/client_rs/static/client.rs (1)
16-17: Consider error handling for GraphQL transport initializationCurrently the
graphqlfunction does not handle potential errors during transport initialization. Consider adding error handling or return a Result type.-pub fn graphql(qg: &QueryGraph, addr: Url) -> GraphQlTransportReqwest { - GraphQlTransportReqwest::new(addr, qg.ty_to_gql_ty_map.clone()) +pub fn graphql(qg: &QueryGraph, addr: Url) -> Result<GraphQlTransportReqwest, TransportError> { + Ok(GraphQlTransportReqwest::new(addr, qg.ty_to_gql_ty_map.clone()))src/metagen/src/client_ts/static/mod.ts (2)
208-213: Consider renaming the type parameter to avoid potential redeclaration.
According to static analysis, the identifierOmight be redeclared elsewhere or cause confusion. You can rename it to something likeUto avoid collisions.- type SelectNodeOut<T> = T extends QueryNode<infer O> | MutationNode<infer O> ? O + type SelectNodeOut<T> = T extends QueryNode<infer U> | MutationNode<infer U> ? U🧰 Tools
🪛 Biome (1.9.4)
[error] 208-208: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
915-944: Consider refactoring the static-only class.
The static analysis tool flagged that this class only contains static members. In some code style guidelines, a simple module of functions is preferred over a class to avoid overhead and confusion.🧰 Tools
🪛 Biome (1.9.4)
[error] 915-944: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/metagen/src/client_py/static/client.py (1)
991-1014: Refactor static-only class if desired.
Similar to the TS file, consider converting theTransportsclass into a simple collection of functions if you align with the style guide that discourages classes containing only static methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.toml(3 hunks)docs/metatype.dev/docs/reference/metagen/index.mdx(6 hunks)rust-toolchain.toml(1 hunks)src/metagen-client-rs/Cargo.toml(1 hunks)src/metagen-client-rs/src/common.rs(1 hunks)src/metagen/src/client_py/static/client.py(10 hunks)src/metagen/src/client_rs/mod.rs(7 hunks)src/metagen/src/client_rs/static/client.rs(1 hunks)src/metagen/src/client_ts/static/mod.ts(13 hunks)src/metagen/src/fdk_py/static/fdk.py(1 hunks)src/metagen/src/fdk_rs/static/fdk.rs(3 hunks)src/metagen/src/fdk_rs/types.rs(4 hunks)src/metagen/src/utils.rs(2 hunks)src/typegate/src/runtimes/wit_wire/hostcall.ts(1 hunks)tests/metagen/metagen_test.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- rust-toolchain.toml
- src/metagen/src/fdk_py/static/fdk.py
- src/metagen-client-rs/Cargo.toml
- src/metagen/src/fdk_rs/types.rs
- Cargo.toml
🧰 Additional context used
🪛 Biome (1.9.4)
src/metagen/src/client_ts/static/mod.ts
[error] 208-208: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 915-944: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 LanguageTool
docs/metatype.dev/docs/reference/metagen/index.mdx
[uncategorized] ~174-~174: A punctuation mark might be missing here.
Context: ...o not include the typegraph client from client_ts. | ### `fdk_p...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (43)
tests/metagen/metagen_test.ts (7)
52-52: Good addition: Directory creation ensures path existence.Adding this directory creation code improves reliability by ensuring the destination path exists before any files are written.
72-73: Typo corrections in comments.Fixed typographical errors: "create" → "crate" and "worksapce" → "workspace".
286-288: Test definition updated to use object notation.Refactoring the test definition to use an object with a
nameproperty improves consistency with other tests in the file that also use this pattern (lines 590-592, 718-720).
498-499: Good documentation of known issue.Adding a FIXME comment clearly documents the known issue with ambiguous unions/either in the fdk_rs implementation, which helps with future debugging and fixes.
708-709: Redirected stderr output for debugging.Updating the command output handling to redirect stderr to the parent process helps with debugging by making error messages visible.
1-716: Hardcoded temporary directory path.Replacing
t.tempDirwith a hardcoded path ("/tmp/stuff") reduces test portability across different environments and may cause issues on non-Unix systems.- const tmpDir = "/tmp/stuff"; + const tmpDir = t.tempDir;
89-89:✅ Verification successful
Added environment variable for Rust client testing.
The new environment variable
METAGEN_CLIENT_RS_TESTappears to enable specific test behavior for the Rust client.
🏁 Script executed:
#!/bin/bash # Check where this environment variable is used in the codebase rg -A 3 "METAGEN_CLIENT_RS_TEST" --type rustLength of output: 852
Verified: Environment variable usage for Rust client testing is correct
The environment variable
METAGEN_CLIENT_RS_TESTis used as intended:
- In
src/metagen/src/client_rs/mod.rs, the variable is read (usingstd::env::var) to enable specific test behavior.- In
src/metagen/src/tests/mod.rs, the tests set and remove the variable as expected.No further changes are necessary.
src/metagen-client-rs/src/common.rs (1)
380-409: Verify unreachable assumption
unreachable!()indicates an assumption guaranteed by prior logic. If there's any possibility of user input or external data leading here, prefer returning a meaningful error instead.docs/metatype.dev/docs/reference/metagen/index.mdx (4)
121-128: Clarify Extra Configuration Keys for Client-Based Generator
This new table listing the keyscrate_name,skip_cargo_toml, andskip_lib_rsis clear and informative. Please verify that it’s placed under the correct generator section (likely for theclient_rsgenerator) and that its scope is distinct from the similar keys later in the file.
184-186: Update fdk_py Generator List Items
The updated bullet list now clearly explains that the custom function decorators will use thestubbed_runtimesselection and that aclient_pybased typegraph client with a specialHostcallTransportimplementation is supported. The changes improve clarity; just ensure the indentation and style remain consistent with similar sections in the document.
217-223: Review fdk_py Extra Configuration Keys Table
The additional table for thefdk_pygenerator extra configuration keys—featuringstubbed_runtimesandexclude_client—is well formatted and aligns with the new documentation updates. No issues detected here.
279-280: Verify fdk_rs Extra Configuration Keys Table Formatting
The extra configuration keys table for thefdk_rsgenerator now correctly listsskip_lib_rsandexclude_client(with the proper anchor in the description). The table is consistent with the documentation style seen in other sections.src/typegate/src/runtimes/wit_wire/hostcall.ts (2)
10-14: Well-designed context type for hostcall functionalityThe
HostCallCtxtype properly encapsulates all required context for making host calls: the Typegate instance, authentication token, and target typegraph URL. This follows good design principles by grouping related data.
53-116: Well-implemented GraphQL transport with proper validationThe
gqlfunction effectively:
- Validates inputs with Zod schema
- Handles variable conversion from string to object
- Properly constructs the request with appropriate headers
- Provides detailed error messages
The TODO comment on line 98 suggests future improvements for internal request handling, which is good practice for noting technical debt.
src/metagen/src/client_rs/static/client.rs (1)
4-36: Well-structured transport module with clear documentationThe new
transportsmodule properly implements the restructuring mentioned in the PR objective by moving transport functionality out of theQueryGraphmethods. The documentation clearly explains each transport type and their purposes.A few considerations:
- The conditional compilation directives (
metagen-genif-not HOSTCALL) are appropriately used- Each transport function follows a consistent pattern, taking the query graph and necessary parameters
- The hostcall transport correctly uses an Arc for thread safety
src/metagen/src/fdk_rs/static/fdk.rs (4)
34-38: Good implementation of Default for MatBuilderAdding the Default implementation for MatBuilder improves ergonomics by allowing users to create instances more easily with
MatBuilder::default().
83-90: Conditionally initialized context with hostcall supportThe code effectively uses conditional compilation to initialize the context with hostcall support when enabled. This aligns with the PR objective of implementing hostcall transport across frameworks.
102-114: Well-designed stubs for non-hostcall buildsThese stubs provide appropriate placeholder implementations when HOSTCALL is not enabled, ensuring the code compiles correctly regardless of build configuration. The
todo!()macro in the hostcall function correctly indicates that this is a stub that shouldn't be called.
115-120: Properly updated Ctx struct with new transport fieldsThe Ctx struct has been appropriately updated to include the QueryGraph and HostcallTransport fields, which are required for the hostcall functionality per the PR objective.
src/metagen/src/client_rs/mod.rs (4)
145-148: Updated render_client_rs to use the new render_client functionThe code now properly delegates to the new
render_clientfunction with default options for hostcall, which improves modularity and allows for different configurations.
151-159: Good design for options structThe introduction of
GenClientRsOptsfollows good design principles by:
- Grouping related configuration options
- Using a struct for future extensibility (more options can be added)
- Using descriptive names that clearly indicate purpose
307-316: Improved render_static with hostcall parameterThe render_static function has been updated to support the hostcall parameter, using the processed_write utility to conditionally include or exclude sections of code based on the parameter.
320-390: Refactored render_data_types to separate types and return typesThe refactored function now correctly separates regular types and partial return types, returning both as part of a tuple. This is a significant improvement that:
- Organizes types more logically
- Makes partial vs. non-partial type handling explicit
- Properly separates concerns between different type categories
src/metagen/src/client_ts/static/mod.ts (8)
23-31: Looks good.
This logic correctly determines whether the node selection should be treated as an alias or a single node object, then iterates over the node instances. The approach is clear and should handle both scenarios properly.
138-141: No concerns found.
The function invocation with thevariant_selectselection parameter is consistent with the usage pattern established above.
448-450: Ternary assignment foroutis clear.
This concise logic for determining the alias or direct node name is straightforward and maintains readability.
481-487: String construction for array subNodes looks correct.
This snippet usesArray.mapand.join(" ")to accumulate the subNode queries, which is a standard and clear approach.
488-508: Union variant expansion is well-handled.
The code handles each variant’s GraphQL type in a structured way, including a check for missing mappings and a fallback error. This is a robust approach.
642-642: Protected access modifier broadens extensibility.
Switching from private to protected is appropriate if subclasses need to override or call this method.
681-686: Good conditional object construction.
By checkingisNode, you either wrap a single value under"value"or map over the entire query. This is a clean solution to support both single-node and multi-node queries.
690-690: Async invocation alignment.
Usingawait this.request(doc, variables, options ?? {})is consistent with the rest of the class’s asynchronous pattern.src/metagen/src/client_py/static/client.py (12)
1-9: No issues with these import and directive lines.
They appear to be required for environment checks and conditional compilation logic.
15-17: Imports look consistent.
Usingurllib.requestfor network calls anduuidfor form boundary generation is standard.
31-31: Boolean coercion is straightforward.
Casting tobool(sub_flags and sub_flags.select_all)is a clear approach here.
579-579: Good approach using an abstract base class.
DefiningGraphQLTransportBase(ABC)makes the intended subclassing clearer.
621-624: Thank you for avoiding mutable defaults.
This change (initializingfilesto{}only whenNone) addresses the classic Python pitfall with mutable default arguments.
681-683: Signature additions appear correct.
Adding optional files to the abstract method clarifies what subclasses must handle.
740-747: Instantiation logic is straightforward.
DeclaringfilesasNoneby default and reassigning withinfetch()ensures no shared mutable state across calls.
835-837: Type alias clarity.
HostcallBindinghelps specify the callable signature for host calls, improving code readability.
840-870: HostcallTransport implementation is clear.
Mirroring the TS implementation, this class gracefully omits file upload support with a proper exception.
871-897: Overloaded method definitions are well-organized.
Offering typed overloads forprepare_queryensures consistent user experience.
899-924: Mutation preparation follows the same consistent pattern.
This symmetry betweenprepare_queryandprepare_mutationpromotes familiarity and maintainability.
929-929: PreparedRequest class is well scoped.
Encapsulating the logic for queries and variables in a single object is clean and reusable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
tests/metagen/typegraphs/identities/rs/fdk.rs (1)
212-214: Consider adding more documentation for return_types moduleWhile the code structure is clear, adding documentation comments for the
return_typesmodule would improve code maintainability, especially explaining the distinction between regular types and partial types.tests/metagen/typegraphs/sample/ts_upload/client.ts (1)
810-810: Add a comment to explain TODOThere's a TODO comment about file support for prepared requests, but it lacks context. Consider adding more details about what needs to be fixed and why.
- // FIXME: file support for prepared requets + // FIXME: file support for prepared requests - implement file extraction and handling for prepared requests similar to the regular request flowtests/metagen/typegraphs/identities/ts/fdk.ts (2)
243-243: Avoid redeclaring the type parameter'O'.Declaring
'O'multiple times (e.g., in type intersections or extends) can lead to collisions or confusion in more complex scenarios. Consider renaming this type parameter to something else to avoid potential confusion or overshadowing.🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
266-266: Consider using optional chaining for clearer and safer checks.Instead of:
if (path[0] && path[0].startsWith("." + key)) {You could write:
- if (path[0] && path[0].startsWith("." + key)) { + if (path[0]?.startsWith("." + key)) {This helps prevent potential runtime errors if
path[0]is undefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/metagen/typegraphs/identities/py/fdk.py (4)
18-28: Enhance theCtxclass with docstrings and validations.Currently, the
Ctxclass constructor does not explicitly validate or document its parameters. Adding docstrings and optional checks (e.g., ensuringbinding,qg, orhostis notNone) can improve maintainability and clarity.
30-46: Consider splitting logic forselection_to_nodesinto multiple helper functions.The function is fairly large and handles multiple responsibilities (selection validation, argument checking, union expansions, etc.). Extracting parts (e.g., argument validation or variant expansion logic) into smaller helpers can reduce complexity and improve readability.
279-290: Sanity-check recursion paths inFileExtractor.When arrays are deeply nested, the recursive calls and path tracking could become difficult to trace. Adding extra logs or a maximum recursion depth might prevent accidental or maliciously deep structures from causing issues.
848-873: Clarify lack of file support inHostcallTransport.The code raises an exception if a file upload is attempted, which is expected. Documenting this behavior with docstrings or clarifying in relevant documentation can reduce confusion for future maintainers or users.
src/metagen/src/client_py/static/client.py (2)
8-8: Confirm whether conditionally ignoring hostcall code is intended.The comment suggests a skip for hostcall features. If you plan to unify code paths later, ensure it doesn't lead to dead code or missing coverage.
580-580: Consider factoring out repeated build logic forGraphQLTransportBase.If multiple subclasses replicate the same structure (multipart form building, setting headers, etc.), centralizing these steps in a shared helper or utility function fosters better maintainability and reduces duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/metatype.dev/docs/reference/metagen/index.mdx(6 hunks)src/metagen/src/client_py/static/client.py(10 hunks)src/metagen/src/client_rs/static/client.rs(1 hunks)tests/metagen/typegraphs/identities/py/fdk.py(1 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs(7 hunks)tests/metagen/typegraphs/identities/ts/fdk.ts(2 hunks)tests/metagen/typegraphs/sample/py/client.py(10 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(12 hunks)tests/metagen/typegraphs/sample/rs/client.rs(15 hunks)tests/metagen/typegraphs/sample/rs_upload/client.rs(3 hunks)tests/metagen/typegraphs/sample/ts/client.ts(13 hunks)tests/metagen/typegraphs/sample/ts_upload/client.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/metatype.dev/docs/reference/metagen/index.mdx
- src/metagen/src/client_rs/static/client.rs
- tests/metagen/typegraphs/sample/rs_upload/client.rs
- tests/metagen/typegraphs/sample/py_upload/client.py
🧰 Additional context used
🧠 Learnings (1)
tests/metagen/typegraphs/identities/ts/fdk.ts (1)
Learnt from: Yohe-Am
PR: metatypedev/metatype#982
File: tests/metagen/typegraphs/identities/ts/fdk.ts:942-969
Timestamp: 2025-03-10T21:56:34.439Z
Learning: The `Transports` class in TypeScript should be maintained as a class with static methods (rather than refactored to top-level functions) to ensure consistency across language implementations and improve API discoverability through namespacing.
🪛 Biome (1.9.4)
tests/metagen/typegraphs/identities/ts/fdk.ts
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 942-969: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 243-243: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
tests/metagen/typegraphs/sample/ts/client.ts
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/metagen/typegraphs/sample/ts_upload/client.ts
[error] 211-211: Shouldn't redeclare 'O'. Consider to delete it or rename it.
'O' is defined here:
(lint/suspicious/noRedeclare)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (29)
tests/metagen/typegraphs/sample/py/client.py (8)
578-683: Implementation of abstract base class enhances modularity and extensibility.The introduction of
GraphQLTransportBaseas an abstract base class with an abstractfetchmethod provides a clean interface for different transport implementations. This aligns with the SOLID principles, specifically the Interface Segregation Principle and the Dependency Inversion Principle.
739-776: Good implementation of the concrete transport class.The
GraphQLTransportUrlibimplementation properly extends the abstract base class and provides a robust implementation of thefetchmethod with appropriate error handling for both HTTP errors and URL errors.
894-903: Well-designed static factory method for transport construction.The
Transportsclass with its staticgraphql_syncmethod provides a clean factory pattern for creating transport instances. This matches the PR objective of transitioning transport construction to a static method.
622-623: Proper initialization of optional parameters.The code now properly initializes the
filesparameter to an empty dictionary when it's None, which prevents potential NoneType errors.
680-682: Modern Python type annotations utilized.The code uses the modern pipe (
|) notation for union types introduced in Python 3.10, which makes the type annotations more readable.
30-30: Explicit boolean conversion improves code clarity.Using
bool()to explicitly convert the expression to a boolean value enhances code readability and prevents potential issues with truthy/falsy values.
782-832: Consistent return type updates align with the new architecture.The changes to return types (using
PreparedRequestinstead of specific implementations) across various methods supports the abstraction introduced by the base class, allowing for different transport implementations to be used interchangeably.
516-517: Query node conversion logic is correctly maintained.The logic for converting query nodes to GraphQL is preserved, ensuring compatibility with existing code while enabling the new transport abstraction.
tests/metagen/typegraphs/identities/rs/fdk.rs (6)
88-92: Good addition of Default implementation for MatBuilderThis implementation correctly leverages the existing
new()method, enhancing code ergonomics by allowing the use ofDefault::default()orMatBuilder::default().
137-141: Hostcall transport integration aligns with PR objectivesThe code now constructs a
QueryGraphinstance and initializes a context with the hostcall transport, which aligns with the PR objective of implementingHostcallTransportfor different frameworks.
153-156: Enhanced Ctx struct provides necessary functionalityThe updated
Ctxstruct now includes both theQueryGraphandHostcallTransportinstances, which enables custom functions to access typegraphs as mentioned in the PR objectives.
238-254: Well-structured transports module with good documentationThe new
transportsmodule is well-documented with clear explanations of supported transport types. Thehostcallfunction appropriately creates aHostcallTransportusing theQueryGraph's type mappings, which matches the migration noted in the PR objectives where transport construction moved to thetransportsmodule.
260-263: Appropriate QueryGraph implementationThe
QueryGraphstruct and its implementation provide a centralized approach to mapping Rust types to GraphQL types, which is beneficial for maintaining consistent type mappings across the codebase.Also applies to: 905-921
1187-1206: Consistent trait implementation for RsProxyPrimitivesThe new
RsProxyPrimitivestrait follows the same pattern as existing traits in thestubsmodule, maintaining consistency in the codebase while extending functionality for proxy primitives.tests/metagen/typegraphs/sample/rs/client.rs (5)
7-24: Well-documented transports module with appropriate functionsThe new
transportsmodule is well-documented and provides clear functions for creating different transport clients. This centralizes transport creation logic, aligning with the PR objective of moving transport construction from a method onQueryGraphto functions in thetransportsmodule.
337-390: Good separation of concerns with return_types moduleThe introduction of the
return_typesmodule enhances type safety by clearly separating input and output types. The use of*Partialstructs with optional fields is appropriate for GraphQL responses where not all fields may be requested.
446-457: Concise QueryGraph initializationThe
query_graph()function provides a clean way to initialize aQueryGraphwith the necessary type mappings, making the code more maintainable and easier to understand.
458-626: Updated method signatures maintain type safetyThe updates to method signatures in the
QueryGraphimplementation reflect the architectural changes while maintaining type safety through the use of appropriate generic parameters and return types.
19-22: Good use of conditional compilation for platform-specific functionalityThe
#[cfg(not(target_family = "wasm"))]annotation appropriately limits the availability of the synchronous GraphQL transport to non-WASM environments, which is a good practice for handling platform-specific functionality.tests/metagen/typegraphs/sample/ts/client.ts (3)
645-664: Method visibility changed from private to protectedThe
requestmethod's visibility has been changed from private (#request) to protected. This is a significant API change that allows derived classes to access this method, which aligns with the hostcall transport implementation mentioned in the PR objectives.
794-800: Constructor parameter change improves extensibilityThe constructor parameters for
PreparedRequesthave been updated to accept a function for handling requests rather than direct dependencies on address and options. This change improves extensibility and supports the hostcall transport architecture mentioned in the PR objectives.
875-890: Refactor class with only static membersThe
Transportsclass only contains static methods, which is generally discouraged in modern JavaScript/TypeScript. Consider refactoring to use standalone functions instead.-export class Transports { - /** - * Get the {@link GraphQLTransport} for the typegraph. - */ - static graphql( - qg: _QueryGraphBase, - addr: URL | string, - options?: GraphQlTransportOptions, - ) { - return new GraphQLTransport( - new URL(addr), - options ?? {}, - qg.typeNameMapGql, - ); - } -} +/** + * Get the {@link GraphQLTransport} for the typegraph. + */ +export function createGraphQLTransport( + qg: _QueryGraphBase, + addr: URL | string, + options?: GraphQlTransportOptions, +) { + return new GraphQLTransport( + new URL(addr), + options ?? {}, + qg.typeNameMapGql, + ); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/metagen/typegraphs/sample/ts_upload/client.ts (3)
645-664: Method visibility changed from private to protectedThe
requestmethod's visibility has been changed from private (#request) to protected. This is a significant API change that allows derived classes to access this method, which aligns with the hostcall transport implementation mentioned in the PR objectives.
794-800: Constructor parameter change improves extensibilityThe constructor parameters for
PreparedRequesthave been updated to accept a function for handling requests rather than direct dependencies on address and options. This change matches the PR objectives of implementing hostcall transport by making the request handling more flexible.
875-890: Refactor class with only static membersThe
Transportsclass only contains static methods. Consider refactoring to use standalone functions instead for better maintainability.-export class Transports { - /** - * Get the {@link GraphQLTransport} for the typegraph. - */ - static graphql( - qg: _QueryGraphBase, - addr: URL | string, - options?: GraphQlTransportOptions, - ) { - return new GraphQLTransport( - new URL(addr), - options ?? {}, - qg.typeNameMapGql, - ); - } -} +/** + * Get the {@link GraphQLTransport} for the typegraph. + */ +export function createGraphQLTransport( + qg: _QueryGraphBase, + addr: URL | string, + options?: GraphQlTransportOptions, +) { + return new GraphQLTransport( + new URL(addr), + options ?? {}, + qg.typeNameMapGql, + ); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 875-890: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/metagen/typegraphs/identities/ts/fdk.ts (1)
942-969: Retain the static class design as per user preference.Static analysis suggests avoiding classes that only contain static members. However, based on your learnings:
The
Transportsclass in TypeScript should be maintained as a class with static methods (rather than refactored to top-level functions) to ensure consistency across language implementations and improve API discoverability.Therefore, it’s acceptable to keep the
Transportsclass.🧰 Tools
🪛 Biome (1.9.4)
[error] 942-969: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/metagen/typegraphs/identities/py/fdk.py (1)
1874-1926: Add or expand unit tests for handler functions.The
handler_*functions (e.g.,handler_primitives,handler_composites, etc.) appear central to framework functionality. Ensure each handler is covered by unit or integration tests to confirm correct context usage and GraphQL calls.Would you like me to generate a test outline or stub to verify each handler’s behavior?
src/metagen/src/client_py/static/client.py (2)
624-625: Avoid mutable default arguments forfiles.While you do set
files = {}if it’sNone, double-check all code paths to ensurefilesis never shared across calls. It's correct as implemented, but be aware if future modifications add more complexity around default values.
677-685: Ensure comprehensive error handling infetch.HTTP or transport issues could leave partial data or lead to inconsistent states. Verify that the calling code gracefully handles or retries upon exceptions. Also confirm that upstream callers handle these thrown exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/metagen/src/fdk_py/static/fdk.py (1)
16-30: New Ctx class with conditional initialization based on HOSTCALL flagThe Ctx class provides the core functionality for executing GraphQL queries and properly stores the binding function and optionally the QueryGraph and HostcallTransport objects when HOSTCALL is enabled.
Consider adding docstrings to the class and methods to improve code maintainability:
+"""Context object for GraphQL queries execution through hostcall or standard transports.""" class Ctx: def __init__( self, binding: "HostcallBinding", # metagen-genif HOSTCALL qg: "QueryGraph", host: "HostcallTransport", # metagen-endif ): + """Initialize Ctx with binding function and optional QueryGraph and HostcallTransport.""" self.__binding = binding # metagen-genif HOSTCALL self.qg = qg self.host = host # metagen-endif def gql(self, query: str, variables: typing.Mapping): + """Execute a GraphQL query with the provided variables.""" return self.__binding(query, dict(variables))tests/internal/py/fdk.py (3)
33-210: Implementation of selection_to_nodes functionThe function handles GraphQL selection conversion thoroughly with proper error handling for various edge cases. It's a core part of the GraphQL client implementation.
Consider adding a docstring that explains the purpose of this complex function and its parameters to improve maintainability.
851-880: Implementation of HostcallTransport classThe HostcallTransport class implements the core functionality for the hostcall mechanism, which is a key objective of this PR. It properly extends the GraphQLTransportBase class and handles GraphQL operations through the binding function.
Consider adding a docstring to explain the purpose of this class and how it differs from other transport implementations.
1115-1122: Implementation of handler_remote_sum decoratorThis function implements the new decorator pattern described in the PR objectives, following the
handler_{fn_name}format. It correctly wraps the user function and provides it with the raw input and a Ctx object containing the necessary components for GraphQL communication.Consider adding a docstring to explain the purpose of this decorator and its parameters:
+""" +Decorator for the remote_sum function that provides it with the necessary context. + +Args: + user_fn: The user-defined function that takes RootSumFnInput and Ctx and returns a float. + +Returns: + A wrapper function that initializes the Ctx object and calls the user function. +""" def handler_remote_sum(user_fn: typing.Callable[[RootSumFnInput, Ctx], float]): def wrapper(raw_inp, gql_fn): qg = QueryGraph() host = Transports.hostcall(qg, gql_fn) cx = Ctx(gql_fn, qg, host) return user_fn(raw_inp, cx) return wrapper
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/metagen/src/client_rs/static/client.rs(1 hunks)src/metagen/src/fdk_py/static/fdk.py(1 hunks)tests/internal/internal.py(1 hunks)tests/internal/py/fdk.py(1 hunks)tests/internal/py/logic.py(1 hunks)tests/metagen/typegraphs/identities/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/py/client.py(10 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(12 hunks)tests/metagen/typegraphs/sample/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/rs/client.rs(15 hunks)tests/metagen/typegraphs/sample/rs_upload/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/rs_upload/client.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/metagen/src/client_rs/static/client.rs
- tests/internal/py/logic.py
- tests/metagen/typegraphs/sample/py/client.py
- tests/metagen/typegraphs/sample/rs_upload/client.rs
- tests/metagen/typegraphs/sample/rs/client.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (23)
tests/metagen/typegraphs/sample/rs_upload/Cargo.toml (1)
7-7: Dependency Declaration Update for metagen-clientThe dependency now explicitly enables the
graphqlfeature while retaining the workspace linkage. This aligns with the broader dependency modularization strategy outlined in the PR objectives.tests/metagen/typegraphs/sample/rs/Cargo.toml (1)
7-7: Updated Workspace Dependency for metagen-clientThe declaration for
metagen-clientnow includes thegraphqlfeature, ensuring consistency with the new dependency configuration across the project.tests/metagen/typegraphs/identities/rs/Cargo.toml (3)
1-4: Package Metadata UpdateThe package name has been updated to
identities_fdkand the version bumped to0.5.1-rc.0. This change supports the migration plan and ensures consistency across modules.
6-11: Standardized Dependency DeclarationsThe dependencies now use workspace versions—with
metagen-clientconfigured with thegraphqlfeature—and include workspace-based declarations foranyhow,serde,serde_json, andwit-bindgen. This standardized approach will help maintain consistency across the workspace.
13-18: Lib Configuration VerificationThe
[lib]section, along with the descriptive comments regarding wasm artifact configuration, is clear. Please ensure that these settings indeed match the intended build targets for wasm artifacts.tests/metagen/typegraphs/sample/py_upload/client.py (8)
579-579: Good enhancement to transport architecture.Converting
GraphQLTransportBaseto an abstract base class provides a clear contract for implementations and enforces that all transport classes must implement required methods.
676-683: Properly defining abstract fetch method.Adding the
@abstractmethoddecorator ensures subclasses must implement this method. The type annotation improvement fromtyping.Optionalto|syntax also uses more modern Python type annotation style.
621-624: Fixed mutable default parameter issue.Using
Noneas the default value and initializing an empty dictionary inside the function is the proper pattern for avoiding the mutable default parameter anti-pattern.
740-777: Good implementation of fetch method with error handling.The implementation of the
fetchmethod inGraphQLTransportUrlibis thorough and handles different error scenarios appropriately. The structured exception handling with detailed error information will be helpful for debugging.
783-791: Simplified class hierarchy for better maintainability.Replacing references to
PreparedRequestUrlibwithPreparedRequestand consolidating the implementation streamlines the codebase and makes it easier to maintain.Also applies to: 800-831
895-904: Improved design with static method and better typing.Converting
graphql_syncto a static method and adding theqg: QueryGraphBaseparameter makes the API more explicit and allows for better dependency injection.
952-953: Enhanced type definitions for optional fields.Using
typing.NotRequiredforpathandprefixfields in the TypedDict definitions properly indicates these fields are optional, improving type safety and documentation.Also applies to: 963-964
748-750: Consistent null-check pattern for function parameters.The implementation follows the same pattern established earlier in the code for handling optional parameters, which creates a consistent coding style throughout the codebase.
tests/internal/internal.py (1)
35-35: Dependency path updated to use the new fdk.py fileThis change aligns with the PR's objective of implementing HostcallTransport by updating the dependency from
logic_types.pyto the newfdk.pyfile, which now contains the necessary types and functionality for the remote_sum function.src/metagen/src/fdk_py/static/fdk.py (3)
3-9: New conditional code generation directives for HostcallTransportThe metagen directives properly segregate code that should only be included when the hostcall feature is enabled vs. disabled, which aligns with the PR objective of implementing HostcallTransport in fdk_py.
10-15: HostcallBinding type alias defined for non-HOSTCALL contextsThis type alias properly defines the expected signature for hostcall bindings as a callable that takes a query string and variables dictionary, and returns a result dictionary.
31-32: GraphQL query execution methodThe gql method provides a clean interface for executing GraphQL queries, converting the variables mapping to a dictionary before passing it to the binding function.
tests/internal/py/fdk.py (6)
18-30: Ctx class implementation with hostcall supportThe Ctx class is properly implemented to manage GraphQL contexts and store the binding function, query graph, and hostcall transport.
527-527: Loop variable shadowing the outer variableInside the loop, the variable
nodeis shadowing the outer variable with the same name, which could lead to confusion and bugs.Rename the loop variable to avoid confusion:
- for node in node.sub_nodes: + for child_node in node.sub_nodes: sub_node_list += f"{convert_query_node_gql(ty_to_gql_ty_map, node, variables, files)} "
633-637: Mutable default argument issueUsing a mutable object (
{}) as a default argument is problematic in Python as it's created only once at function definition time and shared between all calls.Replace with None and initialize within the function:
- def build_req( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: typing.Optional[GraphQLTransportOptions] = None, - files: typing.Dict[str, File] = {}, - ): - headers = {} + def build_req( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions] = None, + files: typing.Optional[typing.Dict[str, File]] = None, + ): + if files is None: + files = {} + headers = {}
756-763: Another mutable default argument issueAnother instance of a mutable default argument issue that should be fixed.
Replace with None and initialize within the function:
- def fetch( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, - ): - if files is None: - files = {} + def fetch( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions], + files: typing.Optional[typing.Dict[str, File]] = None, + ): + if files is None: + files = {}
784-789: Exception chaining issue within except blockWhen raising new exceptions in an except block, you should use
from errto maintain the exception context.Modify to maintain the exception context:
except urllib.error.URLError as err: - raise Exception(f"URL error: {err.reason}") + raise Exception(f"URL error: {err.reason}") from err
862-875: Mutable default in HostcallTransport fetch methodAnother instance of a mutable default argument issue.
Replace with None and initialize within the function:
- def fetch( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, - ): - _ = opts - - if files is None: - files = {} + def fetch( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions], + files: typing.Optional[typing.Dict[str, File]] = None, + ): + _ = opts + + if files is None: + files = {}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (5)
tests/metagen/typegraphs/identities/py/fdk.py (5)
534-534: 🛠️ Refactor suggestionFix variable shadowing in loop.
The loop variable
nodeshadows the parent reference with the same name, which could lead to confusion and errors.- for node in node.sub_nodes: + for sub_node in node.sub_nodes: sub_node_list += ( - f"{convert_query_node_gql(ty_to_gql_ty_map, node, variables, files)} " + f"{convert_query_node_gql(ty_to_gql_ty_map, sub_node, variables, files)} " )
633-637: 🛠️ Refactor suggestionAvoid using a mutable default argument.
Providing
files: typing.Dict[str, File] = {}as a default can lead to unexpected behavior due to Python's default argument mutability. UseNoneas the default and initialize the dictionary inside the function.def build_req( self, doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions] = None, - files: typing.Dict[str, File] = {}, + files: typing.Optional[typing.Dict[str, File]] = None, ): + if files is None: + files = {}
756-769: 🛠️ Refactor suggestionFix mutable default parameter in
GraphQLTransportUrlib.fetch.Same issue with using a mutable dictionary as a default parameter.
def fetch( self, doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, + files: typing.Optional[typing.Dict[str, File]] = None, ): + if files is None: + files = {} req = self.build_req(doc, variables, opts, files)
862-875: 🛠️ Refactor suggestionFix mutable default parameter in
HostcallTransport.fetch.Same issue with using a mutable dictionary as a default parameter.
def fetch( self, doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, + files: typing.Optional[typing.Dict[str, File]] = None, ): _ = opts + if files is None: + files = {} if len(files) > 0: raise Exception("no support for file upload on HostcallTransport")
696-696: 🛠️ Refactor suggestionUse
Noneinstead of{}as default parameter in abstract method.The abstract method signature also uses a mutable default parameter which can be a source of bugs if implementations modify the dictionary.
@abstractmethod def fetch( self, doc: str, variables: typing.Dict[str, typing.Any], opts: GraphQLTransportOptions | None, - files: typing.Dict[str, File] = {}, + files: typing.Optional[typing.Dict[str, File]] = None, ) -> typing.Any: ...
🧹 Nitpick comments (9)
tests/metagen/typegraphs/sample/rs/client.rs (3)
298-305:RootMixedUnionFnOutputbroadens support with scalar variants.
EnablingStringandI64variants can handle complex scenarios but check that all variant arms are handled or tested.
543-543:scalar_unionmethod’s argument type updated.
PassingRootCompositeArgsFnInputfor a scalar union might confuse future maintainers. Provide documentation or rename for clarity if needed.
611-612:identity_updatepartial usage.
Same concern as in the other identity-related function. If partial usage remains essential, consider adding clarifying comments.tests/metagen/typegraphs/identities/py/fdk.py (6)
7-16: Remove unnecessarypassstatements.There are two redundant
passstatements in theCtxclass that can be safely removed.class Ctx: def __init__( self, binding: "HostcallBinding", qg: "QueryGraph", host: "HostcallTransport" ): self.gql = binding self.qg = qg self.host = host - pass - pass
844-844: Consider using conditional imports instead of code generation directives.The code uses custom directives (
metagen-genif HOSTCALLandmetagen-endif) to conditionally include code. Consider using standard Python conditional imports instead, which would be more maintainable and follow Python conventions.Consider refactoring to use Python's standard conditional import mechanisms rather than custom directives.
Also applies to: 1010-1010
2881-2932: Consider improving handler function documentation.These handler functions are crucial for the FDK implementation but lack docstrings explaining their purpose, parameters, return values, and usage examples. Adding documentation would help users understand how to use these functions correctly.
Add docstrings to each handler function explaining:
- The purpose of the function
- How it works with the Hostcall mechanism
- Parameter and return value details
- A simple usage example
For example:
def handler_primitives(user_fn: typing.Callable[[PrimitivesArgs, Ctx], Primitives]): """ Wraps a user function to provide GraphQL hostcall capabilities for primitive types. Args: user_fn: A function that takes PrimitivesArgs and Ctx objects and returns Primitives Returns: A wrapper function that initializes the QueryGraph and HostcallTransport Example: @handler_primitives def my_primitive_handler(args, ctx): # Process args and return primitives return result """ def wrapper(raw_inp, gql_fn): # ... existing code ...
14-14: Remove redundant pass statement.This pass statement is unnecessary as there's already code in the method body.
def __init__( self, binding: "HostcallBinding", qg: "QueryGraph", host: "HostcallTransport" ): self.gql = binding self.qg = qg self.host = host - pass
780-786: Add error details in exception handling.The HTTPError case properly preserves the response details, but the URLError case only includes the reason in the error message without any additional context that could help with debugging.
except request.HTTPError as res: return self.handle_response( GraphQLResponse( req, status=res.status or 599, body=res.read(), headers={key: val for key, val in res.headers.items()}, ) ) except urllib.error.URLError as err: - raise Exception(f"URL error: {err.reason}") + raise Exception(f"URL error connecting to {req.addr}: {err.reason}", err)Also applies to: 787-788
15-16: Remove unnecessary pass statement at class level.The class already has a method defined, so the pass statement is redundant.
self.gql = binding self.qg = qg self.host = host - pass
🛑 Comments failed to post (3)
tests/metagen/typegraphs/sample/rs/client.rs (1)
499-499: 🛠️ Refactor suggestion
Check parameter type for
scalar_argsmethod.
Passingimpl Into<NodeArgs<Post>>for a function namedscalar_argsseems inconsistent. If you intend to passPost-style data, consider renaming or refactoring for clarity.tests/metagen/typegraphs/identities/py/fdk.py (2)
1042-1219:
⚠️ Potential issueRemove duplicated
selection_to_nodesfunction.The function
selection_to_nodesappears twice in the code (first at lines 33-211 and then again here). This duplication should be removed to maintain code clarity and prevent maintenance issues.Remove the second instance of this function as it's a direct duplication.
1028-1039: 🛠️ Refactor suggestion
Remove duplicated imports.
These imports are already defined earlier in the file (lines 19-30). Duplicate imports can cause confusion and potentially mask issues.
Remove these duplicate imports as they're already defined at the beginning of the file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/metagen/typegraphs/sample/py_upload/client.py (1)
847-863: 🛠️ Refactor suggestionFix mutable default argument and add proper error handling.
The implementation correctly rejects file uploads and uses the function binding, but has a mutable default argument issue like the previous fetch method.
- def fetch( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, - ): + def fetch( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions], + files: typing.Optional[typing.Dict[str, File]] = None, + ): + if files is None: + files = {}
🧹 Nitpick comments (3)
tests/metagen/typegraphs/identities/py/fdk.py (1)
33-211: Consider splittingselection_to_nodesfor better maintainability.This function is quite large and covers multiple logical concerns (handling flags, aliasing, arguments, unions, etc.). Splitting some of the innermost logic into helper functions can reduce complexity, improve readability, and facilitate testing.
tests/internal/py/fdk.py (1)
862-873: Check for partial file upload support or provide a clearer message.While the code raises an exception if any files are passed to
HostcallTransport, it may be beneficial to clarify if and when file upload support might be implemented. This could help future maintainers understand the limitation and possibly extend the code.tests/metagen/typegraphs/sample/py_upload/client.py (1)
674-681: Type annotation inconsistency in abstract method.Line 679 uses the Python 3.10+ pipe operator (
|) for union types while the rest of the file usestyping.Optional. Consider usingtyping.Optional[GraphQLTransportOptions]for consistency.- opts: GraphQLTransportOptions | None, + opts: typing.Optional[GraphQLTransportOptions],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/metagen/src/client_py/static/client.py(1 hunks)tests/metagen/typegraphs/sample/py/client.py(1 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(1 hunks)src/metagen/src/client_py/static/client.py(8 hunks)src/metagen/src/client_py/types.rs(1 hunks)tests/internal/py/fdk.py(1 hunks)tests/metagen/typegraphs/identities/py/fdk.py(1 hunks)tests/metagen/typegraphs/sample/py/client.py(7 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(10 hunks)examples/typegraphs/metagen/py/fdk.py(1 hunks)src/metagen/src/client_py/static/client.py(1 hunks)tests/internal/py/fdk.py(1 hunks)tests/metagen/typegraphs/identities/py/fdk.py(1 hunks)tests/metagen/typegraphs/sample/py/client.py(0 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(0 hunks)tests/runtimes/python/py_fail/dep_fail.py(1 hunks)tests/runtimes/python/py_fail/hello_fail.py(1 hunks)src/metagen/src/client_py/static/client.py(8 hunks)src/metagen/src/client_py/static/client.py(1 hunks)tests/metagen/typegraphs/identities/py/fdk.py(8 hunks)tests/metagen/typegraphs/sample/py/client.py(7 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(7 hunks)tests/internal/py/fdk.py(8 hunks)tests/metagen/typegraphs/sample/py/client.py(1 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(1 hunks)tests/metagen/typegraphs/identities/py/fdk.py(1 hunks)examples/typegraphs/metagen/py/fdk.py(9 hunks)src/metagen/src/client_py/static/client.py(1 hunks)src/metagen/src/client_py/types.rs(1 hunks)tests/internal/py/fdk.py(2 hunks)tests/metagen/typegraphs/identities/py/fdk.py(5 hunks)tests/metagen/typegraphs/sample/py/client.py(6 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(3 hunks)tests/runtimes/python/py_fail/dep_fail.py(1 hunks)tests/runtimes/python/py_fail/hello_fail.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/metagen/src/client_py/static/client.py
- src/metagen/src/client_py/static/client.py
- tests/metagen/typegraphs/sample/py_upload/client.py
- tests/metagen/typegraphs/sample/py/client.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/runtimes/python/py_fail/dep_fail.py
- tests/runtimes/python/py_fail/dep_fail.py
- tests/runtimes/python/py_fail/hello_fail.py
- src/metagen/src/client_py/types.rs
- src/metagen/src/client_py/types.rs
- tests/runtimes/python/py_fail/hello_fail.py
- src/metagen/src/client_py/static/client.py
- tests/internal/py/fdk.py
- tests/metagen/typegraphs/identities/py/fdk.py
- examples/typegraphs/metagen/py/fdk.py
- tests/metagen/typegraphs/sample/py/client.py
- tests/metagen/typegraphs/identities/py/fdk.py
- examples/typegraphs/metagen/py/fdk.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (13)
tests/metagen/typegraphs/identities/py/fdk.py (1)
635-635: Avoid using a mutable default dictionary forfiles.Using
{}as a default value can lead to shared state across calls. This practice is discouraged in Python. This issue has been noted previously.A safe fix:
- def build_req(self, doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions] = None, files: typing.Dict[str, File] = {}): + def build_req(self, doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions] = None, files: typing.Optional[typing.Dict[str, File]] = None): + if files is None: + files = {}tests/internal/py/fdk.py (1)
630-637: Avoid using a mutable default forfiles.Using a dictionary (
={}) as the default value can cause shared state between function calls. UseNoneand initialize within the function body to prevent unintended behavior.-def build_req(..., files: typing.Dict[str, File] = {}): +def build_req(..., files: typing.Optional[typing.Dict[str, File]] = None): + if files is None: + files = {}src/metagen/src/client_py/static/client.py (3)
31-31: Ensuresub_flagsis properly validated.You replaced the assignment of
select_allwith a concise boolean expression, which is good. However, confirm that any unexpected data type insub_flagsis caught earlier, or the function might silently fail ifNoneor an unexpected value goes unnoticed.
617-624: Initializefilescorrectly before usage.Like in the other files, relying on
files: typing.Dict[str, File] = {}can produce subtle bugs. Switching to aNonedefault ensures each call gets a fresh dictionary.-def build_req(doc: str, variables: typing.Dict[str, typing.Any], opts: typing.Optional[GraphQLTransportOptions] = None, files: typing.Optional[typing.Dict[str, File]] = None): - if files is None: - files = {} +def build_req( + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions] = None, + files: typing.Optional[typing.Dict[str, File]] = None, +): + if files is None: + files = {}
742-750: Good handling of optionalfilesparameterYour approach of setting
filesto an empty dictionary whenNoneis provided prevents null reference errors if the user doesn’t supply files. This aligns well with the logic inbuild_reqand helps maintain consistency.tests/metagen/typegraphs/sample/py_upload/client.py (8)
15-15: Good addition of ABC import.Adding the
ABCandabstractmethodimports enables proper abstract class definition, which helps enforce implementation of required methods in subclasses.
579-591: Well-structured abstract base class implementation.Converting
GraphQLTransportBaseto inherit fromABCprovides better enforcement of the interface contract. The initializer properly stores the configuration parameters needed by all transport implementations.
738-745: Use a non-mutable default for function parameters.Defining
files: typing.Dict[str, File] = {}uses a mutable default, which can cause unexpected behavior when multiple calls modify the same dictionary object.Apply this diff:
- def fetch( - self, - doc: str, - variables: typing.Dict[str, typing.Any], - opts: typing.Optional[GraphQLTransportOptions], - files: typing.Dict[str, File] = {}, - ): + def fetch( + self, + doc: str, + variables: typing.Dict[str, typing.Any], + opts: typing.Optional[GraphQLTransportOptions], + files: typing.Optional[typing.Dict[str, File]] = None, + ): + if files is None: + files = {}
829-834: Good interface definition for hostcall binding.The
HostcallBindingtype alias clearly defines the expected signature for callback functions, making it easier for developers to implement compatible functions.
836-846: Well-designed HostcallTransport implementation.The initialization is simple and focused, storing only the necessary components for this transport type. Good job capturing the function binding as the primary communication mechanism.
922-972: Well-structured PreparedRequest implementation.The
PreparedRequestclass has been updated to work with the abstract base class and supports both types of transports. Theperformmethod efficiently delegates to the transport's fetch method with proper variable resolution.
985-993: Good implementation of the factory pattern in Transports class.The static methods provide a clean API for creating transport instances. Including the
qgparameter allows the transport to access the type mapping information which is essential for GraphQL operations.
997-1005: Good addition of hostcall transport factory method.Adding a static method for creating HostcallTransport instances provides a consistent API for transport creation. This method properly handles the configuration details so that consumers don't need to worry about them.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #982 +/- ##
==========================================
- Coverage 81.14% 81.12% -0.02%
==========================================
Files 142 143 +1
Lines 17970 18044 +74
Branches 1961 1965 +4
==========================================
+ Hits 14581 14639 +58
- Misses 3371 3387 +16
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@michael-0acf4 hey, what are you referring to by custom protocol? The TS types for the worker manager? |
The |
|
@michael-0acf4 that was just a low effort approach to tag which runtime the hostcall is coming from. Since this is going directly into the http handler, we don't really have a specialized path for hostcalls. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/typegate/src/runtimes/wasm_wire.ts (1)
71-71: Updated URL scheme to include hostcall transport mechanismThe URL scheme now includes
hostcall+to identify this as using the new hostcall transport mechanism. This change aligns with the broader PR objective of implementingHostcallTransportacross different frameworks.Consider adding a brief comment explaining the purpose of this URL format, especially since there was discussion in the PR about this naming convention:
const hostcallCtx = { authToken: token, typegate, + // Uses internal+hostcall+witwire scheme to identify runtime origin for hostcall transport typegraphUrl: new URL(`internal+hostcall+witwire://typegate/${typegraphName}`), };src/metagen/src/utils.rs (4)
4-5: AddBTreeMapto the import list.The
BTreeMapis used in the function signature at line 58, but it's not explicitly imported. It would be clearer to import it directly rather than relying on the wildcard import from interlude.use crate::interlude::*; +use std::collections::BTreeMap;
118-166: Consider returning a newline-terminated string to maintain input format.The function currently joins the output lines with
\nwhich could change the file format if the input used\r\nline endings. Consider preserving the input's line ending style.- output_lines.join("\n") + // Preserve original line endings + if input.contains("\r\n") { + output_lines.join("\r\n") + } else { + output_lines.join("\n") + }
134-143: Simplify the initial matches_set creation.The conversion of a single element array to a HashSet can be simplified using HashSet::from methods.
- let mut matches_set: HashSet<usize> = [first].into_iter().collect(); + let mut matches_set = HashSet::from([first]);
150-161: Simplify post-match filtering logic.The filtering logic for non-matching lines after the first match can be simplified for better readability.
- let post_non_matches = lines[first + 1..] - .iter() - .enumerate() - .filter_map(|(rel_i, line)| { - let original_i = first + 1 + rel_i; - if matches_set.contains(&original_i) { - None - } else { - Some(*line) - } - }); + let post_non_matches = lines[first + 1..].iter().enumerate() + .filter_map(|(rel_i, &line)| { + let original_i = first + 1 + rel_i; + (!matches_set.contains(&original_i)).then_some(line) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/metagen/src/utils.rs(2 hunks)src/typegate/src/runtimes/deno/deno.ts(7 hunks)src/typegate/src/runtimes/wasm_wire.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/typegate/src/runtimes/deno/deno.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (4)
src/metagen/src/utils.rs (4)
31-54: Well-documented function with clear examples.The documentation for
processed_writeis comprehensive, explaining the purpose, supported directives, parameters, return values, and including a usage example.
55-113: Theprocessed_writeimplementation is solid.The implementation properly handles conditional code generation with a stack-based approach for nested directives and appropriate error handling for unmatched or unclosed directives.
60-66: Good use ofonce_cell::sync::Lazyfor regex compilation.Compiling regex patterns once and reusing them is an efficient approach that avoids repeated compilation overhead.
115-118: Improve documentation for thecollect_at_first_instancefunction.The documentation comment is brief and doesn't fully explain the function's behavior, parameters, return values, or provide examples.
-/// Moves lines in the input string that match a pattern to come subsequent -/// to the first instance of a line that matches. Order will be preserved. -/// Useful to collect import statements to top of file. +/// Collects lines from input based on a regex pattern. +/// +/// This function: +/// 1. Includes all lines before the first match of the pattern +/// 2. Includes the first match and all subsequent matches +/// 3. Excludes non-matching lines that appear after the first match +/// +/// # Arguments +/// * `input` - The input text to process +/// * `pattern` - The regex pattern to match lines against +/// +/// # Returns +/// A string containing the collected lines +/// +/// # Examples +/// ``` +/// let input = "line1\nfoo\nbar\nfoo\nline5"; +/// let pattern = regex::Regex::new(r"foo").unwrap(); +/// let result = collect_at_first_instance(input, &pattern); +/// // result will be "line1\nfoo\nfoo" +/// ``` pub fn collect_at_first_instance(input: &str, pattern: ®ex::Regex) -> String {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/metagen/src/utils.rs (3)
74-77: Consider using a more explicit condition check.The current implementation uses XOR to toggle the condition based on the presence of "not", which works correctly but could be more readable. Consider an explicit if-else condition for better clarity.
- let condition = flags.get(flag).copied().unwrap_or(false); - let condition = condition ^ not.is_some(); + let flag_value = flags.get(flag).copied().unwrap_or(false); + let condition = if not.is_some() { !flag_value } else { flag_value };
126-126: Use more descriptive HashSet initialization.Using
[first].into_iter().collect()is not immediately intuitive. Consider using a more explicit initialization for better readability.- let mut matches_set: HashSet<usize> = [first].into_iter().collect(); + let mut matches_set: HashSet<usize> = HashSet::from([first]);
157-157: Consider adding a newline preservation option.The current implementation joins lines with
\n, which might lose the original line endings (e.g., CRLF would be converted to LF). Consider an option to preserve the original line endings or explicitly document this behavior.- output_lines.join("\n") + // Preserves original newlines if important + if let Some(original_ending) = input.lines().next().and_then(|_| input.find('\n')) { + let newline = if original_ending > 0 && input.as_bytes()[original_ending - 1] == b'\r' { "\r\n" } else { "\n" }; + output_lines.join(newline) + } else { + output_lines.join("\n") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/metagen/src/utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (2)
src/metagen/src/utils.rs (2)
107-110: Enhance documentation for the collect_at_first_instance function.While the function has a basic description, it would benefit from more comprehensive documentation similar to the
processed_writefunction, including details about parameters, return values, and usage examples./// Moves lines in the input string that match a pattern to come subsequent /// to the first instance of a line that matches. Order will be preserved. /// Useful to collect import statements to top of file. +/// +/// # Arguments +/// * `input` - The input text to process +/// * `pattern` - The regex pattern to match lines against +/// +/// # Returns +/// A string containing the rearranged lines, with all pattern matches coming after the first match +/// +/// # Examples +/// ``` +/// let input = "line1\nimport foo\nline3\nimport bar\nline5"; +/// let pattern = regex::Regex::new(r"import").unwrap(); +/// let result = collect_at_first_instance(input, &pattern); +/// // result will be "line1\nimport foo\nimport bar\nline3\nline5" +/// ``` pub fn collect_at_first_instance(input: &str, pattern: ®ex::Regex) -> String {
31-105: Well-implemented conditional code generation function.The
processed_writefunction is well designed with comprehensive documentation, proper error handling for unmatched or unclosed directives, and efficient use of lazy-initialized regex patterns.
QueryGraphmethod clashMigration notes
fdk_rs: transport construction is no longer a method onQueryGraphbut a set of functions in thtransportsmodule.fdk_ts: transport construction is no longer a method onQueryGraphbut instead a static method onTransportsclass.fdk_pynow generates all code intofdk.pyhandler_{fn_name}patternCtxobject.Ctx.gqlmethod now takestyping.Mappinginstead of raw JSONstrQueryGraphbut instead a static method onTransportsclass.Summary by CodeRabbit
Summary by CodeRabbit
New Features
HostcallPumpclass for managing host calls and responses, improving the handling of asynchronous operations.QueryGraphclass.Cargo.tomlconfigurations to include workspace features formetagen-client, allowing for specific feature activation.typegraphUrlformat in thehostcallCtxfor improved communication protocols.Refactors & Chores
Tests & Documentation