zod icon indicating copy to clipboard operation
zod copied to clipboard

Fix branded Record keys in ZodRecord

Open googol opened this issue 2 years ago • 16 comments

Because of a quirk of the extends syntax of typescript conditional types, the previous version of this check for a branded key didn't work as expected.

Thank you @akomm for the explanation of why the extends sides need to be swapped at https://github.com/colinhacks/zod/pull/2287#issuecomment-1689922672

Originally I intended to remove the RecordType type entirely, since it would seem initially that it is only there to work around earlier versions of typescript not having the noUncheckedIndexedAccess option. However, it is really meant to work around a problem with using ZodEnum as the key, see this comment: https://github.com/colinhacks/zod/pull/2287#issuecomment-1676624932

~The RecordType conditional type seems to exist for handling the partialness of indexed objects. However that is handled by the tsconfig option noUncheckedIndexedAccess in [email protected] and later.~

googol avatar Apr 03 '23 11:04 googol

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
Latest commit 28dc5c6c51dbb4fac58121f85e2818f602cc902b
Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/64e9d40bd6632700088e4fbc
Deploy Preview https://deploy-preview-2287--guileless-rolypoly-866f8a.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Apr 03 '23 11:04 netlify[bot]

Many thanks for approving this! Super happy to get this annoyance fixed :)

googol avatar Jun 26 '23 20:06 googol

Just to clarify for myself, when can I expect this to get merged and published?

googol avatar Jun 27 '23 21:06 googol

@sangxxh Hey! Could we get this merged soon?

googol avatar Aug 13 '23 18:08 googol

Unfortunately this is currently intentional. We need the Partial here because Zod doesn't do exhaustiveness checking across the values when you pass a ZodEnum as the key schema. in as the keySchema for a ZodRecord, Zod

const schema = z.record(z.enum(['a', 'b']), z.number());
type schema = z.infer<typeof schema>;

schema.parse({ a: 5 }); // passes
var x: schema = { a: 5 } // missing key 'b'

I agree this isn't ideal, and this will likely change in Zod v4.

colinhacks avatar Aug 14 '23 03:08 colinhacks

Ahh I see, I guess I never figured out what the original reason for this weird type is. The problem I'm having with it is that it's messing up records that are keyed by a branded string. These cases are not meant to have partial applied to them, right?

This line probably was an earlier attempt at fixing this, however it doesn't work for me. Do you have an idea on how to fix this?

const schema = z.record(z.string().brand('some-branded-string'), z.number())
type schema = z.infer<typeof schema> // expected Record<string & BRAND<'some-branded-string'>, number>, got Partial<Record<string & BRAND<'some-branded-string'>, number>>

googol avatar Aug 14 '23 07:08 googol

It is also breaking usage of exactOptionalProperties. This breakage was introduced and blocking my older project from upgrading zod at all. Also all branded keys partial.

The following does not work (extracted from zod code, conditionally mapping record type):

type Test<K> = [z.BRAND<string | number | symbol>] extends [K] ? true : false
type Key = string & z.BRAND<'Key'>
type R = Test<Key> // = false

Here's why:

For A extends B, A must be equal or a subset of B. Extends is a bit confusing, because we expect equal or less and not more (in the sense of distinctive items, ... set theory). In example above, Key's inner type string intersects with the brand, which is a superset.

Hence, the relation must be rotated:

type Test<K> = [K] extends [z.BRAND<string | number | symbol>] ? true : false
type Key = string & z.BRAND<'Key'>
type R = Test<Key> // = true

Funny thing is, that non-finite keys, for example string, are inferred as non-partial: TS Playground

Which is absurd.

Can we at least get the fix through for brand:

export declare type RecordType<K extends string | number | symbol, V> = [
  string
] extends [K]
  ? Record<K, V>
  : [number] extends [K]
  ? Record<K, V>
  : [symbol] extends [K]
  ? Record<K, V>
  : [K] extends [BRAND<string | number | symbol>]
  ? Record<K, V>
  : Partial<Record<K, V>>

