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

Add option to enable Zod type coercion for query params #1045

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

solomonhawk
Copy link
Contributor

@solomonhawk solomonhawk commented Nov 14, 2023

Status

READY

Description

Fix #961 I expressed a desire to make the generated zod schema for endpoint query parameters more useful for parsing and validating URLSearchParams by making use of zod's type-coercion. It turns out that part of this problem isn't easily solvable within this library's generated code (more on that below), but the part that is ended up being relatively simple to add.

This PR is one possible (relatively naive) approach to how that might work.

Key points:

  • adds a new override.coerceTypes boolean flag to the output config
  • when enabled, the zod codegen will emit e.g. zod.coerce.string() instead of zod.string()
  • this only applies only to coerce-able types (string, number, boolean, bigint, date)
  • this will apply to nested schemas, and I'm not sure that's always the correct thing to do (e.g. zod.array(zod.coerce.number())) - I expect this is useful in some cases

Additional considerations:

  • zod schemas can be arbitrarily complex, and assuming users want simply zod.coerce.<type>() is a naive/opinionated approach
  • there are merits to taking different approaches, e.g.:
    • zod.string().pipe(zod.coerce.date()) or
    • zod.transform(v => new Date(v)).date() (full example not shown here, but you get the idea - do something with input data before sending it to a parser/schema)
  • another approach we could take would be exposing to users something like mutators for individual zod schemas
    • I didn't opt to attempt to solve for that, which is going to be a lot more complex than this simple and hopefully useful approach

Deeper dive on zod + URLSearchParams

It has been discussed before and there are some lingering challenges that aren't really in the scope of the zod package to solve.
Ref: colinhacks/zod#1763

Assumptions:

  • you are using the zod query params schema to both cast AND validate URLSearchParams

Problems:

  • you still need to collect the entries from URLSearchParams, since it can contain multiple values for the same key
  • you can't just do Array.from(urlSearchParams.entries()) because it will return multiple entries with the same key, whereas the zod schema will expect those to be grouped as an array
  • you instead need to collect the values from the URLSearchParams first, only converting them to an array if there are multiple values for the same key
  • a field may be an array field despite having 0 or 1 values, so you can't only rely on the presence of multiple same keys to determine whether it should be an array (you can write the zod schema in a way that handles this more intelligently using zod.transform() before passing that data into zod.array() or another schema)
  • you could introspect the schema to see if the field is expected to be an array, but that code can be a little hairy
    • the approach zodix takes is to assume it's an array if the key appears more than once
    • remix-params-helper uses schema introspection

Closing thoughts:

I found it a bit challenging to figure out the best way to add test coverage for these changes. There are some reasonably well-defined boundaries among the source files here but still it felt heavy-handed to test at the generateZod level. Spinning up the necessary input data wasn't super straightforward.

Right now the tests cover the main codegen emission which is conditional on override.coerceTypes but does not test the code that pipes the generator options through to the internal function (parseZodValidationSchemaDefinition) that uses it.

I know there has been some discussion in other places that touches on some of the overall design decisions within the library such as using string building vs ASTs for the codegen.

I simply wanted to share my perspective as a new contributor wishing it were easier to contribute test coverage to new features.

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

  1. set up an openapi document that uses query parameters with types other than string (e.g. integer, boolean)
  2. configure orval with:
import { defineConfig } from 'orval';

export default defineConfig({
  validators: {
    output: {
      target: '<target>', // fill in
      client: 'zod',
      override: {
        coerceTypes: true,
      },
    },
    input: {
      target: '<input specfile>', // fill in
    },
  },
});
  1. run orval
  2. the emitted zod schemas should have e.g. zod.coerce.number() rather than zod.number()

- this boolean flag enables type coercion within zod schemas
for `queryParams` *only*
@melloware
Copy link
Collaborator

@solomonhawk this is great! Can you check a couple of other open issues and see if your PR addresses them?

Particularly these 2:

#971

#889

@melloware melloware added the enhancement New feature or request label Nov 14, 2023
@melloware melloware added this to the 6.21.0 milestone Nov 14, 2023
@solomonhawk
Copy link
Contributor Author

I only took a quick glance, but I don't have any reason to believe that these changes would have any impact on those 2 issues.

@melloware
Copy link
Collaborator

well the date one you mentioned above about zod.date() and that ticket is saying it currently generates a REGEXP so I thought this PR was related?

@solomonhawk
Copy link
Contributor Author

solomonhawk commented Nov 14, 2023

The changes in this PR do not alter which zod functions are being selected for which schema fields, so I don't think it would have affected changing zod.regex(..) to zod.date() nor would it fix the JS string escaping for string patterns.

Changing the zod function from zod.regex(..) to zod.date() is a simple change to make, but it's not directly related to the other changes in this PR.

@melloware
Copy link
Collaborator

Thanks for checking

@melloware melloware merged commit e7aad27 into orval-labs:master Nov 14, 2023
2 checks passed
@melloware
Copy link
Collaborator

melloware commented Nov 15, 2023

@solomonhawk mind submitting another PR that moves the property to zod specific node?.

override: {
	useDates: false,
	mutator: {
		path: "src/service/AxiosMutator.ts",
		name: "useAxiosMutator",
		},
	zod: {
		coerceTypes: true,
	},
}

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

Successfully merging this pull request may close these issues.

Zod: Query Params Validator is unusable with URLSearchParams for booleans/numbers
2 participants