fix: wrap GET input params in SuperJSON { json } envelope - #2
Closed
WilliamAGH wants to merge 6 commits into
Closed
Conversation
apiGet sent query params as a bare JSON object in ?input=, but the
Dokploy server runs tRPC with the SuperJSON transformer, which
deserializes a bare object to undefined — every parameterized GET
command failed with HTTP 400 while POSTs and parameterless GETs worked.
Send the input in SuperJSON's serialized shape ({ json: params }), the
same envelope apiPost already uses for request bodies. Adds regression
tests asserting the exact request URL with and without params.
Fixes #1
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes how the CLI client serializes input for parameterized tRPC GET requests so the server-side SuperJSON transformer can correctly deserialize the input payload.
Changes:
- Update
apiGetto encode GETinputas a SuperJSON-style{ json: params }envelope (matchingapiPostbehavior). - Add Vitest coverage asserting the GET URL query shape (wrapped input) and the no-params behavior (no
inputquery string).
File summaries
| File | Description |
|---|---|
| tests/client.test.ts | Adds axios mocking and new tests validating apiGet URL encoding behavior. |
| src/client.ts | Wrapes GET params in { json: ... } when building the tRPC input query string. |
Review details
Suppressed comments (1)
tests/client.test.ts:99
- Same as above: using
as neverfor the axios client mock makes the test harder to reason about and defeats type-checking. Cast toAxiosInstance(or a minimal interface) instead.
get: async (url: string) => {
urls.push(url);
return { data: {} };
},
}) as never;
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Generated parameterless commands always pass Commander's opts, which is
{} once --json is stripped. The existing no-params test called apiGet
with undefined — a path no generated command takes. Add a regression
test for apiGet("project.all", {}) asserting the encoded URL, and keep
the undefined-path test as documentation of the public contract. No
production change: empty opts correctly takes the envelope branch, and
no-input procedures ignore the deserialized {} on the server.
Two tests now cover the two ways generated commands actually call apiGet: with params, and with the empty opts object Commander hands parameterless commands. The undefined-argument test covered a branch no generated caller takes, so it is dropped. Shared beforeEach replaces the per-test axios mock plumbing, and the client comment is trimmed to the durable contract (SuperJSON envelope required, bare object 400s).
Replace per-test env setup, dynamic axios imports, create overrides, URL arrays, closures, and as-never casts with one vi.hoisted vi.fn GET spy in the axios mock, shared describe setup/reset, and direct toHaveBeenCalledWith assertions. The three cases are the apiGet input contract: nonempty params, undefined params, and an empty params object. Trim the client comment to the single durable invariant.
Owner
Author
|
Closing this fork-only duplicate. Dokploy#52 already contains the same fix and tests; this checkout remains only as a temporary local runtime workaround until an upstream release is available. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1.
What's broken
Every generated GET command with parameters (
compose one,application one,*.search,*.readLogs, ...) fails with HTTP 400.apiGet()sends the params as a bare JSON object in?input=, but the Dokploy server runs tRPC with the SuperJSON transformer, so input has to arrive in SuperJSON's serialized shape,{ json: ... }. A bare object deserializes toundefinedand the server rejects the call before the procedure runs. POSTs and parameterless GETs are unaffected, which is why this slipped through.Fix
One line in
apiGet(): wrap the params the same wayapiPost()already wraps its body. 141 of the 223 GET operations inopenapi.jsontake query parameters, and every generated GET command routes through this function, so this covers all of them.This is the immediate, transitional fix: it keeps the existing tRPC transport working against every Dokploy server version that runs the SuperJSON transformer (that setup goes back to tRPC v10-era servers, so nothing is dropped). The larger direction is upstream Dokploy/cli#45 (building on #40), which migrates the whole client off tRPC onto the server's REST surface — that would fix these failures too, but it rewrites POST handling and only works against servers new enough to expose the REST endpoints, so it's a strategic decision rather than a patch. If that migration ever lands, this envelope fix becomes obsolete; until then it's what makes the CLI usable. Upstream #52 is the same one-line change, reported verified against a live 0.30.2 server (400 before, 200 after). None of the three upstream PRs has a review yet.
Tests
Three tests pin down the request URL for the
apiGet()input contract: nonempty params, undefined params, and an empty params object. Full suite passes locally: 21/21 (tsc -bthenvitest run;cli.test.tsshells out to the built CLI, sodist/has to exist first). Fork CI didn't run — GitHub hasn't indexed the fork's workflows yet — so the OS/Node matrix is unverified.