akomm avatar Aug 23 '23 13:08 akomm

Thanks for your comment @akomm! That makes a lot of sense as to why the earlier merged fix didn't really fix the problem.

I've included your suggested solution in a commit on this branch now, to see how it passes the test suite.

I'll do testing on my codebase tomorrow (UTC+3) with this idea, and let you all know

googol avatar Aug 23 '23 22:08 googol

I've verified that the change suggested by @akomm does fix the problem we're having in our codebase, so I've amended the branch to fix that one check only, so that this won't affect the case with ZodEnum

@colinhacks would this be ok to merge now

googol avatar Aug 26 '23 10:08 googol

What's the update here, @colinhacks?

Stefandasbach avatar Oct 05 '23 18:10 Stefandasbach

Is there something besides "no time" blocking this? Some doubts or issues? Maybe we can help.

akomm avatar Oct 13 '23 08:10 akomm

ping @colinhacks. is there anything I could do to help this get merged?

googol avatar Jan 17 '24 11:01 googol

@googol do you have a sense of how a Record<someStringLiteralUnion, boolean> could be enforced via the current state of zod? ran into this issue just now and a bit discouraged that this has been stale for so long. asking both to anticipate potential objections from @colinhacks and to see if you've found a better short-term alternative to manually defining this schema as an object

dkrieger avatar Jan 31 '24 17:01 dkrieger

Unfortunately I have nothing :/

googol avatar Jan 31 '24 22:01 googol

Having partial records when the keys are a literal union is by design. It reflects the way zod parses the records : it does not make sure that every value of the union is represented in the record's keys. For the same reason, we get the same behaviour with branded keys because such keys could have a literal union type under the hood.

I have not given many thoughts to this, but maybe using maps instead if records in such use cases is the way to go.

fwoelffel avatar Feb 22 '24 07:02 fwoelffel

@fwoelffel

ZOD does not seem to be very exact about doing what you say, as I've explained in this comment: https://github.com/colinhacks/zod/pull/2287#issuecomment-1689922672

Using an infinite set (=type string) as key results in a non-partial Record. Example from the above comment: TS Playground

For obvious reason, ZOD can not test that all infinite possible keys are actually present. Yet the type suggests otherwise.

This issue has a long history actually. There were multiple topics regarding the exactOptionalPropertyTypes option in TS. It was initially ignored, typically the issues closed. And then suddenly, out of nothing this partial record update was dropped. Of all possible solutions to this tricky problem, the worst was chosen, without taking any feedback from users, because the prior issues were ignored and closed mostly.

I make it short, after a lot of testing and trying, turns out exactOptionalPropertyTypes isn't quite good and not a good fit with noUncheckedIndexAccess. The best solution would be to mimic z.record to the Record TS behavior, which means literal key union are considered non partial. Anyone who want extra type safety with non-finite set keys can enable noUncheckedIndexAccess, here TS playground example with it enabled: TS PG** **By the way, if you switch noUncheckedIndexAccess on and off in tsconfig on the playground, you need to make some change in the code for the type marks to update (add and remove a line)

My argument for this approach is that it would mimic 1:1 TS type safety level to runtime safety level in zod. If you disable noUncheckedIndexAccess in TS, you will miss the same sort of issues at runtime with zod as with TS. Zod does not make it worse. But now the developer has the option to get more or less safety by simply toggling the TS setting. There is no perfect solution, but this one at least gives control of safety to the developer and its sync with TS behavior.

afterthought IMO this issue turns out to be more broad than initially anticipated. That is why as an afterthought we received the Partial<Record<..,..>> change. It would be great if this could be solved in a clean and straightforward way in zod 3.0. Mimic the behavior as explained above would mean that z.record with z.literal or z.union of z.literal keys need to be mapped into z.object under the hood.

akomm avatar Feb 26 '24 09:02 akomm

There's clearly a lot of confusion about why ZodRecord is the way it is. The comment by @fwoelffel is exactly right:

