Skip to content

fix: narrow zod .required() masks so unrelated nullable fields stay optional - #5393

Closed
breken-ai wants to merge 4 commits into
Dokploy:canaryfrom
breken-ai:fix-4724-schema-required-mask
Closed

breken-ai wants to merge 4 commits into
Dokploy:canaryfrom
breken-ai:fix-4724-schema-required-mask

Conversation

@breken-ai

@breken-ai breken-ai commented Sep 9, 2026 •

Copy link
Copy Markdown

Root cause

The API schemas are built with bare .required() over picked fields, so fields that only apply to other configurations become mandatory for every API caller. Verified all three reported mutations on current main: application.saveBuildType with buildType: "dockerfile" demands herokuVersion: null + railpackVersion: null; application.saveEnvironment with only env demands buildArgs/buildSecrets/createEnvFile; on registry.create the field .required() was actually forcing is imagePrefix (.nullable().optional()) - registryType was mandatory on its own as a bare z.enum(["cloud"]), and this PR fixes both. The UI never notices because it submits every form field.

Fix

Mask-form .required(): saveBuildType requires only {applicationId, buildType}, saveEnvironment requires {applicationId, env}, and registry.create requires the 4 credential fields with registryType getting .default("cloud") matching the DB default (also apiTestRegistry).

