Skip to content

fix(appkit): skip JSON body parsing for parameterized skipBodyParsing routes - #712

Open
fjakobs wants to merge 4 commits into
mainfrom
fjakobs/la-2047-files-json-upload-empty
Open

fjakobs wants to merge 4 commits into
mainfrom
fjakobs/la-2047-files-json-upload-empty

Conversation

@fjakobs

@fjakobs fjakobs commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Uploading a JSON file through the files plugin (POST /api/files/:volumeKey/upload) stores an empty, 0-byte file. The endpoint still responds {"success":true}, so the data is lost silently. Any request whose Content-Type contains json is affected (application/json, application/geo+json, ...). Browsers send application/json when you fetch a .json File as the body, which is what the dev playground's file browser does. Other file types such as Markdown upload fine.

Root cause

  • Plugin.route() records skipBodyParsing routes by their Express pattern, e.g. /api/files/:volumeKey/upload.
  • The server's global express.json type callback checked rawBodyPaths.has(urlPath), an exact string match against the real URL (/api/files/<volume>/upload). A pattern never equals a URL, so the skip never applied.
  • express.json therefore drained the request stream, and the upload handler streamed nothing to the volume.

The existing server test missed this: it passed plugins via config.plugins, which extendRoutes() doesn't read (it uses the plugin context), and its skip cases sent no Content-Type. It passed without ever exercising the skip.

Fix

The server compiles each declared skip path into a regex once, at startup: one path segment per :param, optional trailing slash, case-insensitive. That matches Express 4's default routing, so every URL that reaches the handler also bypasses the JSON parser. Only :param segments are supported, not Express wildcard or regex paths; a code comment notes the limit. getSkipBodyParsingPaths() and the plugin API are unchanged.

