Skip to content

Version key mutation before schema validation can dirty caller objects on ValidationError (and success path) #26

Description

@ig-imanish

Describe the bug

In migrate() (src/migrate.ts:91-93) and migrateAsync() (src/migrate.ts:199-201), the version key is written directly onto the object returned by up() before calling safeParse against schema.and(z.object({ [key]: z.literal(version) })).

If validation fails:

  • The object produced by up() (or a reference it returned, including the caller's original state when a mutating up does return state) now has _version (or custom key) set on it.
  • A caller catching ValidationError can observe a partially-mutated object where the version key is present even though the migration did not succeed.

Even on the success path, the exact object returned from up() gets the version property mutated onto it as a side effect (although currentState / return value comes from parseResult.data, which for Zod object schemas is a fresh object).

Why this is a problem

  • Callers expect that on ValidationError (and MigrationError), their input objects and any temporaries are left as they were.
  • In-place mutating up functions (perfectly legal, though not encouraged) + returning the same reference pollute the previous state object.
  • It violates the principle that a failed migration should have no observable side effects on the provided state.

Reproduction

import { z } from "zod";
import { migrate, ValidationError } from "@nanocollective/json-up";

const upReturned = { name: "ab" }; // fails min(5)

try {
  migrate({
    state: { name: "start" },
    migrations: [
      {
        version: 1,
        schema: z.object({ name: z.string().min(5) }),
        up: () => upReturned,
      },
    ] as const,
  });
} catch (e) {
  if (e instanceof ValidationError) {
    console.log("_version" in upReturned); // => true (bug)
  }
}

Mutating-up case (even worse):

const original = { name: "start" };
try {
  migrate({
    state: original,
    migrations: [{
      version: 1,
      schema: z.object({ name: z.string().min(5), extra: z.boolean() }),
      up: (s: any) => { s.foo = 1; return s; },
    }] as const,
  });
} catch (e) {
  if (e instanceof ValidationError) {
    console.log("_version" in original); // => true (bug)
  }
}

The exact same behavior exists for migrateAsync.

Expected behavior

  • Objects returned from up() (and the original state passed in) must never be mutated by the migration machinery when injecting the version key.
  • The version key must only appear on the successfully returned migrated state.
  • On ValidationError, no version key should be visible on any of the objects the caller supplied or that up() produced.

Environment

  • Package: @nanocollective/json-up
  • Affects both migrate() and migrateAsync()

Additional context

This was identified as a "Medium Priority Issue" during review (src/migrate.ts:91-93 and 199-201).

The mutation happens because the version key is injected into the result object before safeParse. When safeParse fails (or even on success for the up() return value), the side effect remains.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions