Fix WorkspaceFilesClient.Stat() silently swallowing non-404 API errors#4994
Open
Fix WorkspaceFilesClient.Stat() silently swallowing non-404 API errors#4994
WorkspaceFilesClient.Stat() silently swallowing non-404 API errors#4994Conversation
The grants converter for registered_model incorrectly set grants.Function instead of grants.Model, causing Terraform grants to target a function instead of the registered model. Task: 001.md Co-authored-by: Isaac
The direct engine's grantResourceToSecurableType map also mapped registered_models to "function" instead of "model". Update the acceptance test script and outputs to use the correct securable type. Task: 001.md Co-authored-by: Isaac
After fixing the grants field from Function to Model, Terraform will store "model" instead of "function" in state, and the migrated direct state will also use "model" as the securable type. Note: these outputs need to be regenerated on cloud to confirm. Task: 001.md Co-authored-by: Isaac
Change assert.NotEmpty(t, resource.Function) to assert.NotEmpty(t, resource.Model) in TestBundleToTerraformRegisteredModelGrants to match the actual grants field being set for registered model resources. Task: 002.md Co-authored-by: Isaac
…able type Investigation during task 003 revealed that the Unity Catalog API uses FUNCTION as the securable type for registered model grants. The SDK's SecurableType enum has no MODEL value. The Terraform provider maps the model attribute to FUNCTION internally, producing different state IDs (model/... vs function/...) that break migration and direct engine API calls. Reverting all changes from tasks 001, 002 as the original code was correct. Task: 003.md Co-authored-by: Isaac
Stat() checked for 404 status to return a file-not-found error, but fell through to return (zero-value, nil) for any other API error (403, 500, etc.). This caused callers to proceed with bogus file metadata instead of seeing the real error. Task: 001.md Co-authored-by: Isaac
Co-authored-by: Isaac
Verify that non-404 API errors (e.g. 403 PERMISSION_DENIED) from workspace/get-status are surfaced to the user rather than silently swallowed. Before the fix in commit 58849af6b, Stat() returned (nil, nil) on non-404 API errors, causing a nil-pointer panic in the caller. Co-authored-by: Isaac
The previous comment incorrectly stated that the bug caused a nil pointer panic. In reality, since wsfsFileInfo is a value type (not a pointer), Stat() returned a zero-value struct with nil error. This caused export-dir to silently report success without actually exporting anything — worse than a panic because the user doesn't realize the operation failed. Task: 004.md Co-authored-by: Isaac
…st, consolidate unit test - Remove changelog entry (per task request) - Remove acceptance test for export-dir-api-error-propagation - Merge two test functions into a single table-driven test with 3 entries Task: 005.md Co-authored-by: Isaac
Co-authored-by: Isaac
Replace the table-driven TestWorkspaceFilesClientStatErrorHandling with three focused test functions (TestWorkspaceFilesClientStatForbidden, TestWorkspaceFilesClientStatInternalError, TestWorkspaceFilesClientStatNotFound) and a shared statWithError helper. Task: 006.md Co-authored-by: Isaac
Task: 007.md Co-authored-by: Isaac
Co-authored-by: Isaac
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
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
1 participant
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
Fix
WorkspaceFilesClient.Stat()to propagate non-404 API errors instead of returning zero-value file info with nil error.Why
When
Stat()received a non-404 error (e.g. 403 Forbidden, 500 Internal Server Error), it fell through the error handling and returned(zero-value, nil). This caused callers likeworkspace export-dirto silently succeed without exporting anything, and template init to incorrectly report that files already exist.Tests
Added unit tests covering 403, 500, and 404 error responses from the workspace get-status API.