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

String regex-pattern flags are discarded during conversion #116

Open
Spappz opened this issue Apr 25, 2024 · 3 comments · May be fixed by #122
Open

String regex-pattern flags are discarded during conversion #116

Spappz opened this issue Apr 25, 2024 · 3 comments · May be fixed by #122
Labels
help wanted Extra attention is needed

Comments

@Spappz
Copy link

Spappz commented Apr 25, 2024

Minimal reproducible example

You can cut things short once jsonSchema is defined in all honesty—just inspect the pattern. I'm using CommonJS so you can try it out in the Node REPL.

const { z } = require("zod");
const { zodToJsonSchema } = require("zod-to-json-schema");
const Ajv = require("ajv");

const zodSchema = z.object({
  foo: z.string().regex(/bar/i),
});
const jsonSchema = zodToJsonSchema(zodSchema);
const ajvValidator = new Ajv().compile(jsonSchema);

console.log(zodSchema.safeParse({foo: "barbaz"})); // { success: true }
console.log(ajvValidator({foo: "barbaz"}); // true
console.log(zodSchema.safeParse({foo: "BARbaz"})); // { success: true }
console.log(ajvValidator({foo: "BARbaz"}); // false

The problem

The error seems to have two origins. Firstly, and most basically, addPattern just ignores RegExp flags, and only takes as value the RegExp source string.

Secondly, however, is that JSON Schema just doesn't seem to have any defined way to actually reproduce JS RegExp flags.

The solution

I'm not sure if there's a nice, trivial solution to this. A possible avenue, at least for case-insensitivity (in which I'm naturally most interested), is to 'correct' the regex in transit. You'd want to produce transformations like this, probably necessitating some horrid hackery in the process:

/foo/i => "[fF][oO][oO]"
/fooba[rz]/i => "[fF][oO][oO][bB][aA][rRzZ]"
/\\\[ foo [a-z\][1!] ba\\[rz]$/i => "\\\[ [fF][oO][oO] [a-zA-Z\][1!] [bB][aA]\\[rRzZ]$"

Admittedly very messy.

Bottom line

On the face of it, you could wontfix this and just tell users to construct their Zod regexes to be case-insensitive by design, without relying on the flag. However, that should definitely appear in the documentation as a limitation of JSON Schema/this module!

If the above hack is suitable to you, though, I'm happy writing a PR to implement it. I don't think it'd require much difference than just modifying addPattern and how it's called?

@StefanTerdell
Copy link
Owner

Thanks for opening an issue!

I could certainly be open to having an option to try and correct it. Something like 'patternStrategy': 'escape' | 'preserve' | 'transform' where 'preserve' would still be the default behavior and 'transform' would do "its best" to handle compatibility issues be it from flags or whatever.

I'm a bit worried about how deep this rabbit hole goes though. I don't want to start introducing dependencies, and that includes adding enough code for a specific issue to warrant one.

You could certainly take a stab at it if you want, but just FYI, v4 is around the corner (late summer maybe) and will introduce a more granular lifecycle hook system specifically to let "edgy cases" like this be easier to handle in user land.

@Spappz
Copy link
Author

Spappz commented Apr 25, 2024

Thanks you the quick response!

I'm a bit worried about how deep this rabbit hole goes though. I don't want to start introducing dependencies, and that includes adding enough code for a specific issue to warrant one.

Completely reasonable.

Out of all the flags, s seems trivial to implement. m might be more involved, though i expect i to be the worst. That said, unless I'm severely underestimating the number of special cases hidden in the specification, handling i shouldn't take more than a couple dozen lines? Overall I don't think it'd require a new package dependency or monolithic amount of code to make a simple best-attempt, though I fully understand the wariness of maintenance debt.

v4 is around the corner

If something Big™ is coming soon, I'm happy leaving it be for now (I can fix my regexes to be case-insensitive without needing flags, in situ). Could a point about this be added to the Known Issues section of the README in the meantime? Happy to PR that too if it makes anything easier 😏

@StefanTerdell
Copy link
Owner

If something Big™ is coming soon, I'm happy leaving it be for now (I can fix my regexes to be case-insensitive without needing flags, in situ).

Actually @Spappz this might be a good time to play around with something more involved than a known issue warning. If it gets unwieldy we can just purge it in the next major.

A PR would be welcome :)

@StefanTerdell StefanTerdell added the help wanted Extra attention is needed label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants