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

validating ktor (or other HTTP) form/query/path parameters #58

Open
2x2xplz opened this issue Jan 6, 2025 · 1 comment
Open

validating ktor (or other HTTP) form/query/path parameters #58

2x2xplz opened this issue Jan 6, 2025 · 1 comment

Comments

@2x2xplz
Copy link

2x2xplz commented Jan 6, 2025

Very cool project, thank you. Given the integration with ktor, are you planning any methods to validate incoming form, query or path parameters? This library assumes the object can be constructed, then performs validation checks (field is greater than zero, string matches a regex pattern, etc). But due to bugs, malicious actors, and incorrect input, lots of times the supplied data is insufficient to instantiate an object instance, and attempting to do so throws an error before we can start any validation.

For my current project I wrote some custom validators (see example at bottom) that walk through all of the possible input parameters and individually check them all, but this has a few problems:

  • only way to instantiate this is via a manual function call (not with @Validate decorator),
  • the collection constraints like .isContaining don't work on ktor's Parameters - almost all constraints are custom
  • all params are strings, so numeric comparisons require conversion first
  • regarding above, validation throws an error (instead of ConstraintViolation) when parameter is not the correct type ("eleven" not "11", bad date format, etc) unless you specifically test for that (and must "fail fast" if you also plan to validate the resulting value)
  • building a validator for the object itself is duplicitous, since the input parameters have already been tested
  • validating optional params is burdensome - have to check if it exists, after that all references require !!
  • essentially, the validation result is just a boolean value (Success/Failure), not the resulting object (to call methods on, pass to another object, etc)
  • hard to say if this is any easier/safer than just using a bunch of require {} in the object init

I haven't thought this through entirely, but maybe some kind of workflow where the validator would attempt to create an object from the submitted params, if it succeeds, then we perform the normal object validation (via @Validate), but if the object creation fails then we redirect to an alternate set of constraints on the individual parameters to help determine why the instantiation failed (param was missing, wrong type, unparsable, etc)

Another idea would be to have constraints that attempt to coerce a string value to another type (LocalDate, URI, etc). I think this is preferable to Regex matching due to different date patterns in different locales (same with phone numbers, postal codes, etc), and the complexity of a regex validators for URIs, emails, etc. Also works for enums.

Again, thanks for a cool library, looking forward to seeing it grow.

example custom validator:

// validate incoming query params for / route  (allows user to bookmark report with certain defaults)
val validateOrdersQueryParams = Validator<Parameters>(akkurateConfig) {
    val params : Parameters = this.unwrap()
    if (params.contains("start_date")) this.constrain { noError { LocalDate.parse(it["start_date"]!!) } } otherwise { "start_date can't be parsed" }
    if (params.contains("end_date")) this.constrain { noError { LocalDate.parse(it["end_date"]!!) } } otherwise { "end_date can't be parsed" }
    if (params.contains("start_date") && params.contains("end_date")) this.constrain {
        !(LocalDate.parse(it["start_date"]!!).isAfter(LocalDate.parse(it["end_date"]!!)))
    } otherwise { "start_date is after end_date" }
    if (params.contains("sales_min")) {
        this.constrain { noError { it["sales_min"]!!.toInt() } } otherwise { "sales_min is non-numeric" }
        this.constrain { it["sales_min"]!!.toInt() >= 0 } otherwise { "sales_min is out of range" }
    }
}

// attempts to run a function and returns true/false whether it completes without error
fun noError(testFunction: () -> Any) : Boolean {
    val result: Result<Boolean> =
        try {
            testFunction()
            Result.success(true)
        } catch (e: Exception) {
            Result.failure(e)
        }

    return when {
        result.isSuccess -> true
        result.isFailure -> false
        else -> false
    }
}
@nesk
Copy link
Owner

nesk commented Jan 10, 2025

Wow, thank you for the details, this is really insightful!

There are a lot of good remarks here. While I can't promise you anything, here are some thoughts about a lot of things you said.

the collection constraints like .isContaining don't work on ktor's Parameters - almost all constraints are custom

I've just had a look at the API reference, Parameters implements the StringValues interface. I could provide some helper for this.

all params are strings, so numeric comparisons require conversion first

building a validator for the object itself is duplicitous, since the input parameters have already been tested

The initial goal of Akkurate was to focus on validation, not on parsing or casting. However, I can understand there is a need, I'll think about it.

validating optional params is burdensome - have to check if it exists, after that all references require !!

This is something I've already invested a lot of thoughts, it's related to #21

regarding above, validation throws an error (instead of ConstraintViolation) when parameter is not the correct type ("eleven" not "11", bad date format, etc) unless you specifically test for that (and must "fail fast" if you also plan to validate the resulting value)

I've never thought about a constrain to check if anything threw, I'll add this someday, this is clearly a missing feature!

Let's keep the issue as-is for the moment, I'll think more about this and, eventually, create new related issues to fix some of your feedbacks.

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

No branches or pull requests

2 participants