filer: switch workspace upload from import-file to /workspace/import - #6149
Open
Sankalp-Mittal wants to merge 29 commits into
Open
filer: switch workspace upload from import-file to /workspace/import#6149Sankalp-Mittal wants to merge 29 commits into
Sankalp-Mittal wants to merge 29 commits into
Conversation
Replace POST /api/2.0/workspace-files/import-file/{path} with the multipart
variant of POST /api/2.0/workspace/import (via the SDK's Workspace.Upload +
format=AUTO). The previous endpoint is deprecated; the new one has a higher
rate limit (30 vs 20 rps/workspace) and is ~1.5-2x faster for typical bundle
deployments.
Error handling is rewritten to branch on SDK sentinels (errors.Is against
ErrNotFound / ErrResourceAlreadyExists / ErrAlreadyExists /
ErrInvalidParameterValue / ErrPermissionDenied) and the AIP-193 ErrorInfo
reason WORKSPACE_OBJECT_TYPE_MISMATCH, with a message-substring fallback for
workspaces where WP-6031 has not rolled out.
The testserver now decodes the multipart /workspace/import body and mirrors the
real endpoint's format=AUTO notebook detection. A new bodyContains option on the
fault mechanism lets a test target a single file's upload, which is no longer
possible by URL since every upload shares the /workspace/import path.
Co-authored-by: Isaac
Collaborator
Integration test reportCommit: ac478ab
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 7 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
marked this pull request as ready for review
August 4, 2026 08:22
Contributor
Approval status: pending
|
Sankalp-Mittal
marked this pull request as draft
August 4, 2026 11:10
The size limits in WorkspaceFilesClient.Write were described using internal config flag names and figures measured by hand against one workspace, neither of which a reader can verify. Replace them with the documented limits and link the sources. This corrects one figure: the notebook cap was described as a flat 10 MiB, but IPYNB notebooks are documented at 100 MB. Only source-format notebooks are capped at 10 MB, so the limit depends on how format=AUTO classifies the payload. Also restore the rate limit doc link to MaxRequestsInFlight, which the comment carried when the constant was introduced in #81. The value is unchanged. Add a comment above each test in workspace_files_client_test.go stating what it covers. Co-authored-by: Isaac
The local_code_source test filtered recorded uploads by URL path, which no longer matches: /workspace/import carries the target filename in the multipart body. Filter on the body instead, the same way auto-migrate-empty-tfstate does. --del-field raw_body is dropped because the tarball is binary and the request recorder already summarizes it as a size placeholder. This test was added in #6110, after the upload migration branch was cut, so it was not covered by the earlier fixture updates. Co-authored-by: Isaac
Two error shapes stopped mapping to fileAlreadyExistsError when Write moved to sentinel matching. A 409 carrying no error_code unwraps to ErrResourceConflict only. ErrAlreadyExists and ErrResourceAlreadyExists are siblings under it, not parents, so neither matched. The workspace returns exactly this shape, and locker relies on fs.ErrExist here to report which client holds the lock. Match the parent sentinel instead. A 400 with an empty error_code matches no sentinel but ErrBadRequest, so the notebook already-exists check added in #5106 no longer fired. Restore it; that PR notes the newer message format may not have reached all workspaces, and this change has no evidence the rollout has since completed. Both shapes now have test cases; each fails against the previous logic. Co-authored-by: Isaac
The SDK's Workspace.Upload sets X-Databricks-Workspace-Id from cfg.WorkspaceID behind a bare != "" check, so it forwards the CLI-only "none" sentinel as a literal workspace ID. That sentinel is written to .databrickscfg by `auth login --skip-workspace` and names no real workspace, so the upload is misrouted. Encode the same multipart body in newImportForm and post it through apiClient.Do, which routes the header through auth.WorkspaceIDHeaders and maps the sentinel to no header at all. Field names and layout match Upload; the `language` field is omitted because it only applies to format=SOURCE. Co-authored-by: Isaac
Write no longer calls Workspace.Upload, so the mock expectations on it asserted against a call that is gone. Drive the tests through testserver and inspect the multipart body the CLI actually sends, which pins the wire format rather than the SDK call shape: path, content, format=AUTO, and overwrite present only when requested. Adds coverage for the routing header the previous commit fixes, including the "none" sentinel case. Co-authored-by: Isaac
Tighten the size-limit block to the classification rule and its two doc links, and reorganize the path-conflict list so each bullet names the branch that handles it, including the two shapes that carry no error_code. Also move multipartContentLimit above the function that reads it. Co-authored-by: Isaac
Match the notebook extension case-insensitively, so a ".R" source is detected the way the real endpoint detects it. Also require the format field to be AUTO rather than accepting an absent value. The real endpoint defaults to SOURCE, which imports every payload as a notebook, so a caller that omits the field is a bug the fake should surface. The rejection message now reports what was received. Co-authored-by: Isaac
Replace the hand-rolled sys.argv slicing with argparse. The manual parser had to special-case --body-contains before the positionals, so the flag only worked in that one position and the usage string was maintained in two places. Behavior is unchanged: same positionals, same optional error_code default, same payload. Co-authored-by: Isaac
Guards the ToLower added to detectNotebookLanguage: the test fails against the previous case-sensitive switch, where ".R" was stored as a FILE at the unstripped path. Co-authored-by: Isaac
…port The test filtered recorded uploads by URL path, which no longer matches: /workspace/import carries the target filename in the multipart body. Both assertions still show exactly two uploads (the config and the command), so the test's point -- that the requirements file is never uploaded -- is unchanged; only the recorded request shape differs. This test was added in #6166, which merged into this branch after the upload migration, so it was not covered by the earlier fixture updates. Mirrors the fix applied to ai_runtime_task/local_code_source for the same reason. Co-authored-by: Isaac
The local_code_source test asserts the uploaded size of each .air_snapshots tarball. On Windows, git checks the packaged files out with CRLF, which adds a byte per newline and shifts every recorded size, so both Windows jobs failed with 2-4 byte differences. Pin the packaged files to eol=lf, following the per-directory pattern in acceptance/experimental/air/run-submit-deps/.gitattributes. Only the files that actually land in a tarball are listed; debug.log is excluded by .gitignore and stays unpinned. The recorded sizes only became load-bearing when uploads moved to /workspace/import, which records the multipart body; the previous endpoint put the filename in the URL and never recorded the payload. Co-authored-by: Isaac
Python's text mode translates \n to \r\n on write under Windows, so editing a file through this helper replaced its LF endings with CRLF. Pinning the checked out bytes with .gitattributes does not help, because the rewrite happens after checkout: in ai_runtime_task/local_code_source only the upload recorded after the update_file.py step still reported a different size on Windows. Pass newline="" on both the read and the write so the file's existing endings survive verbatim, in either direction. Co-authored-by: Isaac
fb99c27 wrapped unmapped upload failures with "failed to upload <path>", which changed the message the upload/internal_server_error and upload/timeout tests capture. Regenerate their goldens to match. Co-authored-by: Isaac
print_requests.py --sort orders by the whole request, including the multipart body. One synced file is out.requests.txt, whose recorded body differs between runs, so its position in the list was nondeterministic — passing locally but failing in CI. Drop --sort and sort the displayed "METHOD path" lines instead, which are stable. The contains.py assertions are unaffected. Co-authored-by: Isaac
Sankalp-Mittal
marked this pull request as ready for review
August 7, 2026 12:14
#6196 removed the Local config field and rewrote the other test.toml files to drop it, but #5587 added this test.toml with Local = true around the same time, so it was not swept. The acceptance harness rejects undecoded keys, failing the test on config parse before any assertion runs. Remove the key. Co-authored-by: Isaac
#6196 removed the Local config field and regenerated every out.test.toml snapshot, but this test was added on this branch in parallel, so its committed snapshot still carried Local = true. The post-test "no files changed" check regenerates it and fails on the diff. Regenerate to match. Co-authored-by: Isaac
The previous newline="" fix stopped Python from rewriting \n to \r\n on write, but it also made the read preserve CRLF verbatim. Script search strings are always LF, so a multi-line replacement (e.g. duplicate_yaml_merge_key) no longer matched on Windows, where databricks.yml is checked out with CRLF, and update_file.py errored with "old=... not found". Normalize CRLF to LF on read so the LF search string matches, and keep the newline="" write so the result stays LF. Net effect on Windows: the edited file is normalized to LF, which is what every downstream upload/hash assertion expects anyway. Co-authored-by: Isaac
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
2 participants
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.
Changes
Replace
POST /api/2.0/workspace-files/import-file/{path}with the multipartvariant of
POST /api/2.0/workspace/import(the SDK'sWorkspace.Uploadwithformat=AUTO) for all bundle file uploads. Single call site:WorkspaceFilesClient.Writeinlibs/filer.Error handling in
Writenow branches on SDK sentinels instead of raw statuscodes and message parsing. Overwrite/type-mismatch collisions are detected via
the AIP-193
ErrorInforeasonWORKSPACE_OBJECT_TYPE_MISMATCH, with amessage-substring fallback for workspaces where that detail (WP-6031) has not
rolled out.
Reimplements #5165 (@shreyas-goenka) on current
main. That draft predatesseveral tests and fixtures that also assert on the upload request shape, so this
PR additionally migrates those and adds the changelog fragment.
Why
/workspace-files/import-fileis deprecated./workspace/importis documented at 30 requests/sec per workspace(API rate limits), higher than the limit that applied to the
previous endpoint, and is ~1.5–2× faster for a typical bundle deploy.
Why multipart
The JSON form of
/workspace/importsends content base64-encoded in acontentfield capped at 10 MB (
MAX_NOTEBOOK_SIZE_EXCEEDEDabove it; see theworkspace.Importfield description in.codegen/cli.json). The multipart formposts the bytes as a file part and accepts the payload sizes
import-filedid.Since
format=AUTOlets the server classify each payload, the applicable limitfollows the classification:
format=AUTO.py/.sql/.scala/.rwith aDatabricks notebook sourceheader)Notebook limits are enforced for both endpoints, so the maximum uploadable
notebook size is unchanged.
Testserver
/workspace/importbody and mirrors the real endpoint'sformat=AUTOnotebook detection for.py/.sql/.scala/.r(header comment→ NOTEBOOK, otherwise FILE), and returns the collision error shape the CLI
branches on.
acceptance/internal/prepare_server.gorecords multipart bodies in anormalized form so recorded requests stay deterministic and reviewable.
bodyContainsmatch(
fault.py --body-contains SUBSTR). Every upload now shares one method+path,so a single file's upload can only be targeted by its multipart
pathfield.Tests
bundle/sync-upload-edge-cases: a 12 MiB binary (over the10 MB JSON cap), an empty file, notebooks in three languages, a header-less
.py(stored as FILE), a.lvdash.jsondashboard, and non-ASCII / spacedfilenames. Asserts object type/language per upload and
format=AUTOon all.shapes that carry no
error_code(bare 409, and 400 +already exists.).libs/testserver/fault_test.gocovers the newbodyContainsmatch.jqassertions re-aimed from.pathto.body.multipart_form.path.Notes
rolled out, and one of its two messages comes from webapp rather than WCS, so
it cannot be dropped on WCS rollout alone.
This pull request and its description were written by Isaac.