Two behaviors matter beyond the masks (h/t @ioanbeilic's review):

  • saveEnvironment keeps env required. The handler forwards exactly the four fields to updateApplication, and drizzle's mapUpdateSet drops undefined entries and throws when none are left - so an applicationId-only call would pass validation and die as a plain INTERNAL_SERVER_ERROR where a clean BAD_REQUEST belongs.
  • The nullable build-type fields are optional-with-.default(null), not merely optional: drizzle skips undefined on update, so omitting a field must still clear the stale column value (switching build type to dockerfile clears a leftover herokuVersion), exactly like the UI's explicit nulls. registry's imagePrefix follows the same shape.

Backward-compatible: explicit nulls (the UI's shape) and explicit values parse unchanged.

Verification

Ten schema-level vitest cases: the 3 reported mutations, the rejection cases (saveBuildType missing identity fields, saveEnvironment missing applicationId or env), the stale-clear behavior (omitted nullable fields parse to null so updates clear), explicit-null compatibility, registryType defaulting on both registry schemas. Red-green verified: the two revision cases (default-null clearing, applicationId-only rejection) fail against the pre-revision schema, all 10 pass with the fix; the rest of __test__/api stays green (16/16). Full-repo tsc --noEmit left to CI (exceeded our sandbox's limits), but the change is schema-local.

Fixes #4724

Built by breken, your AI support engineer - breken.ai - this one's on us.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or maintainability issues identified.

Summary

  • Requires only applicationId and buildType when saving build configuration.
  • Requires only applicationId when saving environment configuration.
  • Defaults registry creation and connection-test inputs to the database-supported cloud registry type.
  • Adds schema-level regression coverage for omitted fields, required fields, defaults, and explicit-null compatibility.

Update (reporter review, round 2): ioanbeilic flagged that .default(null) on all five build-type fields puts a NULL railpackVersion on the happy path, and railpack.ts interpolated it unguarded (BUILDKIT_SYNTAX=...railpack-frontend:vnull, RAILPACK_VERSION=null), breaking railpack builds. Commit 0b85bf6 adds railpackVersion || "0.15.4" (column default) in both spots, mirroring herokuVersion || "24". The hole is pre-existing (nullable column + .partial() apiUpdateApplication); this PR closes it. __test__/api re-run 16/16.

…ptional

Bare .required() over picked fields makes fields that only apply to
other configurations mandatory for every API caller:
- application.saveBuildType (dockerfile) demands herokuVersion:null +
  railpackVersion:null
- application.saveEnvironment demands buildArgs/buildSecrets/
  createEnvFile
- registry.create demands registryType though the DB defaults it

Use mask-form .required() (zod v4): saveBuildType requires only
{applicationId, buildType}, saveEnvironment only {applicationId}, and
registry.create gets registryType .default("cloud") matching the DB
default (also apiTestRegistry). Explicit nulls (the UI's shape) still
parse unchanged.

Fixes Dokploy#4724
@ioanbeilic

Copy link
Copy Markdown
Contributor

Thanks for picking this up @breken-ai — the direction is right, and mask-form .required() is exactly what I had in mind. Two things I'd fix before this merges, both caused by the same detail: making a field optional is not the same as defaulting it to null, because Drizzle skips undefined.

1. saveEnvironment now accepts a payload that can only fail at the DB layer

.required({ applicationId: true }) also leaves env optional, but the handler forwards exactly those four fields:

await updateApplication(input.applicationId, {
  env: input.env,
  buildArgs: input.buildArgs,
  buildSecrets: input.buildSecrets,
  createEnvFile: input.createEnvFile,
});

and updateApplication does a bare .set({ ...rest }). In drizzle-orm@0.45.2 (the pinned version), mapUpdateSet drops undefined entries and throws when none are left:

const entries = Object.entries(values).filter(([, value]) => value !== undefined)/* … */;
if (entries.length === 0) {
  throw new Error("No values to set");
}

So saveEnvironment({ applicationId: "app-1" }) passes validation and then throws a plain Error → INTERNAL_SERVER_ERROR, where today it is a clean BAD_REQUEST. .required({ applicationId: true, env: true }) keeps the actual fix (buildArgs / buildSecrets / createEnvFile stay optional, which was the complaint) and closes that hole.

2. Optional-without-a-default silently changes what an update clears

Same undefined-is-skipped behaviour, other direction. Today the UI's explicit null writes the column. With the fields merely optional, omitting them leaves the old value in the row: saveBuildType({ applicationId, buildType: "dockerfile" }) on an app that was heroku_buildpacks now succeeds but keeps the stale herokuVersion. It is inert for the build (the builder branches on buildType), but it is a behaviour change none of the seven tests covers.

That is why the issue asked for optional with default: null rather than just optional: .default(null) makes Zod fill the value, Drizzle writes it, and switching build type keeps clearing the fields exactly as the UI does today. It also makes case 1 impossible by construction rather than by a second required field.

Minor, take or leave

  • The three apiCreateRegistry cases pass organizationId / registryId, but apiCreateRegistry is built from createSchema.pick({}) — those keys are not in the schema, so Zod strips them. They read like part of the contract when they are not.
  • apiTestRegistry also changes (registryType gets a default) and has no test.
  • For the record, the field .required() was actually forcing on registry.create is imagePrefix (.nullable().optional()); registryType was mandatory on its own, as a bare z.enum(["cloud"]). This PR fixes both, but the root-cause note only describes the .required() half.

… to null

- saveEnvironment keeps env required: an applicationId-only call would
  pass validation and then die in drizzle's mapUpdateSet as
  INTERNAL_SERVER_ERROR instead of a clean BAD_REQUEST
- saveBuildType nullable fields now default(null): drizzle skips
  undefined on update, so merely-optional fields would leave stale
  values (e.g. herokuVersion surviving a switch to dockerfile); null
  defaults clear them exactly like the UI's explicit nulls
- registry imagePrefix also optional-with-default-null; add
  apiTestRegistry coverage and drop stripped keys from test inputs
@breken-ai

Copy link
Copy Markdown
Author

@ioanbeilic Both catches landed in aba0639 - thank you, these are exactly the kind of details that turn a schema tweak into a behavior change nobody notices until production.

  1. saveEnvironment now requires { applicationId, env } - an applicationId-only call once again fails validation as a clean BAD_REQUEST instead of surviving to drizzle's "No values to set" INTERNAL_SERVER_ERROR.

  2. All the nullable build-type fields are now optional-with-.default(null) (dockerfile, dockerContextPath, dockerBuildStage, herokuVersion, railpackVersion), so an omitted field writes null and clears the stale column value - saveBuildType({ applicationId, buildType: "dockerfile" }) on a former heroku_buildpacks app now clears herokuVersion exactly like the UI's explicit null. imagePrefix on both registry schemas follows the same shape.

On the minor notes: the registry test cases no longer pass the stripped organizationId/registryId keys, apiTestRegistry now has coverage for the registryType default, and the root-cause note in the PR body now correctly names imagePrefix as the field .required() was forcing (with registryType mandatory on its own as the bare enum).

Tests: 10/10 in the schema suite green, including a new applicationId-only rejection case and the default-null clearing case (both verified red against the pre-revision schema); rest of __test__/api still green (16/16).

Built by breken, your AI support engineer - breken.ai - this one's on us.

@ioanbeilic

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround — saveEnvironment and the null-defaulting both look right now.

One consequence of aba0639 though: .default(null) is applied to all five build-type fields, and railpackVersion is the one field the builder interpolates without a fallback. So the minimal call this issue set out to enable now writes NULL and breaks the build it just configured.

Ran the revised schema against the pinned versions (zod@4.3.6, drizzle-zod@0.5.1, drizzle-orm@0.45.2):

saveBuildType({ applicationId: "app-1", buildType: "railpack" })
  parsed:        {..., "railpackVersion": null}
  UPDATE params: [null, null, null, "railpack", null, null, "app-1"]

  → packages/server/src/utils/builders/railpack.ts then emits:
      BUILDKIT_SYNTAX=ghcr.io/railwayapp/railpack-frontend:vnull
      export RAILPACK_VERSION=null

Field by field, the other four are safe precisely because they already guard against null:

field builder usage null-safe
dockerfile dockerfile || "Dockerfile" — filesystem/directory.ts yes
dockerContextPath getDockerContextPath(application) || defaultContextPath yes
dockerBuildStage if (dockerBuildStage) { … } yes
herokuVersion `heroku/builder:${application.herokuVersion || "24"}` yes
railpackVersion `…railpack-frontend:v${application.railpackVersion}` and export RAILPACK_VERSION=${application.railpackVersion} no

The schema tests can't catch this — they assert the value is null, which is exactly what breaks one layer down.

Smallest fix is the pattern heroku.ts already uses, mirroring the column default:

-		`BUILDKIT_SYNTAX=ghcr.io/railwayapp/railpack-frontend:v${application.railpackVersion}`,
+		`BUILDKIT_SYNTAX=ghcr.io/railwayapp/railpack-frontend:v${application.railpackVersion || "0.15.4"}`,
-export RAILPACK_VERSION=${application.railpackVersion}
+export RAILPACK_VERSION=${application.railpackVersion || "0.15.4"}

Worth noting this is a latent hole regardless of this PR — the column is nullable and apiUpdateApplication is .partial(), so railpackVersion: null is already reachable today. This PR just puts it on the documented happy path, so it seems like the right place to close it.

The build-type fields now default to null (a reviewer catch on Dokploy#5393:
the UI previously wrote null explicitly, and the schema change moved
that to zod defaults). railpack.ts interpolated railpackVersion
unguarded, so a null row would emit
BUILDKIT_SYNTAX=...railpack-frontend:vnull and RAILPACK_VERSION=null,
breaking the railpack build. Fall back to the column default, matching
the herokuVersion || "24" pattern.

The hole is latent on main too: the column is nullable and
apiUpdateApplication is .partial().
@breken-ai

Copy link
Copy Markdown
Author

@ioanbeilic Good catch - fixed. railpackVersion || "0.15.4" in both spots (the BUILDKIT_SYNTAX arg and the RAILPACK_VERSION export), mirroring the herokuVersion || "24" pattern. Correct that the other four were already guarded.

And on the scope note - worth saying plainly: the hole was pre-existing. The column is nullable and apiUpdateApplication is .partial(), so railpackVersion: null was reachable before this PR; the schema change just puts it on the documented happy path, which made this the right place to close it. Committed as 0b85bf6 with the test/api suite re-run 16/16.

Built by breken, your AI support engineer - breken.ai finds the bug, proves the fix with unmatched RCA, answers customers in your voice, and keeps your code and support docs clean - this one's on us.

@ioanbeilic

Copy link
Copy Markdown
Contributor

Verified 0b85bf6 — both interpolations guarded, matching the herokuVersion || "24" shape. Agreed on the scope note too.

One last thing and I'll get out of the way: the railpack fix has no test. The 16 cases are all schema-level, and the one that touches railpack asserts expect(parsed.railpackVersion).toBeNull() — that is the input to the bug, not its consequence. Nothing asserts the builder emits v0.15.4 rather than vnull, so reverting those two lines keeps the suite green.

The harness already exists: apps/dokploy/__test__/deploy/railpack.command.test.ts has five getRailpackCommand cases and a createApplication(overrides) factory that already sets railpackVersion: "0.15.4". So it is five lines in a file this PR would otherwise not need to touch:

it("falls back to the column default when railpackVersion is null", () => {
	const command = getRailpackCommand(createApplication({ railpackVersion: null }));

	expect(command).toContain("railpack-frontend:v0.15.4");
	expect(command).not.toContain("vnull");
	expect(command).not.toContain("RAILPACK_VERSION=null");
});

Worth having precisely because the failure mode is quiet: :vnull is not a validation error, it is an image pull that fails at build time, on an app whose config the API call reported as saved successfully.

Reviewer catch on Dokploy#5393: the schema tests assert the input (null), not
the consequence. Clear a revert path: when railpackVersion is null, the
command carries railpack-frontend:v0.15.4, never vnull, never
RAILPACK_VERSION=null. Red against the unguarded builder, green with
the fallback.
@breken-ai

Copy link
Copy Markdown
Author

@ioanbeilic Fair, and that's the right bar - added verbatim, in the harness you pointed at (__test__/deploy/railpack.command.test.ts). Verified the way you meant it: reverting those two lines turns it red (railpack-frontend:vnull), restoring the fallback turns it green. Suite is 6/6 there, 22/22 with __test__/api.

Thanks for working this one all the way through with me - every round made the PR harder to break. Good luck with the rest.

Built by breken, your AI support engineer - breken.ai finds the bug, proves the fix with unmatched RCA, answers customers in your voice, and keeps your code and support docs clean - this one's on us.

@Siumauricio Siumauricio closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API: mutations require explicitly passing unrelated nullable fields (saveBuildType, saveEnvironment, registry.create)

4 participants