Repository navigation
Conversation
…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
|
Thanks for picking this up @breken-ai — the direction is right, and mask-form 1.
|
… 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
|
@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.
On the minor notes: the registry test cases no longer pass the stripped 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 Built by breken, your AI support engineer - breken.ai - this one's on us. |
|
Thanks for the quick turnaround — One consequence of Ran the revised schema against the pinned versions ( Field by field, the other four are safe precisely because they already guard against null:
The schema tests can't catch this — they assert the value is Smallest fix is the pattern - `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 |
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().
|
@ioanbeilic Good catch - fixed. And on the scope note - worth saying plainly: the hole was pre-existing. The column is nullable and apiUpdateApplication is 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. |
|
Verified 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 The harness already exists: 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: |
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.
|
@ioanbeilic Fair, and that's the right bar - added verbatim, in the harness you pointed at ( 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. |
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.saveBuildTypewithbuildType: "dockerfile"demandsherokuVersion: null+railpackVersion: null;application.saveEnvironmentwith onlyenvdemandsbuildArgs/buildSecrets/createEnvFile; onregistry.createthe field.required()was actually forcing isimagePrefix(.nullable().optional()) -registryTypewas mandatory on its own as a barez.enum(["cloud"]), and this PR fixes both. The UI never notices because it submits every form field.Fix
Mask-form
.required():saveBuildTyperequires only{applicationId, buildType},saveEnvironmentrequires{applicationId, env}, andregistry.createrequires the 4 credential fields withregistryTypegetting.default("cloud")matching the DB default (alsoapiTestRegistry).Two behaviors matter beyond the masks (h/t @ioanbeilic's review):
saveEnvironmentkeepsenvrequired. The handler forwards exactly the four fields toupdateApplication, and drizzle'smapUpdateSetdropsundefinedentries and throws when none are left - so anapplicationId-only call would pass validation and die as a plainINTERNAL_SERVER_ERRORwhere a cleanBAD_REQUESTbelongs..default(null), not merely optional: drizzle skipsundefinedon update, so omitting a field must still clear the stale column value (switching build type todockerfileclears a leftoverherokuVersion), exactly like the UI's explicit nulls.registry'simagePrefixfollows 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 (
saveBuildTypemissing identity fields,saveEnvironmentmissingapplicationIdorenv), the stale-clear behavior (omitted nullable fields parse tonullso updates clear), explicit-null compatibility,registryTypedefaulting 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__/apistays green (16/16). Full-repotsc --noEmitleft to CI (exceeded our sandbox's limits), but the change is schema-local.Fixes #4724
The PR appears safe to merge with no actionable correctness, security, or maintainability issues identified.
Summary
applicationIdandbuildTypewhen saving build configuration.applicationIdwhen saving environment configuration.cloudregistry type.Update (reporter review, round 2): ioanbeilic flagged that
.default(null)on all five build-type fields puts aNULLrailpackVersionon the happy path, and railpack.ts interpolated it unguarded (BUILDKIT_SYNTAX=...railpack-frontend:vnull,RAILPACK_VERSION=null), breaking railpack builds. Commit 0b85bf6 addsrailpackVersion || "0.15.4"(column default) in both spots, mirroringherokuVersion || "24". The hole is pre-existing (nullable column +.partial()apiUpdateApplication); this PR closes it.__test__/apire-run 16/16.