fix(apps): detect the no-database failure by code or message - #2217
fix(apps): detect the no-database failure by code or message#2217chenxingyang1019 wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe app error flow detects database initialization failures through current and legacy codes or matching server messages. ChangesDatabase error detection and recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/common_test.go`:
- Around line 98-135: Extend the affected tests around the legacy, unknown-code,
and unrelated-failure cases to assert the resulting error’s expected Category
and Subtype, retaining each input error for comparison. Add a wrapped-cause
scenario and verify the error returned through errs.ProblemOf preserves that
cause chain, without asserting a nonexistent Param field.
In `@tests/cli_e2e/apps/apps_env_pull_live_test.go`:
- Around line 31-33: Remove the LARKSUITE_CLI_CONFIG_DIR requirement from the
setup of this live workflow test. Use the established live-token gate for
skipping unavailable credentials, while retaining
LARK_CLI_E2E_APPS_ENV_PULL_APP_ID as the fixture gate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3863f1f6-3e80-4aa0-8922-c48d80da1c81
📒 Files selected for processing (4)
shortcuts/apps/common.goshortcuts/apps/common_test.gotests/cli_e2e/apps/apps_env_pull_live_test.gotests/cli_e2e/apps/coverage.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2f8263080a2b012064c4a54b20c99404b0626f1c🧩 Skill updatenpx skills add larksuite/cli#fix/apps-no-database-error-match -y -g |
The recovery flow for "db command against an app that has no database"
keyed on one business code (500002759). The server has since renumbered
that case to 400002465, which silently disabled the flow: users now see
the raw internal message ("get workspace id failed by app id") and lose
the cloud-development recovery steps entirely.
Nothing caught the regression. There is no compile error, the unit tests
compare against the same constant they set, and the dry-run E2E never
reaches a live server — the failure is only observable by running a db
command against a real database-less app.
Detect on code OR message instead. Both known codes are kept, plus narrow
lowercase markers of the server's internal wording. The two channels have
opposite failure modes: a code is precise but gets renumbered, a message
survives renumbering but breaks on rewording or localization. Requiring
either to match means one channel changing degrades nothing, and only a
simultaneous change of both regresses.
Markers stay deliberately narrow. "no db branch" in particular must not
also swallow env-pull's "invalid db branch" case, which needs its own
hint; a comment records that widening them requires a test proving the
neighbours still pass through.
Verified on BOE against an app with no database: the rewritten message and
the recovery hint are both restored. Negative controls confirm the
numerically adjacent 400002469 ("table does not exist") and an unrelated
permission failure keep their own hints.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2217 +/- ##
==========================================
+ Coverage 76.08% 76.22% +0.13%
==========================================
Files 983 986 +3
Lines 103429 104278 +849
==========================================
+ Hits 78692 79482 +790
Misses 18752 18752
- Partials 5985 6044 +59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d4162da to
4d1a04a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
chenxingyang1019
changed the title
Review feedback: the new subtests checked only Message and Hint, so a change that reclassified the failure — or replaced the error value and dropped the cause chain — would still have passed. Each case now asserts Category, Subtype and Code are untouched by the rewrite, and that the helper returns the same error value. Inputs use a concrete subtype rather than Unknown, so a clobbered classification is actually observable. One new case wraps a cause and asserts errors.Is still finds it through the rewrite. Also covers the predicate's defensive nil guard, which withAppsHint cannot reach on its own (ProblemOf returns ok=false for untyped errors), closing the two uncovered lines codecov flagged. Both withAppsHint and isAppNoDatabaseError are now at 100%.
|
Addressed the two review threads:
Also closed the codecov gap the report flagged (2 uncovered lines): |
1 participant
Why
withAppsHintkeyed the "this app has no database yet" special case on a single business code,500002759. The server has since renumbered that case to400002465, so the branch no longer fires. Observed on BOE against a database-less app:messagethis app does not have a database yetget workspace id failed by app idhintThe raw message is internal vocabulary (workspace ↔ app-id mapping) — rewriting it is precisely why the special case exists. Worse, the generic hint suggests
+db-env-create, which is not a valid next step for an app that has no database at all.Nothing caught this: no compile error, and the unit tests compare against the same constant they set, so they pass either way. The dry-run E2E never reaches a live server, so the only way to observe it is running a db command against a real database-less app.
What changed
Detect on code OR message. Both known codes are kept (
400002465current,500002759legacy), plus narrow lowercase markers of the server's internal wording. The channels have opposite failure modes — a code is precise but gets renumbered, a message survives renumbering but breaks on rewording or localization — so requiring either means one channel changing degrades nothing, and only a simultaneous change of both regresses.Markers stay deliberately narrow.
"no db branch"must not swallow env-pull's"invalid db branch"case, which needs its own hint; a comment records that widening them requires a test proving the neighbours still pass through.Verification
Unit:
go test ./shortcuts/apps/— 3 new subtests cover the legacy code, an unknown code matched by message (case-insensitive and substring), and negative controls proving"invalid db branch: dev","数据表格不存在"and"permission denied"keep the caller's hint.Live, on BOE lane
boe_lark_cli_db_file:400002469("table does not exist") → not hijacked, keeps its own hint+db-table-list→ unaffectedgo build ./...andgo test ./tests/cli_e2e/apps/pass. The BOE db regression suites (cli_e2e.sh,db_data_e2e.sh) were run against this build; see the review note below on their 3 pre-existing failures.Notes for review
type/subtype/code/log_id) and the cause chain are untouched, as the existing tests assert.apps_env_pull.goandapps_env.godiscriminate purely on English message text, which is the mirror-image fragility — a server rewording or a switch to Chinese silently disables them.Summary by CodeRabbit
Bug Fixes
Tests