Replace error message matching with structural error checks in apps dev and DBFS filer#5517
Open
simonfaltum wants to merge 2 commits into
Open
Replace error message matching with structural error checks in apps dev and DBFS filer#5517simonfaltum wants to merge 2 commits into
simonfaltum wants to merge 2 commits into
Conversation
Two sites branched on err.Error() content. cmd/apps/dev.go now matches the Apps API 404 via errors.Is with apierr.ErrNotFound, and the DBFS filer Read re-stats the path to detect directories instead of matching the SDK's error message. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Approval status: pending
|
Collaborator
|
Commit: 436aee8
22 interesting tests: 15 SKIP, 7 RECOVERED
Top 30 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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.
Why
Found during a full-repo review of the CLI. Two places branched on the text of
err.Error(), which silently breaks the moment the upstream wording changes:cmd/apps/dev.gomatched "does not exist" and "is deleted" to detect a missing app.libs/filer/dbfs_client.gomatched "cannot open directory for reading" to detect a directory read.Changes
Before, both sites compared error message strings; now they use structural checks that keep working if the wording changes.
cmd/apps/dev.go: the Apps API reports a missing or deleted app with HTTP 404 ("App with name X does not exist or is deleted."), sodev-remotenow checkserrors.Is(err, apierr.ErrNotFound)via a smallisAppNotFoundhelper. Note the 404 carries error codeNOT_FOUND, which the SDK maps to a sentinel by status code only, soapierr.ErrResourceDoesNotExistwould not match here.libs/filer/dbfs_client.go: the SDK'sDbfs.Openstats the path and fails with a plain client-side error (not an*apierr.APIError, no sentinel) when it is a directory.Readnow re-stats the path on that non-API-error path and mapsIsDirto the typednotAFileerror, mirroring howFilesClient.Readdisambiguates with a stat call. The extra stat only happens on the rare error path.Test plan
TestIsAppNotFoundcovering the exact 404/NOT_FOUND shape the Apps API returns (direct and wrapped), plus 403 and non-API errors.TestDbfsClientReadDirectoryandTestDbfsClientReadFileDoesNotExistagainst a test server, asserting thefs.ErrInvalidandfs.ErrNotExistmappings.go test ./libs/filer ./cmd/appspasses../task fmt-q,./task lint-q, and./task checkspass.This pull request and its description were written by Isaac.