testserver: reject empty catalog and MLflow model names - #6157
Merged
Merged
Conversation
Collaborator
Integration test reportCommit: 2b277c0
11 interesting tests: 4 RECOVERED, 4 SKIP, 3 flaky
Top 29 slowest tests (at least 2 minutes):
|
radakam
changed the title
radakam
marked this pull request as ready for review
August 5, 2026 10:41
The fake server stored a catalog or registered model created with an empty name, which the CLI could then never find again, so a deploy appeared to succeed and the next plan saw the resource as missing. Both real backends reject it. Return the same 400 they do, verified against a real workspace.
Add acceptance tests showing that bundle deploy with an empty catalog or MLflow model name now fails with the backend error instead of silently succeeding, and use http.StatusBadRequest in catalogs.go for consistency with the rest of the file.
…echo The positive tests asserted the name echoed back in the create response, which holds even if create stores the resource under a key nothing can look up - the bug this change exists to prevent. Read the resource back instead, through the same MapGet the catalogs GET route uses and through ModelRegistryGetModel, so a mis-keyed store fails the test. The rejection tests now also assert the collection stays empty, pinning the symptom directly: a stored-but-unreadable entry is what made a deploy look successful and the next plan see the resource as missing.
These goldens assert the UC and MLflow rejection messages verbatim, so their value depends on those strings still matching the real backends. With Cloud = false nothing ever checked that, and the fake could drift out of sync silently. Enable cloud runs so the integration suite catches it, matching the catalogs/with-schemas sibling. The local goldens are unchanged; only out.test.toml moves.
The cloud runs deployed to a fixed ~/.bundle/<bundle-name>/default, so matrix legs sharing a workspace raced on one root path, and the uploaded files carried no run prefix for cleanBundles to find. Move the bundle name into a template with $UNIQUE_NAME the way the sibling cloud tests do; the deploy fails, but only after the files are uploaded, so there is something to sweep. The models deploy reaches the create call on terraform too and surfaces the same backend message wrapped in terraform's own output, so run both engines and split the deploy output into per-engine goldens rather than dropping terraform. Catalogs stay direct-only, but for the actual reason: terraform rejects catalog resources before any API call, rather than merely formatting the error differently. Assert that the rejected resource was not stored before casting the error body. The cast aborts the test when the rejection is missing, which is exactly the case where the mis-keyed entry is worth reporting.
Trim every comment this branch adds to at most two lines, keeping the reason and dropping the restatement. Two are more than trimming. The catalog engine comment claimed catalogs are "only supported by the direct engine, which rejects them before any API call on terraform" - the relative clause pointed at the wrong engine. And the trailing block in the models test.toml explained an EnvMatrix that file does not set, so it moves to the script, next to the per-engine redirect it actually justifies.
radakam
force-pushed
the
testserver-reject-empty-resource-names
branch
from
August 5, 2026 11:49
bfc8c80 to
2b277c0
Compare
denik
approved these changes
Aug 5, 2026
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
3 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
The catalog and MLflow registered-model fakes now reject an empty
namewith the 400 the real backends return, message verbatim from a real workspace.Why
The fakes stored a resource created with an empty name under that empty key, so the CLI could never read it back: the deploy looked successful and the next plan saw the resource as missing.
Found by fuzz testing.
Tests
Unit tests per endpoint, plus acceptance tests where
bundle deploywith an empty catalog / model name now fails with the backend error. They run against a real workspace too, so the goldens keep the verbatim UC and MLflow messages honest.