Having partial records when the keys are a literal union is by design. It reflects the way zod parses the records : it does not make sure that every value of the union is represented in the record's keys.

It's impossible (in the general case) for Zod to take an arbitrary schema and make a list of literals that will pass validation. As such, it isn't possible for Zod to do exhaustiveness checking on the keys of a given input object, to make sure, say, all the elements of a ZodUnion appear in the object. That's why Zod makes all fields optional for literal keys.

My argument for this approach is that it would mimic 1:1 TS type safety level to runtime safety level in zod

Exactly, this is the goal. For maximum type safety, Zod should add | undefined when there are non-finite keys in z.record(). In practice most people don't currently have noUncheckedIndexedAccess defined and found this behavior surprising before the RecordType change.

It's important to note that Zod's isn't able to alter its runtime behavior based on your tsconfig.json. Your tsconfig doesn't exist once your code is bundled. So Zod needs to make a decision about what the inferred type is, and its runtime behavior should agree with the inferred type. This is also why it's not easy for Zod and z.object() to magically support exactOptionalPropertyTypes.

Let's talk about improvements I'd like to make to ZodRecord.

noUncheckedIndexedAccess

As @akomm has suggested, Zod can check when noUncheckedIndexedAccess is enabled (in the static domain, not runtime) and add | undefined to all inferred ZodRecord types including non-finite keys. I'm in favor of that.

Practically speaking most people don't have noUncheckedIndexedAccess enabled currently, so I still stand behind the decision to introduce RecordType and remove optionality from non-finite keys. But per @akomm's suggestion the optionality should still remain for users with noUncheckedIndexedAccess: true.

After some tinkering I found this can be done statically with the type alias below. If someone finds a less hacky way let me know.

declare const arg: {[k:string]: string};
type noUncheckedIndexedAccess = (typeof arg.qwerty) extends string ? false : true;

This is a change I'd land it in Zod 4 because it's potentially breaking.

Improved exhaustiveness checking

I'm not very enthusiastic about this, and it's very nontrivial. It's the same difficulty that exists with ZodDiscriminatedUnion, which I described here: https://github.com/colinhacks/zod/issues/2106

The problem boils down to this: given an arbitrary Zod schema, determine the set of literal values that exist that will pass validation. You need to do this in both the runtime and static domains.

In the static domain, this requires a recursive type alias. See partialUtil.DeepPartial for an example of what it looks like. It's very annoying and error-prone to unwrap all the ZodOptional, ZodNullable, ZodEffects, ZodReadonly, ZodUnion, ZodDiscriminatedUnion, instances that may be wrapping your ZodLiteral, ZodEnum, and ZodNativeEnum instances.

Same problem at runtime, this requires a recursive visitor pattern. Out of the gate, there's no way to make this compatible with any user-defined ZodType subclasses, since the logic would necessarily be strongly-coupled to the set of first-party Zod-defined subclasses.

Plus there are schemas for which this problem is literally unsolvable like z.literal("asdf").transform(val => val.toUpperCase() as "ASDF"). There's no way for Zod to determine at runtime that "ASDF" is a potentially valid input, so Zod can't correctly due exhaustiveness checking.

I could add in some custom logic to special-case the major use cases (passing a ZodEnum in as the key to z.record) but a half-baked solution like that is just a recipe for maintenance headaches and user frustration. Look at all the issues surrounding ZodDiscriminatedUnion to see that up close.

I will experiment with this regardless and see if I can land on something that addresses major use cases without undue complexity. But I don't think it's likely.

Limiting keySchema

This is probably what I should have done initially: limit KeySchema to ZodString | ZodNumber | ZodSymbol | ZodEnum | ZodNativeEnum. Then there is a well-defined subset of schema types, which makes all the implementation much simpler. This would certainly lead to some issues when people try to use ZodEffects, etc, but at least the API would be clear.

colinhacks avatar Mar 21 '24 19:03 colinhacks