From 2b4cd562a9e57aee9d8a6329fe564f00203d766d Mon Sep 17 00:00:00 2001 From: Wojtek Wierzchowski Date: Fri, 11 Sep 2026 11:11:09 +0200 Subject: [PATCH] [RAPTOR-20140] fix(filesapi): name the File Registry entry a sync creates The Files API takes an entry's name only when the entry is created, and the CLI was never sending one, so what the Registry showed was an accident of which upload route ran. A small change set went through the stage route and landed as 'Untitled Dataset', a class-constant fallback; anything over 20 files or 50 MB went through the zip route and landed as 'wapi-sync.zip', the CLI's own temporary filename, because the platform titles an entry after the file it was built from when nothing else names it. Neither says whose code it holds, and the Registry accepts duplicate names without complaint, so they stacked up as identical rows. Both routes now send 'Artifact: ', as JSON on the create call and as a multipart form field on the zip one, since the Files API reads a POST's parameters from the parsed body and drops them from the query. The id and not the artifact's name, because naming is create-time only: the entry keeps whatever it was created with, so a name the project is free to change would go stale in place, and artifact names are not unique to begin with. The artifact's own id and not its repository's, since a repository is a separate entity in the Workload API. Verified on staging across both routes. --- CHANGELOG.md | 1 + cmd/artifact/code/checkout/cmd_test.go | 6 +- cmd/artifact/code/versions/cmd_test.go | 6 +- docs/commands/artifact.md | 1 + internal/drapi/filesapi/catalogname.go | 42 +++++++ internal/drapi/filesapi/client.go | 5 +- internal/drapi/filesapi/client_test.go | 97 +++++++++------- internal/drapi/filesapi/fromfile.go | 22 +--- internal/drapi/filesapi/multipart.go | 5 + internal/drapi/filesapi/stage.go | 8 +- internal/drapi/filesapi/types.go | 16 +++ internal/workload/sync/engine.go | 55 +++++++++ internal/workload/sync/engine_test.go | 152 +++++++++++++++++++++++-- internal/workload/sync/upload_stage.go | 2 +- internal/workload/sync/upload_zip.go | 30 ++++- 15 files changed, 356 insertions(+), 92 deletions(-) create mode 100644 internal/drapi/filesapi/catalogname.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e1c5cdc09..0fb3177d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - The `dr workload config` setup wizard, which `dr workload up` also opens for a project with no manifest, no longer shows a fresh directory's suggested name as one you typed. Choosing another directory on the way back, or setting up a new workload after looking at an existing one, rebuilds the draft from this run's flags, but the mark that says the name was given by hand outlived the draft it belonged to, so the new directory's suggestion came back filled in as a value. One reflexive Enter could then put a name like `src` on a deployed workload, which is what the name screen exists to prevent. The draft and that mark now start over together, so the suggestion is offered as a placeholder again; a `--name` still counts as given, because that answer belongs to the run rather than to the draft being replaced. - `dr workload up` now deploys onto an errored workload instead of refusing it, whenever the deploy gives it something new to run: a code change, a change to the manifest or the sizing, or `--force-build`. What the workload is running has failed, so there is nothing serving for the swap to endanger, and the plan says so with the platform's own reason for the failure beside the state. A deploy with nothing new in it is still refused, and the refusal names what would work: `--force-build` for a project whose image the platform builds, a change to the image or artifact otherwise, and the delete as the last resort. An errored workload never has its image copied forward, since on a locked version the copy would be locked too, permanently, pointing at an image the registry may no longer have; the version is built instead. A stopped workload whose start comes up errored ahead of a roll is rolled anyway, and the name conflict on an errored holder advises binding to it again. Together these close a deadlock in which a locked production workload whose image the registry had pruned could be neither deployed onto, nor started, nor rebuilt. - `dr workload up --force-build` now rebuilds and rolls even when the working tree and the manifest have not changed, which is the case the flag exists for: an image gone from the registry leaves both exactly as they were. The flag used to be read only by the build step, so a plan with nothing else in it came out empty and the run reported the workload as up to date. A forced build on a locked version locks its successor to match; on a manifest that names its image rather than building it the flag stays idle and says so. +- `dr artifact code sync` and `dr workload up` now name the File Registry entry they create `Artifact: `, instead of leaving it on whatever the platform picked. The Files API takes a name only at creation and the CLI was never sending one, so what the Registry showed was an accident of which upload route ran: a change set small enough for the stage route landed as `Untitled Dataset`, and anything over 20 files or 50 MB landed as `wapi-sync.zip`, the CLI's own temporary filename, because the platform derives a title from the uploaded file when nothing else names it. Neither says whose code it is, and since the Registry accepts duplicate names without complaint, a tenant ended up with a column of identical rows and no way to tell them apart short of a hand rename. The artifact's name is deliberately not what goes in: naming is create-time only, so whatever goes in is what the entry shows for the rest of its life, and a name the project is free to change afterwards would end up telling a reader something that had stopped being true, which is the same defect as the default titles and harder to spot. It would not identify the row either, since nothing stops two projects from choosing the same artifact name and `dr workload up` derives that name from the directory. The id names the artifact that created the entry, which is not always the one deploying from it later: every successful `up` locks the artifact it deployed and mints a fresh one for the next deploy, while the catalog stays where it is. A locked artifact is kept rather than deleted, so the id goes on resolving and goes on being true about where the code came from, naming the first version of the lineage rather than the current one. A large first sync now creates its catalog before uploading, rather than in the same request: the name travels on the create call, which is the only route that tolerates it everywhere, since that route took no request body before the parameter existed while the create-from-file route validates its form strictly and would reject an unknown field outright. The cost is one extra request on a first sync, and it makes the two upload paths the same shape. - `dr artifact code sync` and `dr workload up` now replace files in the catalog when the upload takes the zip route, which is any change set of more than 20 files or 50 MB. The overwrite mode was sent only as a query parameter of the Files API's `fromFile` upload, which the server accepts and ignores, so its rename default applied: every path already in the catalog came back as a `name (2).ext` duplicate holding the new bytes while the original kept the old ones, the version had twice the files, and an image built from it ran stale code next to junk. Smaller change sets, which go through a stage, were never affected. The mode now travels in the multipart form, ahead of the file, where the server reads it; it is still sent in the query as well until the API says which of the two is authoritative. - `dr workload up` now says when `.env` and the manifest have parted, instead of reporting a deploy as up to date without having looked at the file you just edited. Nothing is applied and nothing is written: a deploy stays a function of the committed repo, and reading the file to say the two disagree is not deploying from it. The notice is the one `dr workload config` has printed all along, names only and never values, and it names the flag that settles it on the command you are already running rather than sending you to a different one, carrying the `--dir` this run was given because `config` looks only where it is pointed while `up` walks upward for the manifest. Silent where there is no `.env`, which is the ordinary CI case, and silent where the two files agree. What is left to `config` is everything that is true of the project rather than of this run: the values behind credential references, which nothing can compare; the names the classifier read as local-only, which no flag will ever add; an entry left naming the credential placeholder, which the deploy refuses for itself when it reaches it; and a manifest whose shape no flag can edit, whose refusal counts the whole of `.env` as missing. None of those can be settled by anything the reader is about to run, so on a deploy each would print on every run for the life of the project, and a line that always prints is one you stop reading along with the drift beside it. - `dr workload up --update-env` and `dr workload config --update-env` now name the `.env` variables the manifest does not declare even when the run rewrote something. The notice used to arrive only on the run that found nothing to do, so reconciling every value you could left the file reading as a clean bill of health while a variable it had never carried went on reaching nothing. diff --git a/cmd/artifact/code/checkout/cmd_test.go b/cmd/artifact/code/checkout/cmd_test.go index 860a61940..a5bb9d262 100644 --- a/cmd/artifact/code/checkout/cmd_test.go +++ b/cmd/artifact/code/checkout/cmd_test.go @@ -103,17 +103,13 @@ func (f *fakeClient) callCounts() (all, dl int) { } // Panicking stubs for the rest of filesapi.Client. -func (*fakeClient) CreateCatalog() (*filesapi.CatalogResp, error) { panic("unused") } +func (*fakeClient) CreateCatalog(string) (*filesapi.CatalogResp, error) { panic("unused") } func (*fakeClient) CreateStage(string) (*filesapi.StageResp, error) { panic("unused") } func (*fakeClient) UploadToStage(string, string, string, int64, io.Reader) error { panic("unused") } func (*fakeClient) ApplyStage(string, string, string) (*filesapi.ApplyStageResp, error) { panic("unused") } -func (*fakeClient) UploadFromZipNew(string, int64, io.Reader) (*filesapi.FromFileResp, error) { - panic("unused") -} - func (*fakeClient) UploadFromZipExisting(string, string, string, int64, io.Reader) (*filesapi.FromFileResp, error) { panic("unused") } diff --git a/cmd/artifact/code/versions/cmd_test.go b/cmd/artifact/code/versions/cmd_test.go index 864360b5c..5870c2542 100644 --- a/cmd/artifact/code/versions/cmd_test.go +++ b/cmd/artifact/code/versions/cmd_test.go @@ -50,7 +50,7 @@ func (f *fakeClient) ListVersions(catalogID string, limit int) ([]filesapi.Catal } // Unused interface methods. -func (*fakeClient) CreateCatalog() (*filesapi.CatalogResp, error) { panic("unused") } +func (*fakeClient) CreateCatalog(string) (*filesapi.CatalogResp, error) { panic("unused") } func (*fakeClient) CreateStage(string) (*filesapi.StageResp, error) { panic("unused") @@ -64,10 +64,6 @@ func (*fakeClient) ApplyStage(string, string, string) (*filesapi.ApplyStageResp, panic("unused") } -func (*fakeClient) UploadFromZipNew(string, int64, io.Reader) (*filesapi.FromFileResp, error) { - panic("unused") -} - func (*fakeClient) UploadFromZipExisting(string, string, string, int64, io.Reader) (*filesapi.FromFileResp, error) { panic("unused") } diff --git a/docs/commands/artifact.md b/docs/commands/artifact.md index e5ed63ca8..8db625acf 100644 --- a/docs/commands/artifact.md +++ b/docs/commands/artifact.md @@ -176,6 +176,7 @@ dr artifact code checkout [] [--dir ] [--clean] - `init` creates the `.datarobot/workload/` state directory and binds it to an existing draft artifact. The artifact must already exist (`dr artifact create` or the DataRobot UI); these commands manage an artifact's code, not its lifecycle. It also drops a starter `.drignore` at the project root, in gitignore syntax, listing what `sync` should leave out. Edit it and commit it. A project that already has an ignore file under either name keeps it, and no new one is written. - Projects created before the file was renamed have a `.wapiignore` instead. It is still read when there is no `.drignore` beside it, and `sync` says so once per run, but the name is deprecated: rename the file when convenient. If both are present, `.drignore` is the one that applies and `sync` warns that the other file's patterns are not in effect. Merge them and delete the old one: two ignore files at a project root is a state where patterns you wrote silently stop filtering. The ignore file is uploaded with your code, so if you work with others, agree on the rename rather than doing it alone. - `sync` computes a three-way diff against the last synced state and applies it in one versioned step. Conflicts resolve to the remote copy, and your version is kept as a `*.LOCAL.` file. Preview with `--dry-run`, or use `--diff` to also see per-file diffs. Both exit before any remote write. +- The first `sync` of a project creates the File Registry entry that holds its code, named `Artifact: ` so the row identifies its project in the Registry's Files list. It carries the artifact id rather than the artifact name because naming is create-time only: the entry keeps whatever name it was created with, so a name the project later changes would go stale in place, and artifact names are not unique to begin with (`dr workload up` derives one from the directory). The id is the artifact that first pushed the code; after a deploy locks that artifact and `up` mints its successor, the entry keeps naming the first version of the lineage, which still resolves. Later syncs add versions to the entry and leave its name alone, so an entry created before the CLI sent a name keeps whatever the platform picked (`Untitled Dataset`, or `wapi-sync.zip` if the first sync was large enough to take the zip route). Rename it in the UI, or with `PATCH /api/v2/catalogItems//`; the catalog id is in `.datarobot/workload/config.json`. - For Python projects, the image build requires a `uv.lock` next to `pyproject.toml`. When your project has `pyproject.toml` but no `uv.lock`, `sync` generates one automatically by running your local `uv lock` (your uv configuration, private indexes, and credentials apply) and uploads it with the rest of your code — commit the generated file to your repo. If `uv` is not installed or lock generation fails, sync still completes and prints what to do (`uv lock`, then re-sync); the image build will fail until a lock file is added. This also happens on `--dry-run`/`--diff`, so the preview matches what a real sync would upload. An existing `uv.lock` is never modified, and sync warns if your `.drignore` excludes it. - `versions` lists the artifact's catalog versions, marking the one the artifact currently points at (`*`) and noting the one you last synced. - `checkout` downloads a version into `.datarobot/workload/.checkouts//` for read-only inspection; your working directory is left untouched. `--clean` removes checkout directories instead of downloading. diff --git a/internal/drapi/filesapi/catalogname.go b/internal/drapi/filesapi/catalogname.go new file mode 100644 index 000000000..78a762cc7 --- /dev/null +++ b/internal/drapi/filesapi/catalogname.go @@ -0,0 +1,42 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package filesapi + +// CatalogNameMaxLen is the Files API's ceiling on a catalog entry name, +// counted in characters rather than bytes because the server validates a +// decoded Python string. Overshooting it fails the whole create with a +// 422, so the name is clamped here instead. +const CatalogNameMaxLen = 255 + +// ClampCatalogName trims a name to what the Files API accepts. An empty +// name stays empty and callers leave it off the request, which is what +// puts the platform's own default on the entry. +// +// Exported so a caller assembling a name out of parts measures it the +// same way the server does, rather than keeping a second copy of the rule +// and its reasoning. +// +// Clamping rather than erroring: the name is a label on an entry whose +// contents are the point, and the artifact names it mirrors are allowed +// to be twenty times longer. Refusing the upload over a cosmetic field +// would trade a slightly shortened label for a failed sync. +func ClampCatalogName(name string) string { + runes := []rune(name) + if len(runes) <= CatalogNameMaxLen { + return name + } + + return string(runes[:CatalogNameMaxLen]) +} diff --git a/internal/drapi/filesapi/client.go b/internal/drapi/filesapi/client.go index 892867ffe..ec9d81b6d 100644 --- a/internal/drapi/filesapi/client.go +++ b/internal/drapi/filesapi/client.go @@ -19,12 +19,11 @@ import ( ) type Client interface { - CreateCatalog() (*CatalogResp, error) + CreateCatalog(name string) (*CatalogResp, error) CreateStage(catalogID string) (*StageResp, error) UploadToStage(catalogID, stageID, name string, size int64, body io.Reader) error ApplyStage(catalogID, stageID, overwrite string) (*ApplyStageResp, error) - UploadFromZipNew(name string, size int64, body io.Reader) (*FromFileResp, error) - UploadFromZipExisting(catalogID, name, overwrite string, size int64, body io.Reader) (*FromFileResp, error) + UploadFromZipExisting(catalogID, filename, overwrite string, size int64, body io.Reader) (*FromFileResp, error) PollStatus(statusID string) (*StatusResp, error) AllFiles(catalogID, versionID string) (map[string]FileMeta, error) DownloadFile(catalogID, versionID, path string, w io.Writer) (string, int64, error) diff --git a/internal/drapi/filesapi/client_test.go b/internal/drapi/filesapi/client_test.go index a65813364..78d9f90cf 100644 --- a/internal/drapi/filesapi/client_test.go +++ b/internal/drapi/filesapi/client_test.go @@ -64,20 +64,72 @@ func startServer(t *testing.T, handler http.Handler) *httptest.Server { return srv } +// TestCreateCatalog pins the name onto the JSON body. Sent anywhere else +// it is dropped and the entry lands in the File Registry as "Untitled +// Dataset", which is the whole defect this parameter exists to fix. func TestCreateCatalog(t *testing.T) { + var gotBody map[string]any + startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodPost, r.Method) assert.Equal(t, "/api/v2/files/", r.URL.Path) + assert.NoError(t, json.NewDecoder(r.Body).Decode(&gotBody)) w.WriteHeader(http.StatusCreated) _, _ = w.Write([]byte(`{"catalogId":"cid-1","catalogVersionId":"v0"}`)) })) c := New() - got, err := c.CreateCatalog() + got, err := c.CreateCatalog("my-agent-artifact") require.NoError(t, err) assert.Equal(t, "cid-1", got.CatalogID) assert.Equal(t, "v0", got.CatalogVersionID) + assert.Equal(t, map[string]any{"name": "my-agent-artifact"}, gotBody) +} + +// TestCreateCatalog_NoName checks that an unnamed create sends no name key +// at all rather than an empty string. The server reads the field as `name +// or `, where a blank string is falsy, so "omitted" and +// "empty" are not interchangeable. +func TestCreateCatalog_NoName(t *testing.T) { + var gotBody map[string]any + + startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.NoError(t, json.NewDecoder(r.Body).Decode(&gotBody)) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"catalogId":"cid-1","catalogVersionId":"v0"}`)) + })) + + c := New() + _, err := c.CreateCatalog("") + + require.NoError(t, err) + assert.Empty(t, gotBody, "an unnamed create must not carry a name key") +} + +// TestCreateCatalog_ClampsName covers the one input that would otherwise +// fail the create outright: workload artifact names are allowed to run to +// 5000 characters, the Files API stops at 255. +func TestCreateCatalog_ClampsName(t *testing.T) { + var gotName string + + startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + + assert.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + + gotName, _ = body["name"].(string) + + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"catalogId":"cid-1","catalogVersionId":"v0"}`)) + })) + + c := New() + _, err := c.CreateCatalog(strings.Repeat("é", 300)) + + require.NoError(t, err) + assert.Equal(t, strings.Repeat("é", 255), gotName, + "the limit counts characters, so multi-byte names must not be cut mid-rune") } func TestCreateStage_ApplyStage(t *testing.T) { @@ -480,49 +532,6 @@ func TestUploadFromZipExisting_ContentLengthWithFormFields(t *testing.T) { require.NoError(t, err) } -// TestUploadFromZipNew_HitsFromFileEndpoint locks in the (post-2026-04-30) -// fix that the new-catalog-from-zip path posts to /files/fromFile/ rather -// than /files/. The bare /files/ endpoint silently created an empty catalog -// without extracting the zip, so smoke-tested syncs reported success but -// the remote was empty. -func TestUploadFromZipNew_HitsFromFileEndpoint(t *testing.T) { - startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "/api/v2/files/fromFile/", r.URL.Path) - assert.Equal(t, "true", r.URL.Query().Get("useArchiveContents")) - assert.Contains(t, r.Header.Get("Content-Type"), "multipart/form-data") - - mr, err := r.MultipartReader() - if !assert.NoError(t, err) { - return - } - - part, err := mr.NextPart() - if !assert.NoError(t, err) { - return - } - - assert.Equal(t, "file", part.FormName()) - assert.Equal(t, "wapi-sync.zip", part.FileName()) - - // A new catalog has no paths to collide with, so no overwrite - // field travels: the file is the only part. - _, err = mr.NextPart() - assert.ErrorIs(t, err, io.EOF) - - w.WriteHeader(http.StatusAccepted) - _, _ = w.Write([]byte(`{"catalogId":"new-cid","catalogVersionId":"new-ver","statusId":"sid-new"}`)) - })) - - c := New() - - zipBody := bytes.NewReader([]byte("PK\x03\x04fake-zip")) - resp, err := c.UploadFromZipNew("wapi-sync.zip", int64(zipBody.Len()), zipBody) - require.NoError(t, err) - assert.Equal(t, "new-cid", resp.CatalogID) - assert.Equal(t, "new-ver", resp.CatalogVersionID) - assert.Equal(t, "sid-new", resp.StatusID) -} - // Ensure the package's mime/multipart writer references compile (helps catch // import drift if someone removes the import after extracting helpers). var _ = multipart.ErrMessageTooLarge diff --git a/internal/drapi/filesapi/fromfile.go b/internal/drapi/filesapi/fromfile.go index b8b195286..1d9e9c203 100644 --- a/internal/drapi/filesapi/fromfile.go +++ b/internal/drapi/filesapi/fromfile.go @@ -24,18 +24,6 @@ import ( "github.com/datarobot/cli/internal/drapi" ) -func (c *httpClient) UploadFromZipNew(name string, size int64, body io.Reader) (*FromFileResp, error) { - q := url.Values{} - q.Set("useArchiveContents", "true") - - requestURL, err := drapi.EndpointURL("/files/fromFile/", q) - if err != nil { - return nil, fmt.Errorf("build files url: %w", err) - } - - return uploadZipMultipart(requestURL, nil, name, size, body) -} - // UploadFromZipExisting adds a zip's contents to catalogID as a new version. // // The overwrite mode is sent both as a multipart form field and as a query @@ -45,7 +33,7 @@ func (c *httpClient) UploadFromZipNew(name string, size int64, body io.Reader) ( // duplicate while the original keeps its old bytes. The contract does not // say which location is authoritative, so the query copy stays until it // does; the form field is the one that takes effect today. -func (c *httpClient) UploadFromZipExisting(catalogID, name, overwrite string, size int64, body io.Reader) (*FromFileResp, error) { +func (c *httpClient) UploadFromZipExisting(catalogID, filename, overwrite string, size int64, body io.Reader) (*FromFileResp, error) { if overwrite == "" { overwrite = OverwriteReplace } @@ -62,11 +50,11 @@ func (c *httpClient) UploadFromZipExisting(catalogID, name, overwrite string, si return nil, fmt.Errorf("build fromFile url: %w", err) } - return uploadZipMultipart(requestURL, fields, name, size, body) + return uploadZipMultipart(requestURL, fields, filename, size, body) } -func uploadZipMultipart(requestURL string, fields url.Values, name string, size int64, body io.Reader) (*FromFileResp, error) { - req, err := newStreamingMultipartRequest(requestURL, fields, name, size, body) +func uploadZipMultipart(requestURL string, fields url.Values, filename string, size int64, body io.Reader) (*FromFileResp, error) { + req, err := newStreamingMultipartRequest(requestURL, fields, filename, size, body) if err != nil { return nil, err } @@ -75,7 +63,7 @@ func uploadZipMultipart(requestURL string, fields url.Values, name string, size resp, err := client.Do(req) if err != nil { - return nil, fmt.Errorf("zip upload %s: %w", name, err) + return nil, fmt.Errorf("zip upload %s: %w", filename, err) } defer func() { _ = resp.Body.Close() }() diff --git a/internal/drapi/filesapi/multipart.go b/internal/drapi/filesapi/multipart.go index 18fc5964b..2356cbf57 100644 --- a/internal/drapi/filesapi/multipart.go +++ b/internal/drapi/filesapi/multipart.go @@ -39,6 +39,11 @@ const multipartFormField = "file" // The Files API binds a POST's parameters from the parsed body alone // and drops unrecognized query parameters without complaining, so an // option that has to reach the server travels here and not in the URL. +// That covers the upload's overwrite mode, which is silently lost from a +// query string. A new catalog's name is the counter-case and travels on +// the JSON create call instead: this route validates its form strictly, +// so a field a server does not know fails the whole upload, and re-trying +// without it would mean streaming the archive twice. // // useArchiveContents on the fromFile routes reads like a counter-example // and is not one. It is sent in the query, discarded there like anything diff --git a/internal/drapi/filesapi/stage.go b/internal/drapi/filesapi/stage.go index 677abe05c..a444f72c0 100644 --- a/internal/drapi/filesapi/stage.go +++ b/internal/drapi/filesapi/stage.go @@ -23,7 +23,10 @@ import ( "github.com/datarobot/cli/internal/drapi" ) -func (c *httpClient) CreateCatalog() (*CatalogResp, error) { +// CreateCatalog creates an empty catalog entry, named after name when it +// is non-empty. An empty name leaves the entry on the platform's default, +// which the File Registry shows as "Untitled Dataset". +func (c *httpClient) CreateCatalog(name string) (*CatalogResp, error) { requestURL, err := drapi.EndpointURL("/files/", nil) if err != nil { return nil, fmt.Errorf("build catalog url: %w", err) @@ -31,7 +34,8 @@ func (c *httpClient) CreateCatalog() (*CatalogResp, error) { var resp CatalogResp - if err := drapi.PostJSON(requestURL, "catalog", struct{}{}, &resp); err != nil { + body := CreateCatalogReq{Name: ClampCatalogName(name)} + if err := drapi.PostJSON(requestURL, "catalog", body, &resp); err != nil { return nil, err } diff --git a/internal/drapi/filesapi/types.go b/internal/drapi/filesapi/types.go index 986a24ca4..20e632f3f 100644 --- a/internal/drapi/filesapi/types.go +++ b/internal/drapi/filesapi/types.go @@ -33,6 +33,22 @@ const ( // JSON tags are camelCase: the gateway camelizes Python's snake_case // fields before they reach the wire. +// CreateCatalogReq names a new catalog entry. Name is omitted when empty +// rather than sent blank, because the server reads it as `name or ` and a blank string is falsy there: sending "" would fall through +// to the platform's own title, which is the behaviour this parameter +// exists to replace. +// +// This route is also where the name is safe to send at all. It took no +// request body before the parameter existed, so a server that predates it +// ignores the key and leaves its own default on the entry. The +// create-from-file route validates its multipart form strictly and would +// reject the same name outright, which is why the zip path creates the +// catalog here first rather than naming it on the upload. +type CreateCatalogReq struct { + Name string `json:"name,omitempty"` +} + type CatalogResp struct { CatalogID string `json:"catalogId"` CatalogVersionID string `json:"catalogVersionId"` diff --git a/internal/workload/sync/engine.go b/internal/workload/sync/engine.go index 626f924fc..e29b13c95 100644 --- a/internal/workload/sync/engine.go +++ b/internal/workload/sync/engine.go @@ -306,3 +306,58 @@ func resolveExistingCatalogID(e *Engine) string { return refFromArtifact(e).CatalogID } + +// catalogNameLabel prefixes the id in a new catalog's name, so the File +// Registry row says what the id is rather than leaving a bare hex string +// for a reader to guess at. It names the artifact, which is the object the +// id belongs to; the repository an artifact sits in is a separate entity +// in the Workload API and naming it here would point a reader at the wrong +// kind of thing. +const catalogNameLabel = "Artifact: " + +// newCatalogName is the name to give a catalog this sync creates: the +// artifact pushing the code, so the File Registry row identifies its +// project instead of showing the platform's "Untitled Dataset" or, on the +// zip path, the internal "wapi-sync.zip". +// +// The artifact's name is deliberately not what goes here. Naming is a +// create-time parameter on the Files API, so whatever goes in is what the +// entry shows for the rest of its life, and a name the project is free to +// change afterwards would end up telling a reader something that had +// stopped being true, which is the same defect as the default titles and +// harder to spot. It would not identify the row either, since nothing +// stops two projects from choosing the same artifact name and `dr workload +// up` derives that name from the directory. +// +// The id names the artifact that created the entry, which is not always +// the one deploying from it later: every successful `dr workload up` locks +// the artifact it deployed, so the next deploy mints a fresh one in the +// same repository and repoints the project at it, while the catalog stays +// where it is. A locked artifact is kept rather than deleted, so the id +// goes on resolving and goes on being true about where this code came +// from; it is the first version of the lineage rather than the current +// one. +// +// Only the first sync of a project gets here, so an entry created before +// the CLI sent a name at all keeps whatever the platform gave it. +// +// Returns "" when the artifact is unknown, which leaves the name off the +// request. Phase 1 always fetches the artifact before Phase 5 uploads, so +// that is a guard against future reordering rather than a state a sync +// reaches today. +func newCatalogName(e *Engine) string { + if e.artifact == nil { + return "" + } + + // An artifact the platform gave no id is not a case a sync reaches, since + // the id is how phase 1 fetched it. The name is the only other thing here + // worth showing, and it beats a label with nothing after it. + if e.artifact.ID == "" { + return filesapi.ClampCatalogName(e.artifact.Name) + } + + // Label plus a platform id is nowhere near the API's ceiling, so unlike + // the fallback this needs no budgeting. + return catalogNameLabel + e.artifact.ID +} diff --git a/internal/workload/sync/engine_test.go b/internal/workload/sync/engine_test.go index 17cdeadb1..2129ceaf8 100644 --- a/internal/workload/sync/engine_test.go +++ b/internal/workload/sync/engine_test.go @@ -16,9 +16,11 @@ package sync import ( "errors" + "fmt" "io" "os" "path/filepath" + "strings" stdsync "sync" "testing" "time" @@ -65,14 +67,27 @@ type fakeFilesClient struct { stageID string uploadedFiles map[string][]byte deletedPaths []string - mu stdsync.Mutex + + // zipResp, when set, turns on the zip path; nil keeps it a loud + // error so a test that lands there by accident still fails. + zipResp *filesapi.FromFileResp + + // createdCatalogName records the name the sync asked the Files API to + // give the catalog it created; zippedIntoCatalog, which catalog the zip + // was then added to. + createdCatalogName string + zippedIntoCatalog string + + mu stdsync.Mutex } -func (f *fakeFilesClient) CreateCatalog() (*filesapi.CatalogResp, error) { +func (f *fakeFilesClient) CreateCatalog(name string) (*filesapi.CatalogResp, error) { if f.catalogID == "" { return nil, errors.New("fakeFilesClient.CreateCatalog: no catalogID configured") } + f.createdCatalogName = name + return &filesapi.CatalogResp{CatalogID: f.catalogID, CatalogVersionID: ""}, nil } @@ -114,16 +129,28 @@ func (f *fakeFilesClient) ApplyStage(_, _, _ string) (*filesapi.ApplyStageResp, }, nil } -func (f *fakeFilesClient) UploadFromZipNew(_ string, _ int64, _ io.Reader) (*filesapi.FromFileResp, error) { - return nil, errors.New("fakeFilesClient: UploadFromZipNew not expected") -} +func (f *fakeFilesClient) UploadFromZipExisting( + catalogID, _, _ string, _ int64, body io.Reader, +) (*filesapi.FromFileResp, error) { + if f.zipResp == nil { + return nil, errors.New("fakeFilesClient: UploadFromZipExisting not expected") + } + + f.zippedIntoCatalog = catalogID -func (f *fakeFilesClient) UploadFromZipExisting(_, _, _ string, _ int64, _ io.Reader) (*filesapi.FromFileResp, error) { - return nil, errors.New("fakeFilesClient: UploadFromZipExisting not expected") + if _, err := io.Copy(io.Discard, body); err != nil { + return nil, err + } + + return f.zipResp, nil } -func (f *fakeFilesClient) PollStatus(_ string) (*filesapi.StatusResp, error) { - return nil, errors.New("fakeFilesClient: PollStatus not expected") +func (f *fakeFilesClient) PollStatus(statusID string) (*filesapi.StatusResp, error) { + if f.zipResp == nil { + return nil, errors.New("fakeFilesClient: PollStatus not expected") + } + + return &filesapi.StatusResp{Status: filesapi.StatusCompleted, StatusID: statusID}, nil } func (f *fakeFilesClient) AllFiles(_, _ string) (map[string]filesapi.FileMeta, error) { @@ -143,6 +170,11 @@ func (f *fakeFilesClient) ListVersions(_ string, _ int) ([]filesapi.CatalogVersi return nil, errors.New("fakeFilesClient: ListVersions not expected") } +// testRepoID is the artifact repository the test artifacts belong to. The +// catalog name must not carry it, so it has to be something an assertion +// can look for and fail on. +const testRepoID = "repo-xyz-789" + func initProject(t *testing.T, files map[string]string) string { t.Helper() @@ -159,7 +191,7 @@ func initProject(t *testing.T, files map[string]string) string { } func draftArtifact(id, catalogID, versionID string) *workload.Artifact { - a := &workload.Artifact{ID: id, Name: id, Status: "DRAFT"} + a := &workload.Artifact{ID: id, Name: id, Status: "DRAFT", ArtifactRepositoryID: testRepoID} if catalogID == "" { return a @@ -409,6 +441,60 @@ func TestEngine_Run_FirstSyncStagePath(t *testing.T) { assert.NotEmpty(t, patchedArtifactID, "PatchArtifactCodeRef must be called after upload") assert.Equal(t, "cid-new", patchedCatalogID) assert.Equal(t, "ver-1", patchedVersionID) + + // The catalog this sync created is labelled with the artifact's id, so + // the File Registry row identifies its project instead of showing + // "Untitled Dataset". The id and not the name: see newCatalogName. + assert.Equal(t, catalogNameLabel+"art-abc-123", fake.createdCatalogName) +} + +// TestEngine_Run_FirstSyncZipPathNamesTheCatalog is the large-change-set +// half of the same guarantee. The two upload paths create a catalog +// through different endpoints, so a name wired into only one of them +// leaves every project over the file threshold on "Untitled Dataset". +func TestEngine_Run_FirstSyncZipPathNamesTheCatalog(t *testing.T) { + files := make(map[string]string, StageVsZipFileThreshold+1) + for i := range StageVsZipFileThreshold + 1 { + files[fmt.Sprintf("mod%02d.py", i)] = fmt.Sprintf("x = %d\n", i) + } + + dir := initProject(t, files) + + fake := &fakeFilesClient{ + catalogID: "cid-zip", + zipResp: &filesapi.FromFileResp{ + CatalogID: "cid-zip", + CatalogVersionID: "ver-zip", + StatusID: "sid-zip", + }, + } + + e, err := newWithDeps(dir, Options{Yes: true}, Deps{ + Files: fake, + Artifacts: &fakeArtifactStore{ + GetFn: func(id string) (*workload.Artifact, error) { + return draftArtifact(id, "", ""), nil + }, + }, + Now: time.Now, + }) + require.NoError(t, err) + + t.Cleanup(func() { _ = e.Close() }) + + result, err := e.Run() + require.NoError(t, err) + require.NotNil(t, result) + + assert.Equal(t, "ver-zip", result.NewVersion) + assert.Empty(t, fake.uploadedFiles, "over the threshold the stage path must not run") + + // The zip path names its catalog on the JSON create and then adds the + // archive to it, rather than naming it on the upload: the upload route + // rejects a form field it does not know, so a name there would fail the + // whole sync against a server that predates the parameter. + assert.Equal(t, catalogNameLabel+"art-abc-123", fake.createdCatalogName) + assert.Equal(t, "cid-zip", fake.zippedIntoCatalog) } // The engine is where the deprecation note is picked up, so this is the level @@ -498,3 +584,49 @@ func hashLocal(t *testing.T, dir, rel string) (string, int64, error) { return fileops.HashFile(abs) } + +// TestNewCatalogName_LabelsTheArtifact pins both halves of the name: a +// label saying what the id is, and the artifact's own id rather than its +// name (which the project may change under an entry that cannot follow) or +// its repository (a separate kind of object in the Workload API). +func TestNewCatalogName_LabelsTheArtifact(t *testing.T) { + e := &Engine{ + artifact: &workload.Artifact{ + ID: "art-abc-123", + Name: "name-should-not-appear", + ArtifactRepositoryID: testRepoID, + }, + } + + got := newCatalogName(e) + + assert.Equal(t, "Artifact: art-abc-123", got) + assert.NotContains(t, got, "name-should-not-appear", + "naming is create-time only, so a renamable name would go stale in place") + assert.NotContains(t, got, testRepoID, + "a repository is a different entity; its id here would name the wrong thing") +} + +// TestNewCatalogName_NoID covers an artifact the platform gave no id. +// The name is still worth sending; a label with nothing after it is not. +func TestNewCatalogName_NoID(t *testing.T) { + e := &Engine{artifact: &workload.Artifact{Name: "bare-artifact"}} + + assert.Equal(t, "bare-artifact", newCatalogName(e)) +} + +// TestNewCatalogName_NoIDClampsTheFallback keeps the fallback inside what +// the Files API accepts, since an artifact name may run to twenty times +// the limit. +func TestNewCatalogName_NoIDClampsTheFallback(t *testing.T) { + e := &Engine{artifact: &workload.Artifact{Name: strings.Repeat("a", 400)}} + + assert.Len(t, []rune(newCatalogName(e)), filesapi.CatalogNameMaxLen) +} + +// TestNewCatalogName_NoArtifact pins the guard that leaves the name off +// the request entirely rather than sending a label with no id, which would +// name the entry after nothing at all. +func TestNewCatalogName_NoArtifact(t *testing.T) { + assert.Empty(t, newCatalogName(&Engine{})) +} diff --git a/internal/workload/sync/upload_stage.go b/internal/workload/sync/upload_stage.go index 8949f092b..625121446 100644 --- a/internal/workload/sync/upload_stage.go +++ b/internal/workload/sync/upload_stage.go @@ -58,7 +58,7 @@ func ensureCatalog(e *Engine) (string, error) { return id, nil } - cat, err := e.files.CreateCatalog() + cat, err := e.files.CreateCatalog(newCatalogName(e)) if err != nil { return "", fmt.Errorf("create catalog: %w", err) } diff --git a/internal/workload/sync/upload_zip.go b/internal/workload/sync/upload_zip.go index 444e4a203..2f42112eb 100644 --- a/internal/workload/sync/upload_zip.go +++ b/internal/workload/sync/upload_zip.go @@ -26,6 +26,11 @@ import ( "github.com/datarobot/cli/internal/drapi/filesapi" ) +// syncZipName is the filename the zip travels under. It names the upload, +// not the catalog: the catalog's own name is a separate parameter, and a +// zip whose contents are extracted leaves nothing behind called this. +const syncZipName = "wapi-sync.zip" + // ZipUploader implements the async zip workflow: build a zip locally, // POST it to FilesAPI, poll until terminal. type ZipUploader struct{} @@ -120,14 +125,29 @@ func addToZip(zw *zip.Writer, src, archivePath string) error { return nil } -// postZip dispatches to UploadFromZipNew (first-sync, no catalog) or -// UploadFromZipExisting (subsequent syncs). +// postZip creates the catalog if the project has none, then adds the zip's +// contents to it. +// +// A first sync could instead create and fill in one request, via the +// Files API's create-from-file route, and used to. Creating separately +// keeps the catalog's name on the JSON create call, which is the only +// route where naming is safe against every server: the create-from-file +// route validates its multipart form strictly, so a name reaching a +// server that predates the parameter fails the whole upload, and the +// retry that would paper over it means streaming the entire archive a +// second time. The JSON route ignores what it does not recognize, so +// there the same server just leaves its own default on the entry. +// +// The cost is an extra round trip on first sync and, if the upload then +// fails, an empty catalog with nothing in it. Both are what the stage +// path has always done, so this is one shape rather than two. func postZip(e *Engine, body io.Reader, size int64) (*filesapi.FromFileResp, error) { - if id := resolveExistingCatalogID(e); id != "" { - return e.files.UploadFromZipExisting(id, "wapi-sync.zip", filesapi.OverwriteReplace, size, body) + catalogID, err := ensureCatalog(e) + if err != nil { + return nil, err } - return e.files.UploadFromZipNew("wapi-sync.zip", size, body) + return e.files.UploadFromZipExisting(catalogID, syncZipName, filesapi.OverwriteReplace, size, body) } // waitForCompletion polls until terminal status or ZipPollTimeoutSecs