Rust 🦀 #1275
Rust 🦀 #1275
Conversation
WalkthroughAdds full Rust SDK support: a new PHP class Appwrite\SDK\Language\Rust with type mappings, filters, and codegen helpers; introduces ~30 Rust Twig templates to scaffold a crate (Cargo.toml, README, CHANGELOG, LICENSE, .gitignore, examples, and extensive src templates for client, services, models, enums, error, input_file, id, operator, permission, query, role, tests); GitHub Actions workflows and CI matrices updated to include Rust build/test/publish steps; example generator, test runner scripts, and PHP test harness updated to generate and run the Rust SDK. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
update: djlint config for rust, readme.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (15)
templates/rust/.github/workflows/publish.yml.twig (1)
16-17: Consider consistency in Rust toolchain versions.This workflow uses
@stablewhile the validation workflow uses@1.83.0. This inconsistency could lead to publishing with a different Rust version than what was tested in CI.Consider either:
- Using the same pinned version (
@1.83.0) for consistency, or- Documenting why stable is preferred for publishing
🔎 Proposed fix to align with CI validation version
- name: Setup Rust - uses: dtolnay/rust-toolchain@stable + uses: dtolnay/[email protected]example.php (1)
270-276: Consider setting Rust-specific configuration.The Rust SDK generation uses only default configuration. Based on the
Rust.phplanguage class, there's asetCratePackage()method available. Consider whether a specific crate package name should be set, similar to how other SDKs configure their package names (e.g., Dart usessetPackageName('dart_appwrite')).🔎 Example configuration with crate package name
// Rust if (!$requestedSdk || $requestedSdk === 'rust') { - $sdk = new SDK(new Rust(), new Swagger2($spec)); + $rust = new Rust(); + $rust->setCratePackage('appwrite'); + $sdk = new SDK($rust, new Swagger2($spec)); configureSDK($sdk); $sdk->generate(__DIR__ . '/examples/rust'); }templates/rust/tests/tests.rs (2)
102-105: Clarify duplicate test calls.Lines 102-103 and 104-105 each call the same test functions twice (
test_general_uploadandtest_large_upload). If this is intentional to test idempotency or retry behavior, consider adding a comment. Otherwise, these might be unintentional duplicates.test_general_upload(client, string_in_array).await?; - test_general_upload(client, string_in_array).await?; test_large_upload(client, string_in_array).await?; - test_large_upload(client, string_in_array).await?;
206-269: Consider adding assertions for query tests.The query tests only print outputs but don't verify correctness. While this might be sufficient for integration testing against a mock API that validates the output format, consider whether some basic assertions would improve test reliability.
templates/rust/CHANGELOG.md.twig (1)
44-50: Ensure dependency versions match Cargo.toml.The dependency versions listed in the changelog should match those in
Cargo.toml.twig. When updating dependency versions in Cargo.toml, remember to update this changelog template as well.templates/rust/src/id.rs.twig (1)
16-18: Consider graceful error handling instead of panic.The
.expect("Time went backwards")will panic if system time moves backwards (e.g., NTP correction, VM snapshot restore). For a production SDK, consider returning aResultor using a fallback strategy.🔎 Proposed alternatives
Option 1: Return Result from the function
- pub fn unique_with_padding(padding: usize) -> String { + pub fn unique_with_padding(padding: usize) -> Result<String, std::time::SystemTimeError> { let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("Time went backwards"); + .duration_since(UNIX_EPOCH)?;Option 2: Use a fallback to maintain the current API
let now = SystemTime::now() .duration_since(UNIX_EPOCH) - .expect("Time went backwards"); + .unwrap_or_else(|_| std::time::Duration::from_secs(0));templates/rust/src/permission.rs.twig (1)
15-38: Update misleading documentation comments.The doc comments say "for any user" but all methods accept a
roleparameter that determines the specific role/user. The comments should reflect that these methods create permissions for the specified role.🔎 Proposed documentation fix
- /// Read permission for any user + /// Read permission for the specified role pub fn read(role: impl std::fmt::Display) -> Self { Self::new(format!("read(\"{}\")", role)) } - /// Write permission for any user + /// Write permission for the specified role pub fn write(role: impl std::fmt::Display) -> Self { Self::new(format!("write(\"{}\")", role)) } - /// Create permission for any user + /// Create permission for the specified role pub fn create(role: impl std::fmt::Display) -> Self { Self::new(format!("create(\"{}\")", role)) } - /// Update permission for any user + /// Update permission for the specified role pub fn update(role: impl std::fmt::Display) -> Self { Self::new(format!("update(\"{}\")", role)) } - /// Delete permission for any user + /// Delete permission for the specified role pub fn delete(role: impl std::fmt::Display) -> Self { Self::new(format!("delete(\"{}\")", role)) }templates/rust/src/models/model.rs.twig (1)
51-77: Tests rely onDefaulttrait that may not be meaningful for all models.The test module assumes all field types implement
Default. For models with required fields of custom types (e.g., nested models),Default::default()may produce invalid/empty data that could cause issues during serialization roundtrips if the server rejects empty required fields.Consider adding a comment noting this limitation, or generating more meaningful test data for required fields.
templates/rust/src/input_file.rs.twig (1)
163-168:is_empty()returnsfalsefor path-based files without checking actual size.For
InputFileSource::Path, returningfalseunconditionally could be misleading if the file is actually empty (0 bytes). Consider usingsize_sync()or documenting this behavior.🔎 Suggested improvement
pub fn is_empty(&self) -> bool { match &self.source { InputFileSource::Bytes { data } => data.is_empty(), - InputFileSource::Path { .. } => false, + InputFileSource::Path { path } => { + std::fs::metadata(path).map(|m| m.len() == 0).unwrap_or(true) + } } }templates/rust/src/client.rs.twig (3)
124-136: Consider returningResultinstead of panicking on invalid endpoint.Panicking in
set_endpointfor invalid URLs could cause unexpected crashes in user code. AResultreturn type or a non-builder method that validates would be more robust.🔎 Alternative approach
- pub fn set_endpoint<S: Into<String>>(&self, endpoint: S) -> Self { + pub fn set_endpoint<S: Into<String>>(&self, endpoint: S) -> Result<Self, crate::error::{{ spec.title | caseUcfirst }}Error> { let endpoint = endpoint.into(); if !endpoint.starts_with("http://") && !endpoint.starts_with("https://") { - panic!("Invalid endpoint URL: {}. Endpoint must start with http:// or https://", endpoint); + return Err(crate::error::{{ spec.title | caseUcfirst }}Error::new( + 0, + format!("Invalid endpoint URL: {}. Endpoint must start with http:// or https://", endpoint), + None, + String::new(), + )); } // ... rest of method - self.clone() + Ok(self.clone()) }
216-234: Silent failure inadd_headercould hide configuration errors.When header name or value parsing fails, the method silently proceeds without adding the header. Consider logging a warning or returning a
Resultto help users debug configuration issues.
375-479:call_locationcreates a new HTTP client per invocation.Lines 387-392 build a new
reqwest::Clientfor eachcall_locationcall. While this is needed for the no-redirect policy, client creation has overhead. Consider caching a no-redirect client inClientStateif this method is called frequently.src/SDK/Language/Rust.php (1)
539-552: Unused parameters flagged by static analysis are likely intentional for interface consistency.The
$spec,$namespace,$genericparameters ingetPropertyType,getReturnType, andhasGenericTypeare currently unused but may be required by the parent class interface or reserved for future use. If intentional, consider adding@SuppressWarningsor a brief comment.Also applies to: 643-667, 674-695
templates/rust/src/query.rs.twig (2)
251-257: Unnecessary clone inDisplayimplementation.
to_value(self)takes ownership, butDisplay::fmtonly has&self. The current approach clones the entire Query. Consider derivingSerializeforQuerydirectly or implementing a method that serializes from&self.🔎 Alternative: derive Serialize directly
#[derive(Debug, Clone, serde::Serialize)] #[serde(rename_all = "camelCase")] pub struct Query { method: String, #[serde(skip_serializing_if = "Option::is_none")] attribute: Option<String>, #[serde(skip_serializing_if = "Vec::is_empty")] values: Vec<Value>, } impl std::fmt::Display for Query { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let json_str = serde_json::to_string(self) .unwrap_or_else(|_| String::new()); write!(f, "{}", json_str) } }
210-232:orandandmethods require pre-serialized JSON strings.The API requires users to serialize Query objects to strings first, then pass them to
or/and. This is somewhat awkward. Consider also acceptingVec<Query>directly.🔎 Additional overload suggestion
/// Combine queries with OR (accepts Query objects directly) pub fn or_queries(queries: impl IntoIterator<Item = Query>) -> Self { let values: Vec<Value> = queries.into_iter().map(|q| q.to_value()).collect(); Self::new("or".to_string(), None, values) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.github/workflows/sdk-build-validation.yml.github/workflows/tests.ymlexample.phppyproject.tomlsrc/SDK/Language/Rust.phptemplates/rust/.github/workflows/autoclose.ymltemplates/rust/.github/workflows/publish.yml.twigtemplates/rust/.github/workflows/stale.ymltemplates/rust/.gitignoretemplates/rust/CHANGELOG.md.twigtemplates/rust/Cargo.toml.twigtemplates/rust/LICENSE.twigtemplates/rust/README.md.twigtemplates/rust/examples/basic_usage.rs.twigtemplates/rust/src/client.rs.twigtemplates/rust/src/enums/enum.rs.twigtemplates/rust/src/enums/mod.rs.twigtemplates/rust/src/error.rs.twigtemplates/rust/src/id.rs.twigtemplates/rust/src/input_file.rs.twigtemplates/rust/src/lib.rs.twigtemplates/rust/src/models/mod.rs.twigtemplates/rust/src/models/model.rs.twigtemplates/rust/src/models/request_model.rs.twigtemplates/rust/src/permission.rs.twigtemplates/rust/src/query.rs.twigtemplates/rust/src/role.rs.twigtemplates/rust/src/services/mod.rs.twigtemplates/rust/src/services/service.rs.twigtemplates/rust/tests/tests.rstests/Rust183Test.phptests/languages/rust/test.sh
🧰 Additional context used
🧬 Code graph analysis (1)
example.php (3)
src/SDK/Language.php (1)
Language(5-295)src/SDK/Language/Rust.php (1)
Rust(8-696)src/Spec/Swagger2.php (1)
Swagger2(7-646)
🪛 PHPMD (2.15.0)
src/SDK/Language/Rust.php
274-274: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
408-408: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
539-539: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
645-645: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
646-646: Avoid unused parameters such as '$namespace'. (undefined)
(UnusedFormalParameter)
647-647: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
🪛 Shellcheck (0.11.0)
tests/languages/rust/test.sh
[warning] 4-4: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (50)
templates/rust/LICENSE.twig (1)
1-1: LGTM!The template correctly uses the
rawfilter to output unescaped license content.templates/rust/.gitignore (1)
1-24: LGTM!The .gitignore patterns are comprehensive and follow Rust ecosystem conventions. The exclusion of Cargo.lock is appropriate for library SDKs.
templates/rust/README.md.twig (1)
1-45: LGTM!The README template is well-structured with appropriate badge URLs, installation instructions, and conditional rendering for optional sections. The use of default values for
cratePackageensures robustness..github/workflows/tests.yml (1)
46-46: LGTM!The addition of Rust183 to the SDK matrix is consistent with the PR's objective to add Rust SDK support.
templates/rust/.github/workflows/stale.yml (1)
1-9: LGTM!The workflow correctly delegates to a reusable stale workflow with an appropriate daily schedule.
pyproject.toml (1)
6-9: LGTM!The additions to the djlint ignore list are well-justified with clear comments explaining the Rust-specific false positives (generics and references).
templates/rust/.github/workflows/autoclose.yml (1)
1-12: LGTM!The workflow correctly uses
pull_request_targetto access secrets and delegates to a trusted reusable workflow. The condition appropriately allows PRs from thedevbranch..github/workflows/sdk-build-validation.yml (3)
61-62: LGTM! Rust SDK added to the matrix.The addition of Rust as a server-side SDK is correctly configured and follows the same pattern as other SDKs in the matrix.
203-206: LGTM! Rust build commands are appropriate.The build and test commands follow Rust best practices:
cargo build --releaseensures the code compiles in release modecargo test --libruns library tests
146-148: Rust 1.83.0 version is correctly specified and aligns with project requirements.Verification confirms that Rust 1.83.0 is a stable version released on November 28, 2024, and matches the
rust-version = "1.83"specification inCargo.toml. The workflow's pinning to this specific patch version is appropriate for deterministic builds.templates/rust/.github/workflows/publish.yml.twig (2)
19-20: LGTM! Tests run before publishing.Running tests before publishing is a good practice to ensure code quality.
22-23: Verify CARGO_REGISTRY_TOKEN secret is configured.The workflow requires the
CARGO_REGISTRY_TOKENsecret to publish to crates.io. Ensure this secret is properly configured in the repository settings before the first release.example.php (2)
24-24: LGTM! Rust import added.The import follows the same pattern as other language imports.
281-283: LGTM! Enhanced error handling with Throwable.Catching
Throwablein addition toExceptionprovides better error coverage for PHP 7+ (catches both exceptions and errors). This is a good defensive programming practice.tests/Rust183Test.php (3)
7-10: LGTM! Test configuration is appropriate.The test class configuration correctly sets up the Rust SDK test parameters with appropriate naming and platform settings.
15-15: Ensure Docker image version consistency.The test uses
rust:1.83Docker image, which should align with:
- The rust-version in
Cargo.toml.twig(1.83)- The toolchain in CI workflows (1.83.0)
Verify these versions are consistent across the codebase.
14-15: The paths referenced are correct. Thetests/sdks/rust/directory is generated at test runtime by the SDK generation code inBase.php::testHTTPSuccess(), not pre-existing in the repository. Thetests/languages/rust/test.shscript exists and is properly referenced. This pattern is consistent across all language tests in the codebase.Likely an incorrect or invalid review comment.
templates/rust/Cargo.toml.twig (2)
1-18: LGTM! Package metadata is well-configured.The package metadata section correctly uses template variables and follows Rust packaging best practices. The rust-version of 1.83 aligns with the CI toolchain version.
39-44: LGTM! Conditional test binary configuration.The conditional inclusion of the test binary when
sdk.test == "true"is a good approach that keeps the published crate clean while supporting testing during development.templates/rust/tests/tests.rs (1)
6-22: LGTM! Main test orchestrator is well-structured.The async main function properly orchestrates all test functions in sequence, providing clear output and proper error propagation.
templates/rust/CHANGELOG.md.twig (1)
1-23: LGTM! Changelog follows best practices.The changelog template follows the Keep a Changelog format and provides comprehensive documentation of the initial release, including all features and capabilities.
templates/rust/examples/basic_usage.rs.twig (4)
10-20: LGTM! Client initialization example is clear and well-documented.The example demonstrates proper client initialization with appropriate placeholder values and clear comments guiding users to replace them.
24-41: LGTM! User creation example demonstrates proper error handling.The example shows both success and error cases with informative output, teaching users how to handle the SDK's error types correctly.
59-88: LGTM! Conditional service examples are well-structured.The conditional inclusion of database service examples based on
spec.servicesensures the example remains relevant for different API specifications.
127-143: Excellent error handling demonstration.The example explicitly demonstrates error handling with invalid credentials, showing users what to expect when API calls fail. This is valuable for learning the SDK's error patterns.
templates/rust/src/enums/mod.rs.twig (1)
1-6: LGTM!The module aggregator correctly generates public module declarations and re-exports for all enums. The pattern follows Rust conventions.
templates/rust/src/id.rs.twig (2)
46-118: LGTM!The test suite is comprehensive and covers length validation, uniqueness, hex format, custom IDs, padding behavior, and edge cases.
33-33: fastrand dependency is properly declared in Cargo.toml.The
fastrand = "2.0"dependency is already included in the Cargo.toml template dependencies section, so no action is needed.templates/rust/src/permission.rs.twig (1)
58-91: LGTM!Test coverage is solid and validates the permission string formatting for all builder methods.
templates/rust/src/lib.rs.twig (1)
1-62: LGTM!The library facade follows Rust conventions with:
- Clear module organization
- Idiomatic re-exports of commonly used types
- Type alias for SDK Result type
- SDK metadata as public constants
- Comprehensive documentation with usage example
templates/rust/src/services/mod.rs.twig (2)
24-33: LGTM!The initialization order is correct: services are instantiated with references to
client(lines 28-30) beforeclientis moved into the struct (line 31). This ensures each service has access to the client configuration during construction.
10-14: LGTM!The Service trait provides a clean abstraction for accessing the underlying client from any service implementation.
templates/rust/src/models/request_model.rs.twig (3)
1-23: LGTM!The request model struct follows Rust best practices:
- Proper serde attributes for JSON serialization
skip_serializing_iffor optional fields produces clean JSON- Field name transformation handles various casing conventions
- Conditional
Defaultderive for test convenience
25-49: LGTM!The accessor pattern is idiomatic:
- Fluent setters for optional fields (builder pattern)
- Reference getters for both required and optional fields
- Consistent naming convention
51-77: LGTM!Tests validate struct creation, accessor methods, and JSON serialization round-trip.
templates/rust/src/role.rs.twig (3)
1-66: LGTM!The Role builder implementation is excellent:
- Private constructor ensures controlled construction
- Generic
Into<String>parameters provide flexibility- Optional parameters handled idiomatically with
Option<&str>- Consistent string formatting for all role types
68-90: LGTM!The trait implementations provide ergonomic conversions:
Displayfor string renderingFrom<Role>for owned string extractionFrom<&str>andFrom<String>for flexible role creation
92-155: LGTM!Comprehensive test coverage validates all constructors, optional parameters, and conversions.
templates/rust/src/enums/enum.rs.twig (2)
4-4: Note: Copy derive restricts enum variants to simple types.The
Copyderive on Line 4 means all enum variants must beCopy, which prevents adding fields to variants later. This is appropriate for simple value enums (like status codes or flags) but would need to be removed if variants require associated data.Given this template generates API enums from specifications, the restriction is likely intentional and acceptable.
14-30: LGTM!The
as_str()method andDisplayimplementation provide idiomatic string conversion for enum variants, with proper serde attribute mapping for serialization.templates/rust/src/models/mod.rs.twig (1)
1-24: LGTM!The module structure is well-organized with proper re-exports for convenient access. The
Modeltrait withSerialize + Deserialize + Clone + Debugbounds is appropriate for SDK data models, and the blanket implementations for all generated types ensure consistency.templates/rust/src/models/model.rs.twig (1)
1-49: LGTM on the model generation template.The struct definition with serde attributes, optional field handling with
skip_serializing_if, and builder-style setters for optional fields follow idiomatic Rust patterns. The getter methods returning references are appropriate for the use case.templates/rust/src/services/service.rs.twig (2)
26-34: LGTM on service construction.The service pattern with
new(client: &Client)that clones the client andclient()accessor is clean and follows the SDK's arc-swap architecture for mutable client state.
35-141: Well-structured method generation with proper handling of various parameter types.The template correctly:
- Orders required parameters before optional ones (Lines 52-66)
- Uses
Option<&str>for optionalimpl Into<String>types (Line 61)- Handles query, body, and header parameters appropriately
- Routes to correct client methods based on method type (file upload, webAuth, location, standard)
templates/rust/src/error.rs.twig (1)
1-78: LGTM on error handling design.The error type with
thiserror::Errorderive, status code classification methods (is_client_error,is_server_error), andFromimplementations for common error types provide a solid foundation for SDK error handling.templates/rust/src/input_file.rs.twig (1)
1-161: LGTM on InputFile and ChunkedReader implementation.The dual-source design (Path vs Bytes), async/sync method pairs, and chunked reading with proper seek support are well-implemented. The
Fromtrait implementations provide convenient conversion ergonomics.Also applies to: 171-294
templates/rust/src/client.rs.twig (1)
59-112: LGTM on core client architecture.The
ArcSwappattern for thread-safe mutable configuration, snapshot-based request execution, and the separation of concerns between single/chunked uploads are well-designed. Response handling correctly distinguishes JSON vs non-JSON content types.Also applies to: 332-373, 566-631, 818-874
src/SDK/Language/Rust.php (2)
284-289: Fragile string matching for collection attributes.Relying on description text like
"Collection attributes"or"List of attributes"to determine type is brittle. If the API spec descriptions change, this logic will break silently.Consider whether there's a more reliable way to identify these types (e.g., schema properties, custom annotations).
1-97: LGTM on the Rust language backend implementation.The class provides comprehensive Rust code generation support with:
- Complete keyword list and raw identifier overrides
- Well-structured file scaffolding
- Type mapping with proper handling of arrays, enums, and models
- Useful Twig filters for rustdoc comments, type resolution, and DX improvements (impl Into patterns)
Also applies to: 139-267, 312-468, 470-531, 554-608, 610-641
templates/rust/src/query.rs.twig (1)
1-208: LGTM on the Query builder implementation.Comprehensive coverage of query operations (comparison, sorting, pagination, geospatial) with ergonomic generic bounds (
impl Into<String>,impl Into<Value>). The test suite thoroughly validates serialization and edge cases.Also applies to: 234-377
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/SDK/Language/Rust.php:
- Around line 642-666: The getReturnType method declares parameters $spec,
$namespace, and $generic that are never used; update getReturnType to either
remove these parameters if they are truly unnecessary or explicitly mark them as
intentionally unused for clarity and to silence static-analysis warnings: add a
docblock above getReturnType explaining that $spec, $namespace, and $generic are
reserved for interface compatibility/future use and include @param tags for
each, and/or add a one-line intentional-unused suppression (or a no-op
reference) for $spec, $namespace, and $generic inside getReturnType so tools and
readers know they are intentionally unused.
- Around line 538-551: The parameter $generic in getPropertyType is declared but
unused; mark it as intentionally unused and document why: add a short inline
comment stating it's retained for signature consistency with the Twig filter and
then consume it to avoid unused-variable warnings (e.g., cast to void or rename
to $_generic). Update the getPropertyType function header accordingly so
tools/readers know the parameter is intentionally unused while keeping the
signature compatible with the Twig filter.
- Around line 407-467: The method getParamExample declares an unused $lang
parameter; either remove $lang from the method signature and update any callers
(or the parent/interface if applicable), or if the parameter must remain for
interface compatibility, mark it as intentionally unused by renaming to $_lang
or adding a brief comment inside getParamExample explaining it is deliberately
unused for future/compatibility reasons so linters and readers understand why it
exists; ensure any changes reference the getParamExample method to locate the
change.
- Around line 38-136: getKeywords() is missing the Rust keywords "gen", "try",
and the case-sensitive "Self"; add "gen", "try", and "Self" to the array
returned by getKeywords() in Rust.php, and update getIdentifierOverrides() to
include the mappings "gen" => "r#gen", "try" => "r#try", and "Self" => "r#Self"
so identifiers that collide with these reserved/strict keywords are properly
escaped; edit the arrays returned by the methods getKeywords() and
getIdentifierOverrides() to include these entries.
🧹 Nitpick comments (2)
src/SDK/Language/Rust.php (2)
301-305: Consider simplifying nested array type logic.The triple-nested ternary operators are difficult to read and maintain. Consider extracting this logic into a helper method or using early returns for better clarity.
♻️ Example refactor
self::TYPE_ARRAY => $this->getArrayTypeName($parameter),Add a helper method:
private function getArrayTypeName(array $parameter): string { if (isset($parameter["array"]["model"])) { return "Vec<crate::models::" . ucfirst($parameter["array"]["model"]) . ">"; } if (!empty($parameter["array"]["type"]) && !is_array($parameter["array"]["type"])) { return "Vec<" . $this->getTypeName($parameter["array"]) . ">"; } return "Vec<String>"; }
475-487: Consider wordwrap behavior in code examples.The
rustdocCommentfilter wraps text at 75 characters. This may break code examples or formatted content within documentation comments. Consider detecting and preserving code blocks or preformatted sections.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SDK/Language/Rust.phptemplates/rust/Cargo.toml.twigtemplates/rust/README.md.twigtests/languages/rust/test.sh
✅ Files skipped from review due to trivial changes (1)
- templates/rust/README.md.twig
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/rust/Cargo.toml.twig
- tests/languages/rust/test.sh
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/SDK/Language/Rust.php
273-273: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
407-407: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
538-538: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
644-644: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
645-645: Avoid unused parameters such as '$namespace'. (undefined)
(UnusedFormalParameter)
646-646: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: swift (server)
- GitHub Check: rust (server)
- GitHub Check: apple (client)
- GitHub Check: android (client)
🔇 Additional comments (6)
src/SDK/Language/Rust.php (6)
1-31: LGTM: Class structure and basic setup.The class structure, namespace, imports, and fluent setter pattern are well-implemented and follow PHP best practices.
141-266: LGTM: Comprehensive file generation configuration.The file generation array covers all essential components of a Rust crate: package manifest, source modules, tests, documentation, and CI/CD workflows. The structure follows Rust conventions.
315-335: LGTM: Rust syntax helpers are correct.The helper methods return the correct Rust syntax:
::for static access,"for string quotes, andvec![...]for array literals.
561-607: LGTM: Excellent use of Rust ergonomic patterns.The use of
impl Into<String>andimpl IntoIterator<Item = impl Into<String>>for input parameters is a best practice in Rust that significantly improves developer experience by accepting various input types (owned, borrowed, slices, etc.).
617-633: LGTM: Parameter value conversions match input types.The
.into()and.into_iter().map().collect()conversions correctly align with theimpl Intoandimpl IntoIteratorinput types fromgetInputType, ensuring type safety.
673-694: LGTM: Recursive generic type checking.The
hasGenericTypemethod correctly implements recursive traversal through model definitions to detect generic types, checking bothadditionalPropertiesand nestedsub_schemareferences.
| public function getParamExample(array $param, string $lang = ''): string | ||
| { | ||
| $type = $param["type"] ?? ""; | ||
| $example = $param["example"] ?? ""; | ||
|
|
||
| $output = ""; | ||
|
|
||
| if (empty($example) && $example !== 0 && $example !== false) { | ||
| switch ($type) { | ||
| case self::TYPE_NUMBER: | ||
| case self::TYPE_INTEGER: | ||
| $output .= "0"; | ||
| break; | ||
| case self::TYPE_BOOLEAN: | ||
| $output .= "false"; | ||
| break; | ||
| case self::TYPE_STRING: | ||
| $output .= '""'; | ||
| break; | ||
| case self::TYPE_OBJECT: | ||
| $output .= "serde_json::json!({})"; | ||
| break; | ||
| case self::TYPE_ARRAY: | ||
| $output .= "vec![]"; | ||
| break; | ||
| case self::TYPE_FILE: | ||
| $output .= 'InputFile::from_path("file.png")'; | ||
| break; | ||
| } | ||
| } else { | ||
| switch ($type) { | ||
| case self::TYPE_NUMBER: | ||
| case self::TYPE_INTEGER: | ||
| $output .= $example; | ||
| break; | ||
| case self::TYPE_BOOLEAN: | ||
| $output .= $example ? "true" : "false"; | ||
| break; | ||
| case self::TYPE_STRING: | ||
| $output .= "\"{$example}\""; | ||
| break; | ||
| case self::TYPE_OBJECT: | ||
| $output .= "serde_json::json!({})"; | ||
| break; | ||
| case self::TYPE_ARRAY: | ||
| if (\str_starts_with($example, "[")) { | ||
| $example = \substr($example, 1); | ||
| } | ||
| if (\str_ends_with($example, "]")) { | ||
| $example = \substr($example, 0, -1); | ||
| } | ||
| $output .= "vec![" . $example . "]"; | ||
| break; | ||
| case self::TYPE_FILE: | ||
| $output .= 'InputFile::from_path("file.png")'; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return $output; | ||
| } |
There was a problem hiding this comment.
Unused $lang parameter in getParamExample.
The $lang parameter is declared but never used in the method body. If it's required for interface consistency with a parent class or future use, consider adding a comment explaining this. Otherwise, it can be removed.
🔧 Proposed fix if truly unused
- public function getParamExample(array $param, string $lang = ''): string
+ public function getParamExample(array $param): string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function getParamExample(array $param, string $lang = ''): string | |
| { | |
| $type = $param["type"] ?? ""; | |
| $example = $param["example"] ?? ""; | |
| $output = ""; | |
| if (empty($example) && $example !== 0 && $example !== false) { | |
| switch ($type) { | |
| case self::TYPE_NUMBER: | |
| case self::TYPE_INTEGER: | |
| $output .= "0"; | |
| break; | |
| case self::TYPE_BOOLEAN: | |
| $output .= "false"; | |
| break; | |
| case self::TYPE_STRING: | |
| $output .= '""'; | |
| break; | |
| case self::TYPE_OBJECT: | |
| $output .= "serde_json::json!({})"; | |
| break; | |
| case self::TYPE_ARRAY: | |
| $output .= "vec![]"; | |
| break; | |
| case self::TYPE_FILE: | |
| $output .= 'InputFile::from_path("file.png")'; | |
| break; | |
| } | |
| } else { | |
| switch ($type) { | |
| case self::TYPE_NUMBER: | |
| case self::TYPE_INTEGER: | |
| $output .= $example; | |
| break; | |
| case self::TYPE_BOOLEAN: | |
| $output .= $example ? "true" : "false"; | |
| break; | |
| case self::TYPE_STRING: | |
| $output .= "\"{$example}\""; | |
| break; | |
| case self::TYPE_OBJECT: | |
| $output .= "serde_json::json!({})"; | |
| break; | |
| case self::TYPE_ARRAY: | |
| if (\str_starts_with($example, "[")) { | |
| $example = \substr($example, 1); | |
| } | |
| if (\str_ends_with($example, "]")) { | |
| $example = \substr($example, 0, -1); | |
| } | |
| $output .= "vec![" . $example . "]"; | |
| break; | |
| case self::TYPE_FILE: | |
| $output .= 'InputFile::from_path("file.png")'; | |
| break; | |
| } | |
| } | |
| return $output; | |
| } | |
| public function getParamExample(array $param): string | |
| { | |
| $type = $param["type"] ?? ""; | |
| $example = $param["example"] ?? ""; | |
| $output = ""; | |
| if (empty($example) && $example !== 0 && $example !== false) { | |
| switch ($type) { | |
| case self::TYPE_NUMBER: | |
| case self::TYPE_INTEGER: | |
| $output .= "0"; | |
| break; | |
| case self::TYPE_BOOLEAN: | |
| $output .= "false"; | |
| break; | |
| case self::TYPE_STRING: | |
| $output .= '""'; | |
| break; | |
| case self::TYPE_OBJECT: | |
| $output .= "serde_json::json!({})"; | |
| break; | |
| case self::TYPE_ARRAY: | |
| $output .= "vec![]"; | |
| break; | |
| case self::TYPE_FILE: | |
| $output .= 'InputFile::from_path("file.png")'; | |
| break; | |
| } | |
| } else { | |
| switch ($type) { | |
| case self::TYPE_NUMBER: | |
| case self::TYPE_INTEGER: | |
| $output .= $example; | |
| break; | |
| case self::TYPE_BOOLEAN: | |
| $output .= $example ? "true" : "false"; | |
| break; | |
| case self::TYPE_STRING: | |
| $output .= "\"{$example}\""; | |
| break; | |
| case self::TYPE_OBJECT: | |
| $output .= "serde_json::json!({})"; | |
| break; | |
| case self::TYPE_ARRAY: | |
| if (\str_starts_with($example, "[")) { | |
| $example = \substr($example, 1); | |
| } | |
| if (\str_ends_with($example, "]")) { | |
| $example = \substr($example, 0, -1); | |
| } | |
| $output .= "vec![" . $example . "]"; | |
| break; | |
| case self::TYPE_FILE: | |
| $output .= 'InputFile::from_path("file.png")'; | |
| break; | |
| } | |
| } | |
| return $output; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
407-407: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 407 - 467, The method getParamExample
declares an unused $lang parameter; either remove $lang from the method
signature and update any callers (or the parent/interface if applicable), or if
the parameter must remain for interface compatibility, mark it as intentionally
unused by renaming to $_lang or adding a brief comment inside getParamExample
explaining it is deliberately unused for future/compatibility reasons so linters
and readers understand why it exists; ensure any changes reference the
getParamExample method to locate the change.
| protected function getPropertyType(array $property, array $spec, string $generic = "serde_json::Value"): string | ||
| { | ||
| if (\array_key_exists("sub_schema", $property)) { | ||
| $type = "crate::models::" . ucfirst($property["sub_schema"]); | ||
|
|
||
| if ($property["type"] === "array") { | ||
| $type = "Vec<" . $type . ">"; | ||
| } | ||
| } else { | ||
| $type = $this->getTypeName($property, $spec); | ||
| } | ||
|
|
||
| return $type; | ||
| } |
There was a problem hiding this comment.
Unused $generic parameter in getPropertyType.
The $generic parameter is declared but never used in the method body. It's likely retained for signature consistency with the Twig filter definition (line 488-494), but this could be clarified with a comment.
🧰 Tools
🪛 PHPMD (2.15.0)
538-538: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 538 - 551, The parameter $generic in
getPropertyType is declared but unused; mark it as intentionally unused and
document why: add a short inline comment stating it's retained for signature
consistency with the Twig filter and then consume it to avoid unused-variable
warnings (e.g., cast to void or rename to $_generic). Update the getPropertyType
function header accordingly so tools/readers know the parameter is intentionally
unused while keeping the signature compatible with the Twig filter.
| protected function getReturnType( | ||
| array $method, | ||
| array $spec, | ||
| string $namespace, | ||
| string $generic = "serde_json::Value", | ||
| ): string { | ||
| if ($method["type"] === "webAuth") { | ||
| return "crate::error::Result<String>"; | ||
| } | ||
| if ($method["type"] === "location") { | ||
| return "crate::error::Result<Vec<u8>>"; | ||
| } | ||
|
|
||
| if ( | ||
| !\array_key_exists("responseModel", $method) || | ||
| empty($method["responseModel"]) || | ||
| $method["responseModel"] === "any" | ||
| ) { | ||
| return "crate::error::Result<serde_json::Value>"; | ||
| } | ||
|
|
||
| $ret = ucfirst($method["responseModel"]); | ||
|
|
||
| return "crate::error::Result<crate::models::" . $ret . ">"; | ||
| } |
There was a problem hiding this comment.
Unused parameters in getReturnType method.
The $spec, $namespace, and $generic parameters (lines 644-646) are declared but not used in the method body. These may be required for interface compatibility or reserved for future enhancements. Consider adding a docblock comment to clarify their purpose or removing them if truly unnecessary.
🧰 Tools
🪛 PHPMD (2.15.0)
644-644: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
645-645: Avoid unused parameters such as '$namespace'. (undefined)
(UnusedFormalParameter)
646-646: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 642 - 666, The getReturnType method
declares parameters $spec, $namespace, and $generic that are never used; update
getReturnType to either remove these parameters if they are truly unnecessary or
explicitly mark them as intentionally unused for clarity and to silence
static-analysis warnings: add a docblock above getReturnType explaining that
$spec, $namespace, and $generic are reserved for interface compatibility/future
use and include @param tags for each, and/or add a one-line intentional-unused
suppression (or a no-op reference) for $spec, $namespace, and $generic inside
getReturnType so tools and readers know they are intentionally unused.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/SDK/Language/Rust.php:
- Around line 696-717: The hasGenericType method accesses
$spec["definitions"][$model], $model["additionalProperties"], and iterates
$model["properties"] without null/isset checks; update hasGenericType to first
verify that $model is a key in $spec["definitions"] (use isset or
array_key_exists) and return false if missing, then check that the resolved
$model is an array and that "additionalProperties" exists before evaluating it,
and ensure "properties" exists and is an array before looping and checking each
property's "sub_schema" to avoid undefined index/null errors when recursing.
In @templates/rust/src/client.rs.twig:
- Around line 15-16: The DEFAULT_TIMEOUT constant (const DEFAULT_TIMEOUT: u64 =
10) is too short for file uploads; either increase the default or add a separate
upload-specific timeout and use it for upload requests. Change the constant
strategy by adding a new UPLOAD_TIMEOUT (e.g., 60s) or bump DEFAULT_TIMEOUT,
then update the client code paths that perform file upload operations to use the
new upload timeout (references: DEFAULT_TIMEOUT constant and the upload request
function/method in client.rs.twig) and ensure any request builder or HTTP client
initialization uses the appropriate timeout for uploads.
🧹 Nitpick comments (7)
templates/rust/src/models/model.rs.twig (2)
7-7: Unused template variablefieldNames.The variable
fieldNamesis declared but never used in this template. Consider removing it to reduce template clutter.Proposed fix
-{% set fieldNames = {} %} - {{ definition.description | rustdocComment }}
33-37: Consider extracting field name conversion to a Twig macro.The field name conversion logic is duplicated between the struct definition (lines 14-16) and the impl block (lines 35-37). If this logic needs to change, both locations must be updated in sync. Consider extracting this to a reusable Twig macro or filter to ensure consistency.
templates/rust/src/client.rs.twig (2)
125-136: Consider returningResultinstead of panicking on invalid endpoint.Panicking in library code is generally discouraged as it removes control from the caller. Consider returning a
Result<Self, Error>to allow callers to handle invalid endpoints gracefully.Alternative approach
/// Set the API endpoint pub fn set_endpoint<S: Into<String>>(&self, endpoint: S) -> Result<Self> { let endpoint = endpoint.into(); if !endpoint.starts_with("http://") && !endpoint.starts_with("https://") { return Err({{ spec.title | caseUcfirst }}Error::new( 0, format!("Invalid endpoint URL: {}. Endpoint must start with http:// or https://", endpoint), None, String::new(), )); } // ... rest of implementation Ok(self.clone()) }
217-234: Silent failure when adding invalid headers could cause debugging difficulties.The
add_headermethod silently ignores invalid header names or values (lines 225-231). Consider logging a warning or returning aResultto inform callers when headers cannot be added.src/SDK/Language/Rust.php (3)
279-318: The$specparameter is never used ingetTypeName.While
$specis passed to the method and appears in the signature for API consistency, it's never actually used in the method body. The recursive call at line 312 also doesn't pass$spec. If this is intentional for interface compatibility, consider adding a comment to clarify.Additionally, the complex ternary expression at lines 309-313 could be simplified for readability.
Simplified array handling
self::TYPE_ARRAY => $this->getArrayTypeName($parameter),With a separate helper method:
private function getArrayTypeName(array $parameter): string { if (isset($parameter["array"]["model"])) { return "Vec<crate::models::" . ucfirst($parameter["array"]["model"]) . ">"; } $arrayItems = $parameter["array"] ?? []; if (!empty($arrayItems["type"]) && !is_array($arrayItems["type"])) { return "Vec<" . $this->getTypeName($arrayItems) . ">"; } return "Vec<String>"; }
650-679: Several parameters ingetReturnTypeare unused.The parameters
$spec,$namespace, and$genericare never used in this method. If they're required for interface consistency with other language implementations, consider adding@SuppressWarnings(PHPMD.UnusedFormalParameter)or a comment explaining why they're present.Add suppression annotation
/** * @param array $method * @param array $spec * @param string $namespace * @param string $generic * @return string * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ protected function getReturnType(
681-689: Unused$responsevariable in foreach loop.The
$responsevariable is never used; only the$codekey is needed.Proposed fix
protected function isEmptyResponse(array $responses): bool { - foreach ($responses as $code => $response) { + foreach (array_keys($responses) as $code) { if (!in_array((int)$code, [204, 205])) { return false; } } return !empty($responses); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/Rust.phptemplates/rust/src/client.rs.twigtemplates/rust/src/models/model.rs.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/Rust.php (1)
src/SDK/Language.php (2)
Language(5-295)toPascalCase(116-119)
🪛 PHPMD (2.15.0)
src/SDK/Language/Rust.php
279-279: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
415-415: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
546-546: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
652-652: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
653-653: Avoid unused parameters such as '$namespace'. (undefined)
(UnusedFormalParameter)
654-654: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
683-683: Avoid unused local variables such as '$response'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: react-native (client)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, Rust183)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLIBun11)
- GitHub Check: dotnet (server)
- GitHub Check: rust (server)
- GitHub Check: apple (client)
- GitHub Check: swift (server)
- GitHub Check: android (client)
🔇 Additional comments (8)
templates/rust/src/models/model.rs.twig (1)
70-96: LGTM!The test module provides basic validation for model creation and serialization round-trips. The use of
#[cfg_attr(test, derive(Default))]is a good approach to enable testing without requiring Default in production code.templates/rust/src/client.rs.twig (4)
369-470: LGTM with minor observation on duplicated header logic.The
call_locationmethod correctly handles redirect responses for webAuth endpoints. The header application logic is duplicated (lines 389-395 and 407-412) when handling GET requests with params, but this is functionally correct.
676-802: LGTM!The chunked file upload implementation with resumable upload support is well-structured. The use of
Content-Rangeheaders and the ability to resume from a specific chunk offset is correctly implemented. The defensive check for empty chunks at lines 726-728 adds robustness.
826-830: Empty response handling may fail for types that don't deserialize fromnull.When the response body is empty (line 826-827), deserializing
b"null"works for()andOption<T>but will fail for types that don't accept JSONnull. This could cause confusing errors when the API returns an empty 200 response but the caller expects a concrete type.Consider documenting this behavior or returning a more descriptive error when deserialization of
nullfails.
863-882: LGTM!The tests adequately cover client creation and the builder pattern configuration.
src/SDK/Language/Rust.php (3)
38-142: LGTM!The Rust keywords list and identifier overrides are comprehensive. The use of
r#prefix for raw identifiers is the correct Rust approach for escaping reserved words.
383-405: Object and array$defaultvalues are ignored.When a non-empty
$defaultis provided forTYPE_OBJECTorTYPE_ARRAY(lines 395-400), the actual default value is ignored andserde_json::Value::NullorVec::new()is returned instead. If API specs define meaningful object/array defaults, they won't be reflected in the generated code.Verify if this is intentional behavior or if object/array defaults need to be serialized to Rust syntax.
480-538: LGTM!The Twig filters are well-designed for Rust code generation. The
rustdocCommentfilter correctly handles multi-line documentation, and therustTypefilter properly unescapes HTML entities that Twig may introduce.
| /// Default request timeout in seconds | ||
| const DEFAULT_TIMEOUT: u64 = 10; |
There was a problem hiding this comment.
10-second timeout may be too short for file uploads.
The DEFAULT_TIMEOUT of 10 seconds is applied to all HTTP requests including file uploads. Large file uploads could easily exceed this timeout, especially on slower connections. Consider either increasing this default or using a different timeout strategy for upload operations.
🤖 Prompt for AI Agents
In @templates/rust/src/client.rs.twig around lines 15 - 16, The DEFAULT_TIMEOUT
constant (const DEFAULT_TIMEOUT: u64 = 10) is too short for file uploads; either
increase the default or add a separate upload-specific timeout and use it for
upload requests. Change the constant strategy by adding a new UPLOAD_TIMEOUT
(e.g., 60s) or bump DEFAULT_TIMEOUT, then update the client code paths that
perform file upload operations to use the new upload timeout (references:
DEFAULT_TIMEOUT constant and the upload request function/method in
client.rs.twig) and ensure any request builder or HTTP client initialization
uses the appropriate timeout for uploads.
fix: uploadId. add: tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/SDK/Language/Rust.php:
- Around line 701-722: The hasGenericType method accesses
$model["additionalProperties"] directly which can be undefined; update
hasGenericType to guard that access by checking array_key_exists or using null
coalescing (e.g., treat missing additionalProperties as false) before evaluating
it, so replace direct $model["additionalProperties"] usage in hasGenericType
with a safe check (for example $model['additionalProperties'] ?? false) and keep
the existing logic otherwise.
- Around line 464-471: The current manual substr-based bracket stripping in the
TYPE_ARRAY case is brittle for nested or malformed arrays; replace lines that
check str_starts_with/str_ends_with and substr with a single regex match that
removes only one outermost bracket pair: use preg_match('/^\[(.*)\]$/s',
$example, $m) and if it matches set $example = $m[1]; keep the rest that builds
$output ("vec![" . $example . "]").
- Around line 38-142: The getKeywords() array in Rust.php contains
invalid/non-existent Rust keywords (alignof, offsetof, proc, pure, sizeof);
remove these five entries from the return array in the getKeywords() method and
ensure getIdentifierOverrides() does not reference any of them (update
getIdentifierOverrides() if necessary); keep all other keywords intact and
return the cleaned list from getKeywords().
🧹 Nitpick comments (6)
templates/rust/src/query.rs.twig (3)
210-232: Consider acceptingQueryobjects directly instead of JSON strings.The current API requires users to serialize queries to strings and then deserialize them back:
let query_strings = vec![ Query::equal("released", true).to_string(), Query::less_than("releasedYear", 1990).to_string(), ]; let query = Query::or(query_strings)?;This round-trip through JSON is inefficient and error-prone. Consider accepting
Queryobjects directly:♻️ Proposed alternative API
- pub fn or<I, S>(queries: I) -> crate::error::Result<Self> - where - I: IntoIterator<Item = S>, - S: AsRef<str>, - { - let values: Vec<Value> = queries - .into_iter() - .map(|query| serde_json::from_str(query.as_ref())) - .collect::<Result<Vec<_>, _>>()?; - Ok(Self::new("or".to_string(), None, values)) - } + pub fn or<I>(queries: I) -> Self + where + I: IntoIterator<Item = Query>, + { + let values: Vec<Value> = queries + .into_iter() + .map(|q| q.to_value()) + .collect(); + Self::new("or".to_string(), None, values) + }This allows simpler usage:
let query = Query::or(vec![ Query::equal("released", true), Query::less_than("releasedYear", 1990), ]);If backward compatibility with string inputs is needed, consider providing both APIs (e.g.,
orandor_from_strings).
251-257: Minor efficiency concern in Display implementation.The
to_value()method consumesself, requiring a clone inDisplay::fmt. This is fine for correctness but adds an allocation on every display call.♻️ Optional: Add a non-consuming to_value_ref method
If display performance matters, consider adding a method that borrows:
fn to_value_ref(&self) -> Value { let mut obj = serde_json::Map::new(); obj.insert("method".to_string(), Value::String(self.method.clone())); if let Some(attr) = &self.attribute { obj.insert("attribute".to_string(), Value::String(attr.clone())); } if !self.values.is_empty() { obj.insert("values".to_string(), Value::Array(self.values.clone())); } Value::Object(obj) } impl std::fmt::Display for Query { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let json_str = serde_json::to_string(&self.to_value_ref()) .unwrap_or_else(|_| String::new()); write!(f, "{}", json_str) } }
6-11: Consider derivingSerializefor simpler JSON conversion.The
Querystruct manually implementsto_value()andDisplay. DerivingSerializefrom serde could simplify this and ensure consistency.♻️ Optional: Use serde derive
+use serde::Serialize; + /// Query builder for filtering and sorting database queries -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] pub struct Query { method: String, + #[serde(skip_serializing_if = "Option::is_none")] attribute: Option<String>, + #[serde(skip_serializing_if = "Vec::is_empty")] values: Vec<Value>, }Then
to_value()becomes:pub fn to_value(self) -> Value { serde_json::to_value(&self).unwrap() }This is optional since the current implementation works correctly and matches the explicit format requirements.
src/SDK/Language/Rust.php (3)
284-323: UsetoPascalCase()for consistent naming convention.Lines 288, 291, 315, and 320 use
ucfirst()to capitalize type names. For consistency with other language generators in this codebase and to handle multi-word identifiers properly, consider using the inheritedtoPascalCase()method instead.♻️ Proposed refactor for consistent naming
// Handle enum types if (isset($parameter["enumName"])) { - return "crate::enums::" . ucfirst($parameter["enumName"]); + return "crate::enums::" . $this->toPascalCase($parameter["enumName"]); } if (!empty($parameter["enumValues"])) { - return "crate::enums::" . ucfirst($parameter["name"]); + return "crate::enums::" . $this->toPascalCase($parameter["name"]); } // ... later in the match expression ... self::TYPE_ARRAY => isset($parameter["array"]["model"]) - ? "Vec<crate::models::" . ucfirst($parameter["array"]["model"]) . ">" + ? "Vec<crate::models::" . $this->toPascalCase($parameter["array"]["model"]) . ">" : (!empty(($parameter["array"] ?? [])["type"]) && !\is_array($parameter["array"]["type"]) ? "Vec<" . $this->getTypeName($parameter["array"]) . ">" : "Vec<String>"), default => isset($parameter["model"]) - ? "crate::models::" . ucfirst($parameter["model"]) + ? "crate::models::" . $this->toPascalCase($parameter["model"]) : $parameter["type"],
354-413: Consider consolidating duplicate switch logic.Lines 367-387 and 389-410 contain nearly identical switch statements with only minor differences in output values. This duplication makes maintenance harder.
♻️ Proposed refactor to reduce duplication
public function getParamDefault(array $param): string { $type = $param["type"] ?? ""; $default = $param["default"] ?? ""; $required = $param["required"] ?? ""; if ($required) { return ""; } $output = " = "; + $hasExplicitDefault = !empty($default) || $default === 0 || $default === false; - if (empty($default) && $default !== 0 && $default !== false) { - switch ($type) { - case self::TYPE_NUMBER: - case self::TYPE_INTEGER: - $output .= "0"; - break; - case self::TYPE_BOOLEAN: - $output .= "false"; - break; - case self::TYPE_STRING: - $output .= "String::new()"; - break; - case self::TYPE_OBJECT: - $output .= "serde_json::Value::Null"; - break; - case self::TYPE_ARRAY: - $output .= "Vec::new()"; - break; - case self::TYPE_FILE: - $output .= "InputFile::default()"; - break; - } - } else { - switch ($type) { - case self::TYPE_NUMBER: - case self::TYPE_INTEGER: - $output .= $default; - break; - case self::TYPE_BOOLEAN: - $output .= $default ? "true" : "false"; - break; - case self::TYPE_STRING: - $output .= "String::from(\"$default\")"; - break; - case self::TYPE_OBJECT: - $output .= "serde_json::Value::Null"; - break; - case self::TYPE_ARRAY: - $output .= "Vec::new()"; - break; - case self::TYPE_FILE: - $output .= "InputFile::default()"; - break; - } - } + switch ($type) { + case self::TYPE_NUMBER: + case self::TYPE_INTEGER: + $output .= $hasExplicitDefault ? $default : "0"; + break; + case self::TYPE_BOOLEAN: + $output .= ($hasExplicitDefault && $default) ? "true" : "false"; + break; + case self::TYPE_STRING: + $output .= $hasExplicitDefault ? "String::from(\"$default\")" : "String::new()"; + break; + case self::TYPE_OBJECT: + $output .= "serde_json::Value::Null"; + break; + case self::TYPE_ARRAY: + $output .= "Vec::new()"; + break; + case self::TYPE_FILE: + $output .= "InputFile::default()"; + break; + } return $output; }
686-694: Unused loop variable inisEmptyResponse.Line 688: The
$responsevariable in the foreach loop is never used. You can replace it with an underscore or usearray_keys()if only codes are needed.♻️ Proposed fix
protected function isEmptyResponse(array $responses): bool { - foreach ($responses as $code => $response) { + foreach ($responses as $code => $_) { if (!in_array((int)$code, [204, 205])) { return false; } } return !empty($responses); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SDK/Language/Rust.phptemplates/rust/src/lib.rs.twigtemplates/rust/src/query.rs.twigtemplates/rust/src/services/service.rs.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/rust/src/services/service.rs.twig
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T12:47:16.427Z
Learnt from: ItzNotABug
Repo: appwrite/sdk-generator PR: 1273
File: src/Spec/Swagger2.php:167-179
Timestamp: 2025-12-25T12:47:16.427Z
Learning: In src/Spec/Swagger2.php, when parsing `x-oneOf` entries from OpenAPI specs, the `$ref` key is always guaranteed to be present by design. If it's missing, it indicates a broader spec validation issue rather than a normal edge case to handle.
Applied to files:
src/SDK/Language/Rust.php
🧬 Code graph analysis (1)
src/SDK/Language/Rust.php (1)
src/SDK/Language.php (2)
Language(5-295)toPascalCase(116-119)
🪛 PHPMD (2.15.0)
src/SDK/Language/Rust.php
284-284: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
420-420: Avoid unused parameters such as '$lang'. (undefined)
(UnusedFormalParameter)
551-551: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
657-657: Avoid unused parameters such as '$spec'. (undefined)
(UnusedFormalParameter)
658-658: Avoid unused parameters such as '$namespace'. (undefined)
(UnusedFormalParameter)
659-659: Avoid unused parameters such as '$generic'. (undefined)
(UnusedFormalParameter)
688-688: Avoid unused local variables such as '$response'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Go112)
- GitHub Check: dotnet (server)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Go118)
- GitHub Check: react-native (client)
- GitHub Check: android (client)
- GitHub Check: flutter (client)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (10)
templates/rust/src/lib.rs.twig (1)
1-63: LGTM! Clean library structure with good documentation.The module organization follows Rust conventions with clear separation between core functionality and utility modules. The re-exports, Result type alias, and SDK constants are well-designed.
One minor observation: the usage example at lines 21-24 shows
set_*methods returningClient, implying a builder pattern. Ensure these methods inclient.rs.twigactually returnSelffor method chaining to work as documented.templates/rust/src/query.rs.twig (1)
259-654: Excellent test coverage.The test suite is comprehensive, covering:
- All query operators (equal, not_equal, comparison operators, etc.)
- Pagination methods (limit, offset, cursor_*)
- Spatial/geometry queries
- Composite queries (or/and) including error cases
- JSON serialization round-trip
src/SDK/Language/Rust.php (8)
1-31: LGTM: Clean class structure and configuration.The class declaration, namespace imports, and basic configuration methods follow proper PHP conventions and the parent Language abstract class pattern.
147-277: LGTM: Comprehensive Rust crate scaffolding.The file structure follows idiomatic Rust conventions with proper module organization (
src/lib.rs,src/models/mod.rs,src/services/mod.rs), test structure (tests/tests.rs), and CI workflows. The template scoping (default, copy, service, definition, etc.) provides good flexibility for code generation.
325-348: LGTM: Rust-specific syntax helpers.The utility methods correctly return Rust-specific syntax:
::for static access,"for string quotes, andvec![...]for arrays.
485-543: LGTM: Well-designed Twig filters for Rust codegen.The filters cover essential transformations:
rustdocComment: Properly formats documentation with///prefix and word wrappingpropertyType,inputType,returnType: Delegate to type resolution helperscaseEnumKey: Correctly usestoPascalCase()for enum variantsrustType: Unescapes HTML entities for angle brackets in genericsrustCrateName: Converts hyphens to underscores per Rust naming conventionsstripProtocol: Utility for cleaning URLs
551-620: LGTM: Excellent use ofimpl Into<T>for ergonomic APIs.The
getInputTypemethod demonstrates good Rust API design by using:
impl Into<String>to accept&str,String,Cow<str>, etc.impl IntoIterator<Item = impl Into<String>>to accept slices, arrays, and vectorsThis significantly improves the developer experience while maintaining type safety. The careful checks to avoid wrapping enums, models, and primitives in
Intotraits show attention to detail.
622-646: LGTM: Proper parameter value conversions.The
getParamValuemethod correctly applies necessary transformations:
.into()forStringparameters acceptingimpl Into<String>.into_iter().map(|s| s.into()).collect()forVec<String>parameters accepting iteratorsThis pairs well with the
getInputTypemethod's trait bounds.
655-684: LGTM: Comprehensive return type handling.The
getReturnTypemethod correctly handles various response scenarios:
webAuth→Result<String>for OAuth flowslocation→Result<Vec<u8>>for binary/redirect responses- Empty responses (204/205) →
Result<()>- Generic/unknown responses →
Result<serde_json::Value>- Typed responses →
Result<crate::models::ModelName>
686-694: LGTM: Correct empty response detection.The
isEmptyResponsemethod correctly identifies HTTP 204 (No Content) and 205 (Reset Content) status codes as empty responses. The logic ensures at least one response is present before returning true.
| public function getKeywords(): array | ||
| { | ||
| return [ | ||
| "abstract", | ||
| "alignof", | ||
| "as", | ||
| "become", | ||
| "box", | ||
| "break", | ||
| "const", | ||
| "continue", | ||
| "crate", | ||
| "do", | ||
| "else", | ||
| "enum", | ||
| "extern", | ||
| "false", | ||
| "final", | ||
| "fn", | ||
| "for", | ||
| "if", | ||
| "impl", | ||
| "in", | ||
| "let", | ||
| "loop", | ||
| "macro", | ||
| "match", | ||
| "mod", | ||
| "move", | ||
| "mut", | ||
| "offsetof", | ||
| "override", | ||
| "priv", | ||
| "proc", | ||
| "pub", | ||
| "pure", | ||
| "ref", | ||
| "return", | ||
| "self", | ||
| "sizeof", | ||
| "static", | ||
| "struct", | ||
| "super", | ||
| "trait", | ||
| "true", | ||
| "type", | ||
| "typeof", | ||
| "unsafe", | ||
| "unsized", | ||
| "use", | ||
| "virtual", | ||
| "where", | ||
| "while", | ||
| "yield", | ||
| "async", | ||
| "await", | ||
| "dyn", | ||
| "union", | ||
| "gen", | ||
| "try", | ||
| "Self", | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * @return array | ||
| */ | ||
| public function getIdentifierOverrides(): array | ||
| { | ||
| return [ | ||
| "type" => "r#type", | ||
| "ref" => "r#ref", | ||
| "move" => "r#move", | ||
| "static" => "r#static", | ||
| "const" => "r#const", | ||
| "struct" => "r#struct", | ||
| "enum" => "r#enum", | ||
| "trait" => "r#trait", | ||
| "impl" => "r#impl", | ||
| "fn" => "r#fn", | ||
| "let" => "r#let", | ||
| "mut" => "r#mut", | ||
| "use" => "r#use", | ||
| "pub" => "r#pub", | ||
| "crate" => "r#crate", | ||
| "mod" => "r#mod", | ||
| "super" => "r#super", | ||
| "self" => "r#self", | ||
| "where" => "r#where", | ||
| "async" => "r#async", | ||
| "gen" => "r#gen", | ||
| "try" => "r#try", | ||
| "Self" => "r#Self", | ||
| "await" => "r#await", | ||
| "loop" => "r#loop", | ||
| "while" => "r#while", | ||
| "for" => "r#for", | ||
| "if" => "r#if", | ||
| "else" => "r#else", | ||
| "match" => "r#match", | ||
| "return" => "r#return", | ||
| "break" => "r#break", | ||
| "continue" => "r#continue", | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust keywords complete list official reference 2024 2025
💡 Result:
Here are the Rust keywords from the official reference (including 2024 reserved-keyword changes). Sources: [1][2].
Strict keywords (cannot be used as identifiers)
_ , as , async , await , break , const , continue , crate , dyn , else , enum , extern , false , fn , for , if , impl , in , let , loop , match , mod , move , mut , pub , ref , return , self , Self , static , struct , super , trait , true , type , unsafe , use , where , while. [1]
Reserved keywords (currently reserved for future use; includes 2024 addition)
abstract , become , box , do , final , gen , macro , override , priv , try , typeof , unsized , virtual , yield. [1][2]
Weak/contextual keywords (only keywords in specific contexts)
'static , macro_rules , raw , safe , union. [1]
Sources:
[1] The Rust Reference — Keywords (official).
[2] Rust 2024 Edition Guide — gen keyword (reservation).
🏁 Script executed:
#!/bin/bash
# Verify Rust keywords from official documentation
# Check the Rust reference book for authoritative keyword list
curl -s "https://doc.rust-lang.org/reference/keywords.html" | head -500Repository: appwrite/sdk-generator
Length of output: 20967
Remove non-existent keywords from the list: alignof, offsetof, proc, pure, sizeof
The getKeywords() array includes five keywords that are not present in the official Rust reference: alignof, offsetof, proc, pure, and sizeof. These appear to be remnants from very early Rust versions or possibly from C/C++ interop, but they are not valid Rust keywords in any current edition (including 2024). Remove these to maintain accuracy.
All legitimate Rust keywords (strict, reserved, and contextual) are correctly included. The list matches the official Rust reference documentation.
🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 38 - 142, The getKeywords() array in
Rust.php contains invalid/non-existent Rust keywords (alignof, offsetof, proc,
pure, sizeof); remove these five entries from the return array in the
getKeywords() method and ensure getIdentifierOverrides() does not reference any
of them (update getIdentifierOverrides() if necessary); keep all other keywords
intact and return the cleaned list from getKeywords().
| case self::TYPE_ARRAY: | ||
| if (\str_starts_with($example, "[")) { | ||
| $example = \substr($example, 1); | ||
| } | ||
| if (\str_ends_with($example, "]")) { | ||
| $example = \substr($example, 0, -1); | ||
| } | ||
| $output .= "vec![" . $example . "]"; |
There was a problem hiding this comment.
Fragile bracket stripping for array examples.
Lines 465-470 manually strip leading/trailing brackets from array examples. This approach is brittle and could fail with nested arrays or malformed input (e.g., "[[1,2]]" would become "[1,2]" then produce vec![[1,2]]).
🔧 Proposed fix using trim
case self::TYPE_ARRAY:
- if (\str_starts_with($example, "[")) {
- $example = \substr($example, 1);
- }
- if (\str_ends_with($example, "]")) {
- $example = \substr($example, 0, -1);
- }
+ $example = trim($example, '[]');
$output .= "vec![" . $example . "]";
break;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 464 - 471, The current manual
substr-based bracket stripping in the TYPE_ARRAY case is brittle for nested or
malformed arrays; replace lines that check str_starts_with/str_ends_with and
substr with a single regex match that removes only one outermost bracket pair:
use preg_match('/^\[(.*)\]$/s', $example, $m) and if it matches set $example =
$m[1]; keep the rest that builds $output ("vec![" . $example . "]").
| protected function hasGenericType(?string $model, array $spec): bool | ||
| { | ||
| if (empty($model) || $model === "any") { | ||
| return false; | ||
| } | ||
|
|
||
| $model = $spec["definitions"][$model]; | ||
|
|
||
| if ($model["additionalProperties"]) { | ||
| return true; | ||
| } | ||
|
|
||
| foreach ($model["properties"] as $property) { | ||
| if (!\array_key_exists("sub_schema", $property) || !$property["sub_schema"]) { | ||
| continue; | ||
| } | ||
|
|
||
| return $this->hasGenericType($property["sub_schema"], $spec); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Potential undefined index access on additionalProperties.
Line 709 directly accesses $model["additionalProperties"] without checking if the key exists. If the model definition doesn't include this key, PHP will issue a warning/notice.
🔧 Proposed fix using null coalescing
protected function hasGenericType(?string $model, array $spec): bool
{
if (empty($model) || $model === "any") {
return false;
}
$model = $spec["definitions"][$model];
- if ($model["additionalProperties"]) {
+ if ($model["additionalProperties"] ?? false) {
return true;
}
foreach ($model["properties"] as $property) {
if (!\array_key_exists("sub_schema", $property) || !$property["sub_schema"]) {
continue;
}
return $this->hasGenericType($property["sub_schema"], $spec);
}
return false;
}🤖 Prompt for AI Agents
In @src/SDK/Language/Rust.php around lines 701 - 722, The hasGenericType method
accesses $model["additionalProperties"] directly which can be undefined; update
hasGenericType to guard that access by checking array_key_exists or using null
coalescing (e.g., treat missing additionalProperties as false) before evaluating
it, so replace direct $model["additionalProperties"] usage in hasGenericType
with a safe check (for example $model['additionalProperties'] ?? false) and keep
the existing logic otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @templates/rust/src/operator.rs.twig:
- Around line 47-156: validate_numeric currently misses Value::Null and
non-numeric JSON (strings/booleans) and therefore lets NaN/Infinity or
non-numbers through; update validate_numeric to first assert value.is_number()
(panic with param_name if false), then use as_f64() to check for is_nan() or
is_infinite(); extract the repeated "divisor != 0" check into a small helper
(e.g., validate_nonzero_divisor) that accepts &Value and a name and checks
number-ness then tests as_f64()/as_i64() for zero, and call that helper from
divide, divide_with_min and modulo; ensure all numeric operator builders
(increment_by, increment_with_max, decrement_by, decrement_with_min, multiply,
multiply_with_max, etc.) call the tightened validate_numeric so non-numeric
Value::Null or strings/booleans are rejected consistently.
🧹 Nitpick comments (2)
templates/rust/src/operator.rs.twig (2)
33-45: Consider avoidingmethod: Stringallocation and/or returningResult<String, _>fromparse_operator.
serde_json::to_stringhere should be effectively infallible (givenValue), but the API currently commits you to panics and repeated allocations.
257-412: Add tests for NaN/Infinity + non-number inputs, and coverdivide_with_min(0, ...).
The zero-panic tests are good; extending coverage here will lock in the numeric contract.Example test additions
#[test] #[should_panic(expected = "divisor cannot be zero")] fn test_divide_zero() { divide(0); } + #[test] + #[should_panic(expected = "divisor cannot be zero")] + fn test_divide_with_min_zero() { + divide_with_min(0, 1); + } + #[test] #[should_panic(expected = "divisor cannot be zero")] fn test_modulo_zero() { modulo(0); } + + #[test] + #[should_panic(expected = "value must be a finite number")] + fn test_increment_by_nan() { + increment_by(f64::NAN); + } + + #[test] + #[should_panic(expected = "value must be a finite number")] + fn test_increment_by_non_number() { + increment_by("not-a-number"); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoretemplates/rust/src/operator.rs.twigtemplates/rust/src/query.rs.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/rust/src/query.rs.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLIBun13)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: rust (server)
- GitHub Check: android (client)
- GitHub Check: swift (server)
- GitHub Check: react-native (client)
- GitHub Check: kotlin (server)
- GitHub Check: apple (client)
🔇 Additional comments (2)
.gitignore (1)
28-28: LGTM!The addition of the
demosdirectory to.gitignorealigns well with the PR's goal of generating example and demo code while keeping version control clean. This mirrors the existingexamples/*exclusion and makes sense for a Rust SDK generation feature.templates/rust/src/operator.rs.twig (1)
172-229: Wire format for array operators is correct. The current implementation matches the canonical behavior across SDKs (Node.js, etc.). Array operators intentionally flatten slice values into thevaluesarray as{"method":"arrayAppend","values":[a,b]}, not{"method":"arrayAppend","values":[[a,b]]}. No changes needed.Likely an incorrect or invalid review comment.
| fn validate_numeric(value: &Value, param_name: &str) { | ||
| if let Some(num) = value.as_f64() { | ||
| if num.is_nan() || num.is_infinite() { | ||
| panic!("{} cannot be NaN or Infinity", param_name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn increment() -> String { | ||
| parse_operator("increment", vec![Value::from(1)]) | ||
| } | ||
|
|
||
| pub fn increment_by<T: Into<Value>>(value: T) -> String { | ||
| let val = value.into(); | ||
| validate_numeric(&val, "value"); | ||
| parse_operator("increment", vec![val]) | ||
| } | ||
|
|
||
| pub fn increment_with_max<T: Into<Value>, M: Into<Value>>(value: T, max: M) -> String { | ||
| let val = value.into(); | ||
| let max_val = max.into(); | ||
| validate_numeric(&val, "value"); | ||
| validate_numeric(&max_val, "max"); | ||
| parse_operator("increment", vec![val, max_val]) | ||
| } | ||
|
|
||
| pub fn decrement() -> String { | ||
| parse_operator("decrement", vec![Value::from(1)]) | ||
| } | ||
|
|
||
| pub fn decrement_by<T: Into<Value>>(value: T) -> String { | ||
| let val = value.into(); | ||
| validate_numeric(&val, "value"); | ||
| parse_operator("decrement", vec![val]) | ||
| } | ||
|
|
||
| pub fn decrement_with_min<T: Into<Value>, M: Into<Value>>(value: T, min: M) -> String { | ||
| let val = value.into(); | ||
| let min_val = min.into(); | ||
| validate_numeric(&val, "value"); | ||
| validate_numeric(&min_val, "min"); | ||
| parse_operator("decrement", vec![val, min_val]) | ||
| } | ||
|
|
||
| pub fn multiply<T: Into<Value>>(factor: T) -> String { | ||
| let val = factor.into(); | ||
| validate_numeric(&val, "factor"); | ||
| parse_operator("multiply", vec![val]) | ||
| } | ||
|
|
||
| pub fn multiply_with_max<T: Into<Value>, M: Into<Value>>(factor: T, max: M) -> String { | ||
| let val = factor.into(); | ||
| let max_val = max.into(); | ||
| validate_numeric(&val, "factor"); | ||
| validate_numeric(&max_val, "max"); | ||
| parse_operator("multiply", vec![val, max_val]) | ||
| } | ||
|
|
||
| pub fn divide<T: Into<Value>>(divisor: T) -> String { | ||
| let val = divisor.into(); | ||
| validate_numeric(&val, "divisor"); | ||
|
|
||
| if let Some(num) = val.as_f64() { | ||
| if num == 0.0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } else if let Some(num) = val.as_i64() { | ||
| if num == 0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } | ||
|
|
||
| parse_operator("divide", vec![val]) | ||
| } | ||
|
|
||
| pub fn divide_with_min<T: Into<Value>, M: Into<Value>>(divisor: T, min: M) -> String { | ||
| let val = divisor.into(); | ||
| let min_val = min.into(); | ||
| validate_numeric(&val, "divisor"); | ||
| validate_numeric(&min_val, "min"); | ||
|
|
||
| if let Some(num) = val.as_f64() { | ||
| if num == 0.0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } else if let Some(num) = val.as_i64() { | ||
| if num == 0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } | ||
|
|
||
| parse_operator("divide", vec![val, min_val]) | ||
| } | ||
|
|
||
| pub fn modulo<T: Into<Value>>(divisor: T) -> String { | ||
| let val = divisor.into(); | ||
| validate_numeric(&val, "divisor"); | ||
|
|
||
| if let Some(num) = val.as_f64() { | ||
| if num == 0.0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } else if let Some(num) = val.as_i64() { | ||
| if num == 0 { | ||
| panic!("divisor cannot be zero"); | ||
| } | ||
| } | ||
|
|
||
| parse_operator("modulo", vec![val]) | ||
| } |
There was a problem hiding this comment.
Fix numeric validation: NaN/Inf can currently slip through as null, and non-numbers are accepted for numeric ops.
Because the numeric functions take T: Into<Value>, passing f64::NAN / f64::INFINITY can become Value::Null during conversion, and validate_numeric() won’t panic (it only checks as_f64()). Also, inputs like "5" or true serialize into the operator JSON today.
Proposed fix (tighten numeric + non-zero validation, dedupe logic)
fn validate_numeric(value: &Value, param_name: &str) {
- if let Some(num) = value.as_f64() {
- if num.is_nan() || num.is_infinite() {
- panic!("{} cannot be NaN or Infinity", param_name);
- }
- }
+ // Reject non-numbers (also catches NaN/Infinity that may have converted to null).
+ let num = value
+ .as_f64()
+ .unwrap_or_else(|| panic!("{param_name} must be a finite number"));
+ if !num.is_finite() {
+ panic!("{param_name} cannot be NaN or Infinity");
+ }
}
+
+fn validate_nonzero(value: &Value, param_name: &str) {
+ validate_numeric(value, param_name);
+ if value.as_f64().unwrap() == 0.0 {
+ panic!("{param_name} cannot be zero");
+ }
+}
pub fn divide<T: Into<Value>>(divisor: T) -> String {
let val = divisor.into();
- validate_numeric(&val, "divisor");
-
- if let Some(num) = val.as_f64() {
- if num == 0.0 {
- panic!("divisor cannot be zero");
- }
- } else if let Some(num) = val.as_i64() {
- if num == 0 {
- panic!("divisor cannot be zero");
- }
- }
+ validate_nonzero(&val, "divisor");
parse_operator("divide", vec![val])
}
pub fn divide_with_min<T: Into<Value>, M: Into<Value>>(divisor: T, min: M) -> String {
let val = divisor.into();
let min_val = min.into();
- validate_numeric(&val, "divisor");
+ validate_nonzero(&val, "divisor");
validate_numeric(&min_val, "min");
-
- if let Some(num) = val.as_f64() {
- if num == 0.0 {
- panic!("divisor cannot be zero");
- }
- } else if let Some(num) = val.as_i64() {
- if num == 0 {
- panic!("divisor cannot be zero");
- }
- }
parse_operator("divide", vec![val, min_val])
}
pub fn modulo<T: Into<Value>>(divisor: T) -> String {
let val = divisor.into();
- validate_numeric(&val, "divisor");
-
- if let Some(num) = val.as_f64() {
- if num == 0.0 {
- panic!("divisor cannot be zero");
- }
- } else if let Some(num) = val.as_i64() {
- if num == 0 {
- panic!("divisor cannot be zero");
- }
- }
+ validate_nonzero(&val, "divisor");
parse_operator("modulo", vec![val])
}🤖 Prompt for AI Agents
In @templates/rust/src/operator.rs.twig around lines 47 - 156, validate_numeric
currently misses Value::Null and non-numeric JSON (strings/booleans) and
therefore lets NaN/Infinity or non-numbers through; update validate_numeric to
first assert value.is_number() (panic with param_name if false), then use
as_f64() to check for is_nan() or is_infinite(); extract the repeated "divisor
!= 0" check into a small helper (e.g., validate_nonzero_divisor) that accepts
&Value and a name and checks number-ness then tests as_f64()/as_i64() for zero,
and call that helper from divide, divide_with_min and modulo; ensure all numeric
operator builders (increment_by, increment_with_max, decrement_by,
decrement_with_min, multiply, multiply_with_max, etc.) call the tightened
validate_numeric so non-numeric Value::Null or strings/booleans are rejected
consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @templates/rust/tests/tests.rs:
- Line 131: Remove the misleading standalone println!("Invalid endpoint URL:
htp://cloud.appwrite.io/v1") in templates/rust/tests/tests.rs or replace it with
a real test: create a test that constructs the client/request using the invalid
URL and asserts the operation returns an Err (e.g., assert!(result.is_err()) or
matches the expected error), ensuring the test exercises the URL validation path
rather than only printing a message.
🧹 Nitpick comments (5)
templates/rust/tests/tests.rs (5)
98-101: Clarify the intent of duplicate upload calls.The same upload operations are called twice consecutively. If this is intentional (e.g., testing idempotency), consider adding a comment to clarify. Otherwise, these appear to be unintentional duplicates.
107-129: Error test cases don't fail when errors are not returned.The error tests for 400, 500, and 502 responses expect errors and handle them correctly. However, the
Ok(_) => {}branches mean if these endpoints unexpectedly succeed, the test passes silently without indicating the unexpected behavior.Consider adding assertions or print statements in the
Okbranch to flag unexpected success:Proposed improvement
match general.error400().await { - Ok(_) => {}, + Ok(_) => eprintln!("Expected error400 to fail, but it succeeded"), Err(e) => { println!("{}", e.message); println!("{}", e.response); }, }
133-133: Explicitly ignoring result may hide errors.The
let _ = general.empty().await;statement explicitly discards the result, potentially hiding errors. Ifempty()is expected to succeed, consider using?to propagate errors. If it's expected to be a no-op regardless of outcome, add a comment explaining why the result is ignored.
153-201: Hardcoded absolute paths reduce portability.Lines 155 and 188 use hardcoded absolute paths (
/app/tests/resources/file.pngand/app/tests/resources/large_file.mp4) that assume a specific directory structure (likely Docker container layout). This reduces portability and may cause tests to fail in different environments.Consider:
- Making paths configurable via environment variables
- Using relative paths from the project root
- Using
env!("CARGO_MANIFEST_DIR")to construct paths relative to the crate rootExample using CARGO_MANIFEST_DIR
use std::env; let project_root = env!("CARGO_MANIFEST_DIR"); let upload_file = Path::new(project_root).join("tests/resources/file.png");
238-245: Duplicate query tests with identical parameters.Lines 238-245 contain what appear to be duplicate test cases:
- Lines 238-239: Both test
distance_equalwith the same parameters- Lines 240-241: Both test
distance_not_equalwith the same parameters- Lines 242-243: Both test
distance_greater_thanwith the same parameters- Lines 244-245: Both test
distance_less_thanwith the same parametersIf these duplicates are intentional (e.g., testing different internal representations), consider adding comments to clarify. Otherwise, remove the redundant calls.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/rust/tests/tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: swift (server)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Rust183)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, FlutterBeta)
🔇 Additional comments (1)
templates/rust/tests/tests.rs (1)
6-22: LGTM!The test harness setup is well-structured with proper async runtime configuration and client initialization for mock testing.
| async fn test_foo_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | ||
| let foo = Foo::new(&client); | ||
|
|
||
| // Foo Service | ||
| match foo.get("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("foo.get => error {}", e), | ||
| } | ||
|
|
||
| match foo.post("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("foo.post => error {}", e), | ||
| } | ||
|
|
||
| match foo.put("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("foo.put => error {}", e), | ||
| } | ||
|
|
||
| match foo.patch("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("foo.patch => error {}", e), | ||
| } | ||
|
|
||
| match foo.delete("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("foo.delete => error {}", e), | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn test_bar_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | ||
| let bar = Bar::new(&client); | ||
|
|
||
| // Bar Service | ||
| match bar.get("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("bar.get => error {}", e), | ||
| } | ||
|
|
||
| match bar.post("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("bar.post => error {}", e), | ||
| } | ||
|
|
||
| match bar.put("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("bar.put => error {}", e), | ||
| } | ||
|
|
||
| match bar.patch("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("bar.patch => error {}", e), | ||
| } | ||
|
|
||
| match bar.delete("string", 123, string_in_array).await { | ||
| Ok(response) => println!("{}", response.result), | ||
| Err(e) => eprintln!("bar.delete => error {}", e), | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Test functions don't propagate failures, making automated testing ineffective.
Both test_foo_service and test_bar_service catch errors with eprintln! but always return Ok(()). This means:
- Failed API calls won't cause the test to fail
- The test suite will report success even when operations fail
- Manual log inspection is required to detect issues
Additionally, there are no assertions validating that responses contain expected values.
Consider either:
- Propagating errors with
?operator for operations that must succeed - Using assertions (e.g.,
assert!,assert_eq!) to validate responses - Returning errors for unexpected failures:
Err(e.into())
Example fix for test_foo_service
async fn test_foo_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> {
let foo = Foo::new(&client);
- // Foo Service
- match foo.get("string", 123, string_in_array).await {
- Ok(response) => println!("{}", response.result),
- Err(e) => eprintln!("foo.get => error {}", e),
- }
+ let response = foo.get("string", 123, string_in_array).await?;
+ println!("{}", response.result);
+ // Add assertions as needed:
+ // assert_eq!(response.result, expected_value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn test_foo_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | |
| let foo = Foo::new(&client); | |
| // Foo Service | |
| match foo.get("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.get => error {}", e), | |
| } | |
| match foo.post("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.post => error {}", e), | |
| } | |
| match foo.put("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.put => error {}", e), | |
| } | |
| match foo.patch("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.patch => error {}", e), | |
| } | |
| match foo.delete("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.delete => error {}", e), | |
| } | |
| Ok(()) | |
| } | |
| async fn test_bar_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | |
| let bar = Bar::new(&client); | |
| // Bar Service | |
| match bar.get("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.get => error {}", e), | |
| } | |
| match bar.post("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.post => error {}", e), | |
| } | |
| match bar.put("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.put => error {}", e), | |
| } | |
| match bar.patch("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.patch => error {}", e), | |
| } | |
| match bar.delete("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.delete => error {}", e), | |
| } | |
| Ok(()) | |
| } | |
| async fn test_foo_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | |
| let foo = Foo::new(&client); | |
| let response = foo.get("string", 123, string_in_array).await?; | |
| println!("{}", response.result); | |
| // Add assertions as needed: | |
| // assert_eq!(response.result, expected_value); | |
| match foo.post("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.post => error {}", e), | |
| } | |
| match foo.put("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.put => error {}", e), | |
| } | |
| match foo.patch("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.patch => error {}", e), | |
| } | |
| match foo.delete("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("foo.delete => error {}", e), | |
| } | |
| Ok(()) | |
| } | |
| async fn test_bar_service(client: &Client, string_in_array: &[String]) -> Result<(), Box<dyn std::error::Error>> { | |
| let bar = Bar::new(&client); | |
| // Bar Service | |
| match bar.get("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.get => error {}", e), | |
| } | |
| match bar.post("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.post => error {}", e), | |
| } | |
| match bar.put("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.put => error {}", e), | |
| } | |
| match bar.patch("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.patch => error {}", e), | |
| } | |
| match bar.delete("string", 123, string_in_array).await { | |
| Ok(response) => println!("{}", response.result), | |
| Err(e) => eprintln!("bar.delete => error {}", e), | |
| } | |
| Ok(()) | |
| } |
What does this PR do?
Adds Rust SDK support 🦀
Some notable arch related information -
Uses
arc-swapto maintain proper mutability of Client like how other SDKs manage. Its usually auto-handled for others but this needs to be explicitly managed. Usecase is not frequent but always a possibility where you want to change something on a client, add headers, etc. This allows us to redefine client and the service with new instances. Its not cheap but definitely not expensive as well since this is not a normal thing to change/add/update headers or client itself too frequently.Uses
tokio, the obvious choice andreqwestfor multipart.Test Plan
Manual, E2Es.
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.