Related hardening (separate commits)

  • Fail loudly instead of writing empty files. If anything still reads the body before the upload handler (custom middleware, another body parser, or a route pattern the matcher doesn't cover), the handler now returns a 500 ("Upload body was already consumed...") instead of uploading 0 bytes and reporting success. It checks req.readableEnded before streaming.
  • service-context.test.ts no longer depends on the shell. Some Databricks-hosted dev environments export DATABRICKS_WORKSPACE_ID, which skipped the mocked SCIM lookup and failed a test. The shared beforeEach now clears it.
  • Higher vitest timeouts for the appkit and playground projects (30s test/hook). createTestApp, the mlflow tests and the dev-playground database tests import large module graphs inside tests and hooks. Under CPU load with coverage, the module transforms alone exceeded the 5s/10s defaults, and the first timeout left a harness app open, so the rest of the file failed too.

Tests

  • upload.test.ts: a request whose body was already drained gets a 500, and the connector's upload is never called. Separately, reverting the server fix and posting JSON through the real server now produces this 500 instead of a 0-byte success.
  • server.test.ts: the skip-body-parsing test now wires the plugin through the plugin context, uses the real /api/files/:volumeKey/upload pattern, and sends application/json. It covers the URLs that must skip (query string, trailing slash, different case) and those that must still parse (other routes, a :param spanning two segments, an empty segment).
  • files/tests/plugin.integration.test.ts: a new test boots a real app (server + files plugin with an allow-all volume), uploads {"hello":"world"} as application/json, and asserts the SDK uploadFile call receives exactly those bytes.
  • The server.test.ts and integration tests fail on main (the integration test receives '') and pass with this change.

Verification

End to end against a Databricks workspace, with a minimal AppKit app (server + files plugin on a Unity Catalog volume, allow-all policy), uploading through the HTTP route with curl:

File Before After
data.json (25 bytes, application/json) stored as 0 bytes, reads back empty stored as 25 bytes, reads back intact
notes.md (15 bytes, text/markdown) 15 bytes 15 bytes

pnpm build, pnpm docs:build, pnpm check:fix, pnpm -r typecheck and the full pnpm test (5929 tests) pass locally.

This pull request and its description were written by Isaac.

… routes

Routes declared with `skipBodyParsing` are recorded by their Express
pattern (e.g. `/api/files/:volumeKey/upload`), but the server compared
that pattern to the request URL with an exact `Set.has`, so it never
matched. `express.json` then drained `application/json` upload bodies
before the files handler read them, and JSON files were uploaded as
0 bytes while the API reported success.

Compile each declared pattern into a regex (one segment per `:param`,
optional trailing slash, case-insensitive, like Express 4 routing).

The existing server test passed plugins via `config.plugins`, which
`extendRoutes()` ignores, and sent no content type, so it passed without
exercising the skip at all. Wire it through the plugin context with the
real files pattern, and add a files integration test that uploads an
`application/json` body and checks the SDK receives the exact bytes.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Fabian Jakobs <fabian.jakobs@databricks.com>
@fjakobs
fjakobs requested a review from a team as a code owner November 6, 2026 15:25
@fjakobs
fjakobs requested a review from ditadi November 6, 2026 15:25
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

📦 Bundle size report

Compared against bundle-size-baseline.json (main).

@databricks/appkit

npm tarball (packed): 1.2 MB (+624 B) — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 1.3 MB (+389 B) 460 KB (+136 B)
Type declarations 465 KB 169 KB (+1 B)
Source maps 2.5 MB (+1.3 KB) 859 KB (+428 B)
Other 11 KB 3.7 KB
Total 4.2 MB (+1.7 KB) 1.5 MB (+565 B)
Per-entry composition (own code — deps external (as shipped))
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
. 106 KB (+128 B) 2.5 KB 109 KB (+128 B) external 345 KB (+305 B)
./beta 103 KB 482 B 104 KB external 310 KB
./testing 45 KB (-3 B) 32 KB (+70 B) 77 KB (+67 B) external 224 KB (+106 B)
./tsdown 520 B 0 B 520 B external 813 B
./type-generator 26 KB 0 B 26 KB external 74 KB

Chunks:

Entry Chunk Load Size (gz)
. index.js initial 102 KB
. utils.js initial 4.6 KB
. remote-tunnel-manager.js lazy 2.5 KB
./beta beta.js initial 83 KB
./beta modular.js initial 6.0 KB
./beta stream-manager.js initial 5.9 KB
./beta service-context.js initial 4.2 KB
./beta databricks.js initial 3.3 KB
./beta client.js initial 647 B
./beta index.js initial 20 B
./beta supervisor-api.js lazy 192 B
./beta databricks.js lazy 176 B
./beta index.js lazy 114 B
./testing manifest.js initial 32 KB
./testing index.js initial 10 KB
./testing wide-event-emitter.js initial 2.9 KB
./testing index.js lazy 28 KB
./testing remote-tunnel-manager.js lazy 2.5 KB
./testing utils.js lazy 1.8 KB
./tsdown index.js initial 520 B
./type-generator index.js initial 26 KB

@databricks/appkit-ui

npm tarball (packed): 397 KB — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 429 KB 147 KB
Type declarations 267 KB 98 KB
Source maps 853 KB 286 KB
CSS 16 KB 3.2 KB
Total 1.5 MB 534 KB
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
./js 5.3 KB 49 KB 55 KB 208 KB 14 KB
./js/beta 2.1 KB 0 B 2.1 KB 0 B 5.0 KB
./react 433 KB 49 KB 481 KB 1.3 MB 177 KB
./react/beta 4.9 KB 0 B 4.9 KB 0 B 12 KB

Chunks:

Entry Chunk Load Size (gz)
./js index.js initial 5.2 KB
./js chunk initial 120 B
./js apache-arrow lazy 49 KB
./js/beta beta.js initial 2.1 KB
./react index.js initial 431 KB
./react tslib initial 2.1 KB
./react apache-arrow lazy 49 KB
./react/beta beta.js initial 4.9 KB

fjakobs and others added 3 commits November 6, 2026 15:34
If anything reads the request body before the files upload handler (a
body parser, custom middleware), the handler streamed an empty body and
reported success, so the file was silently saved as 0 bytes. Check
`req.readableEnded` before streaming and return a 500 with a clear
error instead.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Fabian Jakobs <fabian.jakobs@databricks.com>
Shells on Databricks-hosted dev environments export
DATABRICKS_WORKSPACE_ID, which makes ServiceContext skip the mocked SCIM
lookup and fails the pre-configured client test. Clear it in the shared
beforeEach; the test that covers the env var sets it explicitly.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Fabian Jakobs <fabian.jakobs@databricks.com>
createTestApp, the mlflow tests and the dev-playground database tests
import large module graphs inside tests and hooks. Under CPU load with
coverage, module transforms alone exceeded the 5s/10s defaults, and the
first timeout left a harness app open, failing the rest of the file.
Give the appkit and playground projects 30s test and hook timeouts.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Fabian Jakobs <fabian.jakobs@databricks.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

🤖 AppKit PR bot

🔬 Run evals

Start an eval for this PR from the evals-monitor app: Go to Evals Monitor →

📦 Try this PR's app template

Scaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh auth login — and the Databricks CLI):

gh run download 42245126232 -R databricks/appkit -n appkit-template-0.90.0-pr.263e596-fjakobs-la-2047-files-json-upload-empty-712 -D appkit-pr-712 \
  && unzip -o "appkit-pr-712/appkit-template-0.90.0-pr.263e596-fjakobs-la-2047-files-json-upload-empty-712.zip" -d "appkit-pr-712" \
  && databricks apps init --template "appkit-pr-712"

The template pins @databricks/appkit and @databricks/appkit-ui to tarballs built from this branch, so the scaffolded app runs against this PR's code.

This branch has not been deployed

No deployments
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.

1 participant