Dependency on regress should be configurable

[thomasmost]

Settings should include either a with_regress_converters or a without_regress_converters setter so that a dependency on regress is not guaranteed.

Perhaps a smaller version of this could be configuring the tool to generate struct definitions alone, without any converters.

[ahl]

That's an interesting idea. Is the objection to regress that its a large dependency? What would you propose we do to validate string formats if we didn't have that dependency? In stand-alone, generated crates, I believe we only include the regress dependency when it's actually needed.

[thomasmost]

Yes, it's a large dependency and I think there's value to generating bindings that can be used without making assumptions about validation strategy.

That said this is just one man's request for a more custom-izable library :) overall the utility is still fantastic!

[thomasmost]

Oh, here's a question, if I don't set with_struct_builder(true)); — does that eliminate the regress dependency?

Update

Looks like no haha

[jct-tympanon]

if there's going to be work in this area -- would you also consider caching parsed regexes (or is there another issue tracking that)? i see the current behavior is to parse validation regexes every time they are used? this can be pretty expensive for long/complex regexes. seems like the work to cache the parsed regex could fit neatly inside the work to abstract the regexes out.

[jct-tympanon]

a little follow-up on this: i benchmarked my own application, using types generated from the AWS xray schema. that schema makes extensive use of regex validations, including some long expressions (~1k chars long). in my case, about 20% of CPU use for the whole application is re-parsing those long regexes for every new instance.

i'm going to work on improving that for my own needs. if I can make some progress, would you consider looking at a PR or two? the first thing I'm going to do is add some benchmarks to measure progress.

[ahl]

I would love to see some functionality in a regex parser that allowed for static definition of a regex: we could parse the regex during code generation and emit code that corresponds to the parsed form.

[jct-tympanon]

I would love to see some functionality in a regex parser that allowed for static definition of a regex: we could parse the regex during code generation and emit code that corresponds to the parsed form.

there's a pretty interesting related discussion on the regex crate but it sounds like a long stretch. i also guess typify is strongly bound to regress because we specifically need ES regex, right? Though I admit I'm not sure where other rust regex parsers might diverge from that.

i guess the straightforward alternative is to use lazy_static or std LazyLock and pull the regexes into module-scoped static variables. that could be feature-gated if there's concern about backwards compatibility (something like feature = "static-regex").

a more invasive approach is to separate validation from deserialization, and have typify produce Validator types which contain the regexes as fields. the application is then in control of the lifetime of those. that was what i was thinking of at first,

// 'validator' lifetime could be 'static or scoped to some large parse operation, suitable to the application.
fn parse_a_type(json: &str, validator: &MyTypeValidator) -> Result<MyType> {
   let parsed = json::parse()?;
   // the validator type could also have a convenience api like "parse_and_check" which does the serde step too.
   validator.check(parsed)
}

but i think it's a bit counter-intuitive; a lot of validation must be performed during deserialization (required fields), and it wouldn't be obvious whether numeric limits would be applied in deserialization or by the validator.

[jct-tympanon]

i did a commit on my fork that moved the regex into a LazyStatic<Regex>. I did two benchmarks: "Long regex", which is my usecase, and "short regex", which was taken from one of typify's existing test cases. The impact was very large for both (details below).

I hope you'll consider a PR for this? The cost of parsing the regex is very large compared to the cost of testing it.

baseline benchmark (before change):

long regex              time:   [57.224 µs 57.363 µs 57.495 µs]
short regex             time:   [5.0025 µs 5.0069 µs 5.0115 µs]

patched benchmark:

long regex              time:   [296.48 ns 296.68 ns 296.90 ns]
                        change: [-99.483% -99.482% -99.481%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

short regex             time:   [112.14 ns 112.21 ns 112.27 ns]
                        change: [-97.764% -97.761% -97.759%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 25 outliers among 100 measurements (25.00%)
  13 (13.00%) low severe
  5 (5.00%) low mild
  2 (2.00%) high mild
  5 (5.00%) high severe

here's an example of the generated code from one of the tests:

static ID_OR_YOLO_YOLO_PATTERN: std::sync::LazyLock<regress::Regex> =
    std::sync::LazyLock::new(|| regress::Regex::new(".*").unwrap());
impl ::std::str::FromStr for IdOrYoloYolo {
    type Err = self::error::ConversionError;
    fn from_str(value: &str) -> ::std::result::Result<Self, self::error::ConversionError> {
        if (&*ID_OR_YOLO_YOLO_PATTERN).find(value).is_none() {
            return Err("doesn't match pattern \".*\"".into());
        }
        Ok(Self(value.to_string()))
    }
}

[ahl]

Looks like a nice, straightforward improvement. I would suggest you move the static into the body of the from_str to reduce name conflicts. A PR would be great.