[RAPTOR-20140] fix(filesapi): name the File Registry entry a sync creates - #913
[RAPTOR-20140] fix(filesapi): name the File Registry entry a sync creates#913wojtekwdr wants to merge 1 commit into
Conversation
|
🎫 Jira: |
Code OwnershipCli Maintainers
Workload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
adamalpi
left a comment
adamalpi
left a comment
There was a problem hiding this comment.
Reasoning and tests are strong; four inline notes, one of which is a rollout decision worth settling before merge.
| // sends a name, and one against a server predating the parameter fails on | ||
| // the unknown key rather than degrading. | ||
| type CreateCatalogReq struct { | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
[High] This makes the first sync of every project fail against a pre-#158141 Files API, for a cosmetic label
This doc states the consequence plainly — "one against a server predating the parameter fails on the unknown key rather than degrading" — and I think that decision is worth surfacing outside a code comment, because it is a new hard version coupling between the CLI and the platform on a core path.
Why it bites in practice rather than in theory:
omitemptydoes not save it.newCatalogNamereturns""only whene.artifact == nil, and the engine's own comment says that never happens ("Phase 1 always fetches the artifact before Phase 5 uploads"), so the key is always sent on a real sync.- A rejection is fatal, not degraded:
drapi.Post(post.go:71-73) turns any non-2xx intoErrFromResp.dr workload upon a fresh project dies at catalog creation with a raw API error, and the user has no way to connect that to a label they never asked for. - The CLI documents self-managed/on-prem instances as a first-class target (
README.md:60,docs/commands/auth.md:429, the "Custom/On-Prem" option indr auth set-url), and those clusters routinely trail cloud. I grepped for any feature detection or minimum-version check in the repo and there is none —docs/commands/start.mdversion-checks the CLI against a template, never the platform against the CLI.
I could not test an old server from here, so whether it 422s or quietly ignores the key is unverified — which is exactly why this is worth confirming rather than assuming. Note the asymmetry while you do: an unknown JSON key is far more likely to be rejected than an unknown multipart field, so CreateCatalog is the exposed route and the zip path probably is not.
Three placements:
- Retry without the name on rejection, localized to
CreateCatalog— every cluster keeps working, costs one wasted request on old servers, and the label falls back toUntitled Dataset, i.e. exactly today's behaviour, so nothing regresses. - Document a minimum platform version in the CHANGELOG and
docs/commands/artifact.md— zero code, but turns a cosmetic improvement into a support constraint operators discover from release notes. - Ship as-is — correct only if every instance this CLI is expected to reach is already past #158141. That is a product call.
My read is that hard failure is the wrong default when the benefit is a label, but it is your call; it just should not be made only here.
There was a problem hiding this comment.
It is the other way round, which changes the fix.
POST /files/ had no validator_class before #158141, just a bare post() that never read the body, so an unknown key there is ignored and the entry keeps the platform default. /files/fromFile/ already had FormValidationMixin and a validator without name, so that is the one that 422s. I put a junk key through both against staging and they reject unknown keys identically today, so the strictness is real; what differs is only which of them was strict back then.
That makes retry the worst option for the route that actually breaks, since re-sending means streaming the whole archive a second time. So the zip path now creates its catalog first and uploads into it, and the name only ever travels on the JSON create. There is no version coupling left to document or retry around. It costs one extra request on a first sync, and the empty-catalog-if-the-upload-fails case is what the stage path has always done, so the two paths are now one shape.
| // That covers the upload's overwrite mode and the name a newly created | ||
| // catalog gets, both of which are silently lost from a query string. | ||
| // | ||
| // useArchiveContents on the fromFile routes reads like a counter-example |
There was a problem hiding this comment.
[Medium] This documents a live silent-failure dependency and defers the one-line fix into code the PR is already rewriting
First: this is a better answer than the one I gave on #898. "The query rule is over-general" was wrong in an interesting way — the query parameter really is dropped, and extraction survives only on a server-side default. Good dig.
But that makes the deferral the problem. By this comment's own account, extraction happens "only because the server's declared form default for that field is already true … a flip of that default would stop extraction with no error." The failure mode is a catalog holding a wapi-sync.zip blob instead of extracted source, an image built from that blob, and a zero exit code — quieter than the overwrite bug #898 just fixed.
And the fix is in code this PR is editing. UploadFromZipNew now builds a fields map at fromfile.go:39 to carry name, and UploadFromZipExisting has had one since #898, so this is fields.Set("useArchiveContents", "true") in two functions already being rewritten, next to a parameter being moved for the identical reason.
"Moving it into the form is a separate change" is the only record that it needs doing — I grepped the repo and there is no ticket, TODO, or issue reference anywhere. Either do it here, or file it and cite the id in this comment so it does not rest on a sentence in a doc block.
There was a problem hiding this comment.
RAPTOR-19915, filed before this PR. It is not in the comment because we do not put ticket ids in code comments, which is why that line reads as bare as it does.
Leaving it rather than folding it in: it is a separate defect with its own ticket, and unlike name it is safe against every server version, since use_archive_contents has been a declared form field for years. Nothing about it is blocked on this change.
…ates 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: <artifact id>', 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.
eb420ba to
2b4cd56
Compare
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @wojtekwdr What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
adamalpi
left a comment
adamalpi
left a comment
There was a problem hiding this comment.
Thanks for the correction on the route strictness — deleting the create-from-file route rather than retrying around it is a better fix than any of the three I offered. Two small leftovers from the restructure inline, neither blocking.
| 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) |
There was a problem hiding this comment.
[Low] The last asymmetry: the zip path holds the catalog id and then returns the server's echo instead
Now that postZip gets catalogID from ensureCatalog, line 72 still returns resp.CatalogID, resp.CatalogVersionID, while the stage path returns its own id and takes only the version from the response (upload_stage.go:52). Returning catalogID here would make the two genuinely one shape — which is the stated goal of this change — and drop a dependency on /files/<id>/fromFile/ echoing catalogId back.
That echo now matters on a first sync, where it did not before: previously the first sync went through the create-from-file route, whose response necessarily carried the id it had just minted, so the existing route's response only ever had to be right for subsequent syncs.
I checked before raising it and this is not a live break — phase5_execute.go:282 already assigns newCatalogID from that same field on every subsequent zip sync in production, so the field is populated. It is robustness and consistency, not a regression.
One note on the test: engine_test.go:466 sets zipResp.CatalogID to "cid-zip", the same value as fake.catalogID, so both implementations pass identically. Making the two differ would pin which one the code actually uses.
| // 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 |
There was a problem hiding this comment.
[Low] The empty-catalog case is parity with the stage path, but it is new to this one
"Both are what the stage path has always done" is accurate about the stage path and worth stating — the wrinkle is that UploadFromZipNew was atomic. No catalog existed unless the upload succeeded, so the zip path is acquiring this failure mode rather than matching one it already had.
phase6_state.go:38 writes cfg.CatalogID only after phase 5 succeeds, so a failed first sync leaves an entry nothing has recorded, and the retry's resolveExistingCatalogID comes back empty and creates another.
What makes it worth a line in this PR specifically: those leftovers now carry an identical Artifact: <id> name, so repeated failures reproduce exactly the indistinguishable column of rows this change exists to remove. Previously they would at least have been the same Untitled Dataset nobody expected to be unique.
Not worth blocking on, and I would not pin the id before the upload — that trades a leak for a wrong binding, which is worse. Just that the trade reads better stated as what it is than as parity.
2 participants
RATIONALE
Code pushed to the File Registry showed up as
Untitled Dataset, or aswapi-sync.zipwhen the change set was big enough to take the zip route, because the CLI never sent a name: one route falls back to a class constant, the other titles the entry after the file it was built from, which is our temp archive. The Registry accepts duplicate names silently, so a tenant ends up with a column of identical rows and no way to tell whose code is whose.The Files API gained an optional
nameon its create routes in DataRobot#158141. This sends it.CHANGES
Entries are named
Artifact: <artifact id>now, on both upload routes: JSON onPOST /files/, a multipart form field onPOST /files/fromFile/, since that API binds a POST from the parsed body and drops query parameters.The id rather than 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 anyway (
upderives one from the directory). The artifact's own id rather than its repository's, since a repository is a different entity. The cost of that is the id names the artifact that first pushed the code rather than the one deploying from it later, which keeps resolving because locked artifacts are retained.Existing entries keep their names. No rename on sync, deliberately: it would mean writing over a name someone set by hand.
Checked on staging against both routes, before and after.
Note
Low Risk
Label-only change on catalog create for code upload paths, with tests and no rename of existing entries; behavior depends on the platform accepting the new
nameparameter.Overview
dr artifact code syncanddr workload upnow set a meaningful File Registry title on first catalog creation instead of leavingUntitled Dataset(stage path) orwapi-sync.zip(large zip uploads).New entries are named
Artifact: <artifact-id>. The sync engine’snewCatalogNameuses the artifact id (not the renamable artifact name or repo id) because Files API naming is create-only; later syncs do not rename existing rows.The Files API client sends that name on both create paths: JSON
nameonCreateCatalog, and anamemultipart field before the file onUploadFromZipNew(query params are ignored). Names are clamped to 255 characters viaclampCatalogName/CatalogNameMaxLen.UploadFromZipNewnow takes separatecatalogNameand zipfilenamearguments.Docs (
CHANGELOG,artifact.md) and tests cover JSON body, zip multipart, omission when empty, clamping, and engine stage vs zip first-sync behavior.Reviewed by Cursor Bugbot for commit eb420ba. Configure here.