Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat extra extractors #102

Closed
wants to merge 7 commits into from
Closed

Feat extra extractors #102

wants to merge 7 commits into from

Conversation

y-haidar
Copy link

This is not done, but would like to request feedback on this approach.

Also, I got a question; the axum::Json::from_request already does deserialize so the code below would never return an Err(in theory?).

match serde_path_to_error::deserialize(value) {
Ok(v) => Ok(Json(v)),
Err(error) => Err(JsonSchemaRejection::Serde(error)),
}

Maybe do a trace::error and do something else? Not sure what, but it feel like the error shouldn't be exposed to crate users.

@y-haidar
Copy link
Author

hmmm do I need master branch? Sorry, didn't notice the workflow; I am new to this. I'll make a new PR when ready, but I will keep this up to get your feedback.

@tamasfe
Copy link
Owner

tamasfe commented Jan 11, 2024

hmmm do I need master branch? Sorry, didn't notice the workflow; I am new to this. I'll make a new PR when ready

No need, this looks good to me, thank you.

I currently have very tight deadlines at work, I'll do a review on the weekened, but please feel free to ping me in case I forget.

@y-haidar
Copy link
Author

Sorry I know this is long, you can read this later.

... I'm not sure how common it is to have JSON there, but the error format definitely beats serde's.

I miss understood your comment. Indeed, while testing my work, I was getting schema validation error ... ,"error":"\"true\" is not of type \"boolean\"" ....

The problem is:

// Incorrect, Value is: `String("024fe6c1-a33e-45a5-8157-2e48f5ff6da9")`
let value: Value = axum::extract::Path::from_request_parts(parts, state)
   ...
// Correct, but jsonschema doesn't support validating `T` only `serde_json::Value`
let value: T = axum::extract::Path::from_request_parts(parts, state)
  ...

The RawPathParams that I started with, in a75d258, gave me wrong impressions by working with uuid(as it kept the key), but ofc it will not work with a bool value. Quick fix is to require Serialize, serialize T from axum::extract::Path::from_request_parts, then validate, then return the same T. Is this okay? Maybe worth opening an issue on jsonschema-rs?

@y-haidar
Copy link
Author

y-haidar commented Jan 12, 2024

Closing PR. Using validator along with schemars can achieve what I originally wanted. I was hoping jsonschema to do this all by itself to reduce dependencies that do similar things. Below is what I am satisfied with:

const REGEX_STR: &'static str = "[A-Z][a-z]+";

lazy_static::lazy_static! {
  static ref RE_TWO_CHARS: Regex = Regex::new(REGEX_STR).unwrap();
}

#[derive(Deserialize, JsonSchema, Validate)]
struct TestPath {
  #[validate(length(equal = 3))]
  input1: String,
  #[validate(regex = "RE_TWO_CHARS")]
  input2: String,
  input3: bool,
}

Sorry for the time wasted. Also noticed axum-valid, which supports aide.

@y-haidar y-haidar closed this Jan 12, 2024
@Wicpar
Copy link
Collaborator

Wicpar commented Jan 12, 2024

You also have garde which can validate regex from strings and other safer features like explicit opt-out from validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants