refine() function gets called even when parser already failed within translate()/refine()/regex()

[ctollerud]

This is very similar to this one: https://github.com/colinhacks/zod/issues/2113

Happening in version 3.21.4

refine() function still runs even though a previous translate has already invalidated it. It's getting passed some aribtrary object instead of the string that it expects.

const parseResult = z
	.unknown()
	.transform((x, ctx) => {
		if (typeof x === 'string') return x;

		console.log('I fail because input is a number (correct behavior)');
		ctx.addIssue({
			code: 'custom',
		});
		return z.NEVER;
	})
	.transform((x: string) => {
		console.log("I don't get executed (correct behavior since transform above invalidated the input)");
		return x;
	})
	.refine((x: string) => {
		//BUG
		console.log("I shouldn't get called but I do!!!!");
		console.log(`I should be a string but I am a ${typeof x}!!!`); //'object'
		console.log(x); // some sort of '{status:'aborted'} object (the value underlying z.NEVER?)
	})
	.safeParse(42);

console.log(`succeeded:  ${parseResult.success}`); // false (correct behavior)

EDIT: looks like there are more scenarios that are causing this 'zombie refine' scenario. Here's two more:

• A failure within a refine causing it:

import * as z from 'zod';

const parseResult = z
	.string()
	.refine(x => false) // force anything/everything to fail
	.transform(x => {
		console.log("I don't get called"); // correct
		return x;
	})
	.refine((x: string) => {
		console.log("I shouldn't get called!!!!!!");
		console.log(typeof x); // string
		console.log(x); // '123'
	})
	.safeParse('123');

console.log(`succeeded:  ${parseResult.success}`); // false (correct behavior)

• a failure within a regex causing it:

import * as z from 'zod';

const parseResult = z
	.string()
	.regex(/abc/) // fail because input doesn't match regex
	.transform(x => {
		console.log("I don't get called"); // correct
		return x;
	})
	.refine((x: string) => {
		console.log("I shouldn't get called!!!!!!");
		console.log(typeof x); // string
		console.log(x); // '123'
	})
	.safeParse('123');

console.log(`succeeded:  ${parseResult.success}`); // false (correct behavior)

These were pretty easy to stumble upon, so there are likely more out there as well.

[JacobWeisenburger]

confirmed

[Darkhogg]

I am also seeing this problem with a refine -> transform -> refine situation, and in a particular way that makes the second refine crash because it doesn't get the type it expects (the transform is never executed):

(Note: this code is a bit silly, but demonstrates the issue)

import z from 'zod'

const schema = z.string()
  .refine((str) => str.match(/^[0-9]+$/))
  .transform((str) => parseInt(str))
  .refine((num) => num.toExponential())

console.log(schema.parse('123'))
console.log(schema.parse('1234'))
console.log(schema.parse('1234a'))

Last line will crash because code expects a number and gets a string

[ctollerud]

@Darkhogg In case it's helpful as a temporary workaround, using the transform to do the validation still seems to work fine

https://zod.dev/?id=validating-during-transform

[Darkhogg]

@ctollerud That is actually very useful, thank you.

[michaeldebetaz]

I am using 3.21.2 and I confirm that refine is called even when the previous regex fails.

import { z } from "zod";

const schema = z
  .object({
    birthDate: z
      .string()
      .regex(/^\d{2}\.\d{2}\.\d{4}$/)
      .transform((input) => {
        const data = input.split(".");
        const day = data[0];
        const month = data[1];
        const year = data[2];
        return `${year}-${month}-${day}`;
      })
  })
  .refine((data) => {
    console.log(data.birthDate); // 👈 This should never log if regex fails
    return true;
  });

schema.parse({
  birthDate: "10.11.203" // ❌ This will incorrectly log "10.11.203" and the regex Error will throw after that
});

[Jonatthu]

Confirmed this fails as well:

export const PhoneNumberValidationSchema = z.object({
	phoneNumber: z.string().min(10, 'Phone number must be at least 10 characters long'),
});


const schema = PhoneNumberValidationSchema.refine(
	({ phoneNumber }) => {
		try {
		// I should NOT get called BUT I DO!!! :(
			const exist = client.onboarding.phonenumberExists.query({ phoneNumber });

			return !exist;
		} catch {
			return false;
		}
	},
	{ message: 'Phone number already exists', path: ['phoneNumber'] }
);

[kawaemon]

It seems like refinement functions (such as .regex() or .min() or things like that) throws non-fatal issue so that parsing process will continue.

To stop parsing you must mark them as fatal manually. (as documented here: https://zod.dev/?id=abort-early) This rule also applies to .transform() too.

I couldn't find any way to mark fatal the issues from functions like .min() :/

import { z } from "zod";

const numberInString = z
  .string()
  .transform((val, ctx) => {
    const parsed = parseInt(val);
    if (isNaN(parsed)) {
      ctx.addIssue({
        fatal: true, // this line is very important...
        code: z.ZodIssueCode.custom,
        message: "Not a number",
      });
      return z.NEVER;
    }
    return parsed;
  })
  .refine((x) => {
    // ...to ensure this `x` is not NaN
    console.log(x);
    return true;
  });

console.log(numberInString.safeParse("aaa"));
console.log("---");
console.log(numberInString.safeParse("1234"));

the above code outputs

{ success: false, error: [Getter] }
---
1234
{ success: true, data: 1234 }

if fatal: false(default), it shows (z.NEVER is actually { status: "aborted" } on runtime.)

{ status: 'aborted' }
{ success: false, error: [Getter] }
---
1234
{ success: true, data: 1234 }

[ctollerud]

@kawaemon That's some good context, and it makes sense that it's designed so that multiple refines be stacked so that you can report on more than one validation failure at once. And it makes sense that this is the root mechanism that is misbehaving in this instance.

The real issue is when you do something like refine ->transform-> refine, it ends up completely violating the types, and the only reasonable behavior is to prevent the second refine from executing if the first one fails.

[kawaemon]

The real issue is when you do something like refine ->transform-> refine, it ends up completely violating the types

Yes, I agree with that.

I think the source of that issue is role conflict between .transform and .(super)Refine. .transfrom should focus on transforming the value, not refining.

Therefore my opinion is: when .transform reports issues, they should be taken as meaning that the value couldn't be transformed correctly, so should be treated as fatal issue. By doing so type safety of further process will be ok. I think this behavior is more user-friendly than current implementation.

However this change is clearly breaking change so we need to reach consensus among contributors and users. What do you think?

[ctollerud]

@kawaemon Note that this bug can also occur when the initial failure occurs in a refine, not just transform. (See my second example in the original post.)

I agree with your assessment that failure in transform should be treated as fatal (anything else would violate types)...

...but in addition to this, hitting a transformative operator (for example pipe or transform) should cause any already-occurred refinement failures to enter a 'fatal' state wherein no more refines get ran.

All of this together would keep the types sound and align with expected behavior.

[kawaemon]

Note that this bug can also occur when the initial failure occurs in a refine, not just transform. (See my second example in the original post.)

Ah I see. That's very weird. I'll try to fix these issues and will submit a PR if I could get a good solution :D

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ctollerud]

@JacobWeisenburger @colinhacks I'm writing to hopefully prevent this from closing, since this is something IMHO that ought to be resolved, since it's kind of a nasty bug.

If noone gets to it before me, I can attempt a fix in a week or two, but I'd have to familiarize myself with the codebase.

[amir27111998]

Note that this bug can also occur when the initial failure occurs in a refine, not just transform. (See my second example in the original post.)

Ah I see. That's very weird. I'll try to fix these issues and will submit a PR if I could get a good solution :D

Where you at bro

[kasparkallas]

Running into this as well. The refine hits with x value of { "status": "aborted" }, breaking types and being unexpected in general:

export const paymentOptionSchema = z
  .object({
    transferAmountEther: etherAmountSchema
      .optional()
      .refine((x) => {
        console.log({
          x // ??? This sometimes ends up as an object of `{ "status": "aborted" }`.
        })
        return (x ? parseEther(x) > 0n : true);
      })
  });

export const etherAmountSchema = z
  .string()
  .transform((x, ctx) => {
    try {
      return formatEther(parseEther(x));
    } catch {
      ctx.addIssue({
        code: z.ZodIssueCode.custom,
        message: "Not an ether amount.",
      });
      return z.NEVER;
    }
  });

[email protected]

UPDATE: Scratch that! I was missing fatal: true in the addIssue: https://zod.dev/?id=abort-early

[silasdavis]

I can confirm:

UPDATE: Scratch that! I was missing fatal: true in the addIssue: https://zod.dev/?id=abort-early

Works around the issue, but this is still a bug since zod violates it's own typings.

For example, without the fatal flag to addIssue:

export const Numeric = z.union([bigDecimalSchema, z.number(), z.string().min(1), z.bigint()]).transform((v, ctx) => {
  try {
    return Big(v);
  } catch (err) {
    ctx.addIssue({
      code: z.ZodIssueCode.custom,
      message: 'invalid_numeric: ' + (isErrorWithMessage(err) ? err.message : ''),
    });
    return z.NEVER;
  }
});

// This typechecks fine, since `v` should be `BigDecimal`
// but without the `fatal` flag it can be `{ "status": "aborted" }`
Numeric.refine(v => v.gte(0))

Either the type signature of refine should be changed to reflect the fact that it may receive aborted parses (e.g. { "status": "aborted" }) (in which case the current shape of the abort message is not safe since that might be a valid instance of the type I am parsing) or it should never call future refinements after receiving a NEVER.

I cannot think of a reason I would return z.NEVER without wanting an early exit. I'm assuming there is some code-level architecture reason for the redundancy between fatal and the NEVER return.

[acontreras89]

I would be willing to contribute to this if someone is kind enough to point me in the right direction.

From what I see in the docs, this may be somehow intentional:

By default, parsing will continue even after a refinement check fails

In which case, maybe an option would be to accept a fatal flag in .refine params, like this:

const schema = z.string()
  .refine((str) => str.match(/^[0-9]+$/), { fatal: true })
  .transform((str) => parseInt(str))
  .refine((num) => num.toExponential())

console.log(schema.parse('1234a')) // this should work now 

[airjp73]

I'm also seeing this issue. Since zod doesn't run transform after any failed refinements, it seems like it also shouldn't run any refinements that come after that transform.

[Darkhogg]

At the very least, the typings should reflect that refine might be called after not running a transform...

[kirkwaiblinger]

Question - can the behavior be that returning the runtime value of z.NEVER, without any other issues being added, be treated the same as if the following issue had been added?

ctx.addIssue({
   fatal: true,
   // roughly, 
   message: "tranform signalled error by returning `z.NEVER`",
   code: z.ZodIssue.Unknown // or whatever
}

IMHO as a user this wouldn't be a breaking change. The current behavior that passes { "status": "aborted" } through is what is totally broken, and this would be a welcome bugfix.

I'd be happy to try to submit a PR for this if accepted.

---

Related - is there any reason you would ever want a transform that didn't add issues with fatal: true? Downstream usages can and should assume that the transform has succeeded when they run (which, as others have noted, is even what the types currently indicate explicitly). The current behavior causes type unsafety, and evil runtime exceptions, even when .safeParse() is used.