Add unified validation system with async support #56
Conversation
Introduces a unified Validatable trait to support both legacy validator and new v2 validation engines, including async validation. Adds AsyncValidatedJson extractor for async validation scenarios, updates extractors and prelude, and provides conversion helpers for error normalization. Updates documentation and adds comprehensive tests for sync and async validation flows.
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified validation system to RustAPI that aims to support both the legacy validator crate and a new V2 validation engine with async capabilities. The changes add a Validatable trait for uniform validation access, an AsyncValidatedJson extractor for async validation scenarios, and comprehensive documentation with test coverage. The PR also includes a significant version bump from 0.1.15 to 0.1.188.
Changes:
- Added unified
Validatabletrait with blanket implementation forvalidator::Validatetypes - Implemented
AsyncValidatedJsonextractor for async validation with v2 engine - Enhanced
#[derive(Validate)]macro to generateValidatableimplementations - Added path parameter auto-detection in route handler macros
- Updated documentation with examples for both sync and async validation
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Version bump to 0.1.188 across all workspace crates |
Cargo.lock |
Lockfile updates reflecting version changes |
crates/rustapi-core/src/validation.rs |
New validation module with Validatable trait and error conversion helpers |
crates/rustapi-core/src/extract.rs |
Updated ValidatedJson to use Validatable trait, added new AsyncValidatedJson extractor |
crates/rustapi-core/src/lib.rs |
Export new validation module |
crates/rustapi-core/Cargo.toml |
Added validator dependency |
crates/rustapi-macros/src/lib.rs |
Enhanced derive_validate to generate Validatable impl, added path param auto-detection |
crates/rustapi-rs/src/lib.rs |
Added validation types to prelude exports |
crates/rustapi-rs/Cargo.toml |
Added rustapi-validate and async-trait dependencies |
crates/rustapi-rs/tests/validation_tests.rs |
New test file covering legacy validator, v2 sync, and async validation |
docs/cookbook/src/crates/rustapi_validation.md |
Comprehensive rewrite documenting unified validation system |
| // Blanket implementation for types implementing the external validator::Validate trait | ||
| impl<T: validator::Validate> Validatable for T { | ||
| fn do_validate(&self) -> Result<(), ApiError> { | ||
| match validator::Validate::validate(self) { | ||
| Ok(_) => Ok(()), | ||
| Err(e) => Err(convert_validator_errors(e)), | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This blanket implementation creates a trait implementation conflict. When a type implements validator::Validate, it automatically gets a Validatable implementation through this blanket impl. However, the #[derive(rustapi_macros::Validate)] macro also generates a Validatable implementation (see rustapi-macros/src/lib.rs:1445). This means any type that implements both validator::Validate AND uses #[derive(rustapi_macros::Validate)] will have two conflicting implementations of Validatable, causing a compiler error.
The solution is to remove the blanket implementation and instead implement Validatable only through the macro, or use a different trait hierarchy. Alternatively, you could make the macro check if validator::Validate is already implemented and skip the Validatable impl generation in that case.
| // Blanket implementation for types implementing the external validator::Validate trait | |
| impl<T: validator::Validate> Validatable for T { | |
| fn do_validate(&self) -> Result<(), ApiError> { | |
| match validator::Validate::validate(self) { | |
| Ok(_) => Ok(()), | |
| Err(e) => Err(convert_validator_errors(e)), | |
| } | |
| } | |
| } |
Cargo.toml
Outdated
|
|
||
| [workspace.package] | ||
| version = "0.1.15" | ||
| version = "0.1.188" |
There was a problem hiding this comment.
The version jump from 0.1.15 to 0.1.188 appears to be an unusual version increment. This is a jump of 173 minor versions, which doesn't follow typical semantic versioning practices. This could indicate:
- A typo (perhaps meant to be 0.1.18 or 0.1.16)
- An incorrect merge from a different branch
- An automated version numbering issue
Please verify this is the intended version number. If this is a typo, it should be corrected to maintain proper version history.
| ## The Unified Validation System | ||
|
|
||
| First, define your rules using attributes on your struct. | ||
| RustAPI (v0.1.15+) introduces a unified validation system that supports: |
There was a problem hiding this comment.
The documentation references "v0.1.15+" but the PR updates the version to "0.1.188". This creates an inconsistency where the documentation suggests the feature was introduced in v0.1.15, but the actual code is being released as v0.1.188.
If 0.1.188 is the correct version, this should be updated to say "v0.1.188+" to accurately reflect when this feature is available. If 0.1.188 is a typo, then the version in Cargo.toml should be fixed instead.
| // Convert v2 ValidationErrors to ApiError | ||
| let field_errors: Vec<crate::error::FieldError> = errors | ||
| .fields | ||
| .iter() | ||
| .flat_map(|(field, errs)| { | ||
| let field_name = field.to_string(); | ||
| errs.iter().map(move |e| crate::error::FieldError { | ||
| field: field_name.clone(), | ||
| code: e.code.to_string(), | ||
| message: e.message.clone(), | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| return Err(ApiError::validation(field_errors)); |
There was a problem hiding this comment.
The error conversion logic here is duplicated from the convert_v2_errors function in crates/rustapi-core/src/validation.rs (lines 43-57). This violates the DRY (Don't Repeat Yourself) principle and creates a maintenance burden.
Instead of duplicating this conversion logic, this code should use the existing convert_v2_errors helper function from the validation module, like so:
if let Err(errors) = value.validate_full(&ctx).await {
return Err(crate::validation::convert_v2_errors(errors));
}This would ensure consistency in error format and make maintenance easier.
| // Convert v2 ValidationErrors to ApiError | |
| let field_errors: Vec<crate::error::FieldError> = errors | |
| .fields | |
| .iter() | |
| .flat_map(|(field, errs)| { | |
| let field_name = field.to_string(); | |
| errs.iter().map(move |e| crate::error::FieldError { | |
| field: field_name.clone(), | |
| code: e.code.to_string(), | |
| message: e.message.clone(), | |
| }) | |
| }) | |
| .collect(); | |
| return Err(ApiError::validation(field_errors)); | |
| return Err(crate::validation::convert_v2_errors(errors)); |
| // Built-in async rule (requires database integration) | ||
| #[validate(async_unique(table = "users", column = "email"))] | ||
| pub email: String, |
There was a problem hiding this comment.
The documentation mentions a built-in async_unique rule at line 66, but there's no evidence in the PR that this feature is actually implemented. This could mislead users into thinking they can use #[validate(async_unique(table = "users", column = "email"))] when this functionality may not exist yet.
Either:
- Remove this example if the feature is not implemented
- Clearly mark it as a future feature or example of what's possible
- Ensure the async_unique rule is actually implemented in the rustapi-validate crate
The same applies to the mention of "requires database integration" - if this isn't implemented, it should be documented as a limitation or future feature.
|
|
||
| ```rust | ||
| use rustapi_macros::Validate; | ||
| use rustapi_validate::v2::{ValidationContext, RuleError}; |
There was a problem hiding this comment.
The code example is missing the use serde::Deserialize; import statement. The struct on line 63 derives Deserialize but the import is not shown, which will cause compilation errors if users copy this example.
Add the missing import:
use rustapi_macros::Validate;
use rustapi_validate::v2::{ValidationContext, RuleError};
use serde::Deserialize; // Add this line| use rustapi_validate::v2::{ValidationContext, RuleError}; | |
| use rustapi_validate::v2::{ValidationContext, RuleError}; | |
| use serde::Deserialize; |
crates/rustapi-macros/src/lib.rs
Outdated
| impl #impl_generics ::rustapi_rs::validation::Validatable for #name #ty_generics #where_clause { | ||
| fn do_validate(&self) -> Result<(), ::rustapi_rs::ApiError> { | ||
| match ::rustapi_validate::v2::Validate::validate(self) { | ||
| Ok(_) => Ok(()), | ||
| Err(e) => Err(::rustapi_rs::validation::convert_v2_errors(e)), | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
This implementation references ::rustapi_rs::validation::Validatable, but the Validatable trait is actually defined in rustapi-core, not rustapi-rs. The macro is in the rustapi-macros crate which depends on rustapi-rs, and while rustapi-rs re-exports types from rustapi-core, this creates a circular reference path.
The generated code should use ::rustapi_core::validation::Validatable instead, or ensure the path resolution works correctly through the workspace structure. This may cause compilation issues depending on how users import the macro.
| // Auto-detect path parameters from function arguments | ||
| let auto_params = collect_path_params(&input); | ||
|
|
||
| // Extract metadata from attributes to chain builder methods | ||
| let mut chained_calls = quote!(); | ||
|
|
||
| // Add auto-detected parameters first (can be overridden by attributes) | ||
| for (name, schema) in auto_params { | ||
| chained_calls = quote! { #chained_calls .param(#name, #schema) }; |
There was a problem hiding this comment.
The path parameter auto-detection feature is being added without any documentation, tests, or clear indication of whether this is a new feature or a fix. The comment on line 469 states "can be overridden by attributes", but this auto-detection behavior may be surprising to users who didn't explicitly request parameter type specification.
Additionally, this changes the behavior of route handlers by automatically adding parameter metadata based on function signature inspection. This could potentially:
- Override user-specified parameter configurations if the order matters
- Add unexpected parameters if the heuristics are wrong
- Break existing code that relies on manual parameter specification
Consider either:
- Documenting this new auto-detection feature in the PR description and documentation
- Adding tests to verify the auto-detection works correctly
- Making it opt-in rather than automatic
- Clarifying whether this is intentional new functionality or if it was accidentally included in this PR
| // Auto-detect path parameters from function arguments | |
| let auto_params = collect_path_params(&input); | |
| // Extract metadata from attributes to chain builder methods | |
| let mut chained_calls = quote!(); | |
| // Add auto-detected parameters first (can be overridden by attributes) | |
| for (name, schema) in auto_params { | |
| chained_calls = quote! { #chained_calls .param(#name, #schema) }; | |
| // Extract metadata from attributes to chain builder methods | |
| let mut chained_calls = quote!(); | |
| // Detect whether the user has explicitly specified any `#[param]` attributes. | |
| // If so, we skip auto-detection to avoid surprising overrides or duplicates. | |
| let has_explicit_params = fn_attrs.iter().any(|attr| { | |
| attr.path() | |
| .segments | |
| .last() | |
| .map(|s| s.ident == "param") | |
| .unwrap_or(false) | |
| }); | |
| // Auto-detect path parameters from function arguments only when no explicit `#[param]` | |
| // attributes are present. This keeps manual configuration authoritative. | |
| if !has_explicit_params { | |
| let auto_params = collect_path_params(&input); | |
| for (name, schema) in auto_params { | |
| chained_calls = quote! { #chained_calls .param(#name, #schema) }; | |
| } |
| // Create validation context from request | ||
| // TODO: Extract validators from App State | ||
| let ctx = ValidationContext::default(); |
There was a problem hiding this comment.
The code contains a TODO comment about extracting validators from App State. This indicates incomplete functionality. The ValidationContext::default() is used, which may not have access to necessary resources like database connections that are required for async validation rules (e.g., uniqueness checks).
This could cause async validators to fail silently or not work as expected in production. Consider either:
- Implementing the App State integration before merging
- Documenting this limitation clearly in the documentation
- Making it clear that users must provide their own validation context setup
| ## Backward Compatibility | ||
|
|
||
| The system is fully backward compatible. You can continue using `validator::Validate` on your structs, and `ValidatedJson` will accept them automatically via the unified `Validatable` trait. | ||
|
|
||
| ```rust | ||
| // Legacy code still works! | ||
| #[derive(validator::Validate)] | ||
| struct OldStruct { ... } | ||
|
|
||
| async fn handler(ValidatedJson(body): ValidatedJson<OldStruct>) { ... } | ||
| ``` |
There was a problem hiding this comment.
The documentation claims "The system is fully backward compatible" but this is not entirely accurate due to the blanket implementation conflict identified in crates/rustapi-core/src/validation.rs (lines 13-20).
If a struct were to derive both validator::Validate and rustapi_macros::Validate, it would have conflicting implementations of the Validatable trait, causing a compilation error. This breaks the backward compatibility promise.
The documentation should either:
- Clarify that types should use EITHER validator::Validate OR rustapi_macros::Validate, but not both
- Be updated once the implementation conflict is resolved
Updated the macro in rustapi-macros to implement Validatable for ::rustapi_core instead of ::rustapi_rs, ensuring compatibility for crates that depend on rustapi-core directly. Added rustapi-core as a dependency in rustapi-validate.
Updated the version numbers for the workspace and all related crates from 0.1.188 to 0.1.191 in Cargo.toml and Cargo.lock. No other functional changes were made.
Add unified validation system with async support a4b1a7b
This pull request introduces a unified validation system to the RustAPI framework, making it easier to perform both synchronous and asynchronous validation on extracted request data. It adds a new
Validatabletrait to abstract over different validation engines, introduces anAsyncValidatedJsonextractor for async validation use-cases, and improves OpenAPI parameter inference in the macros. The changes also ensure that validation errors are consistently converted into API errors for response handling.Validation System Improvements:
Added a new
Validatabletrait inrustapi-coreto provide a unified interface for synchronous validation, supporting both external (validator) and internal (rustapi_validate::v2) engines. Blanket implementations and error conversion helpers are included. (crates/rustapi-core/src/validation.rs,crates/rustapi-core/Cargo.toml,crates/rustapi-core/src/lib.rs,crates/rustapi-rs/src/lib.rs) [1] [2] [3] [4] [5] [6] [7]Updated the
ValidatedJsonextractor to use the newValidatabletrait, allowing seamless switching between validation engines. (crates/rustapi-core/src/extract.rs) [1] [2]Introduced a new
AsyncValidatedJsonextractor for request bodies that require asynchronous validation (e.g., database checks), leveraging theAsyncValidatetrait fromrustapi_validate::v2. This includes error conversion and documentation. (crates/rustapi-core/src/extract.rs,crates/rustapi-core/src/lib.rs,crates/rustapi-rs/src/lib.rs) [1] [2] [3]Macro and OpenAPI Enhancements:
Added automatic detection of path parameters and their schema types in route handler macros, improving OpenAPI documentation generation and reducing manual annotation. (
crates/rustapi-macros/src/lib.rs) [1] [2]The
derive(Validate)macro now also generates aValidatableimplementation for types, ensuring compatibility with the new validation system. (crates/rustapi-macros/src/lib.rs)Dependency and Version Updates:
0.1.188and added new dependencies (validator,rustapi-validate,async-trait,uuid) to the workspace andrustapi-rsfor validation support. (Cargo.toml,crates/rustapi-core/Cargo.toml,crates/rustapi-rs/Cargo.toml) [1] [2] [3] [4]These changes make validation more flexible, robust, and user-friendly in the RustAPI ecosystem.