fix: fix check of auth options required#1346
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors CLI authentication handling to make it more declarative and type-safe. Previously, all commands shared a single commonOptions object and manually hid unsupported auth options, leading to inconsistent validation and unclear error messages. The new approach uses dedicated auth helper functions that explicitly define which auth strategies each command supports.
Changes:
- Introduced
connectionOptions.tswith three auth helper functions (withPasswordAuth,withApiTokenAuth,withEitherAuth) that encapsulate auth validation logic - Updated all CLI commands to use the appropriate auth helper function instead of manually hiding options
- Moved auth validation from runtime handlers to declarative yargs
check()functions - Added comprehensive unit and integration tests
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli/connectionOptions.ts | New module defining connection options and auth helper functions with validation |
| src/cli/commonOptions.ts | Removed in favor of the new modular approach |
| src/cli/record/import.ts | Now uses withEitherAuth and guestSpaceOptions |
| src/cli/record/export.ts | Now uses withEitherAuth and guestSpaceOptions |
| src/cli/record/delete.ts | Now uses withApiTokenAuth, removed runtime validation |
| src/cli/plugin/upload.ts | Now uses withPasswordAuth, removed guestSpaceId parameter |
| src/cli/customize/apply.ts | Now uses withPasswordAuth and guestSpaceOptions |
| src/cli/customize/export.ts | Now uses withPasswordAuth and guestSpaceOptions |
| src/cli/tests/connectionOptions.test.ts | New unit tests for auth helper functions |
| src/cli/tests/commands.test.ts | New integration tests verifying auth validation in commands |
| src/tests/api.test.ts | Removed redundant test for auth prioritization (now handled at CLI layer) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/cli/connectionOptions.ts
Outdated
| * Either (username AND password) or API token is required. | ||
| * When both are provided, password auth takes priority (api-token is cleared). | ||
| */ | ||
| export const withEitherAuth = (args: yargs.Argv) => { |
There was a problem hiding this comment.
好みもあるけど、ビルダーパターンでも良さそうに思いました。
サポートする認証方法が増えたときとか(例: OAuth追加)、プロキシなどのネットワーク系のオプションを個別に有効無効にしたくなった場合の改修コストが抑えやすそうです。
function buildConnectionOptions(
{
auth: ("password"|"apiToken")[], // 複数指定された場合はいずれか必須になる
guestSpace?: boolean // (デフォルトfalse)
}
)There was a problem hiding this comment.
あー確かにその方が良さそうですね
ちょっとそういう方針で調整してみます
Why
Previously, all commands shared the same
commonOptionswhich included all auth options (username, password, api-token, etc.) regardless of whether the command supported them. Commands had to.hide()unsupported options and perform runtime validation in handlers, which was error-prone and provided unclear feedback to users.What
commonOptionswith a newconnectionOptions.tsmodule that separates auth strategies:withPasswordAuth: For commands requiring username/password auth (plugin upload, customize apply/export)withApiTokenAuth: For commands requiring API token auth (record delete)withEitherAuth: For commands accepting either username/password or API token (record import/export)guestSpaceOptions: Separated guest space options, applied only to commands that need themcheck()for declarative validation, removing runtime checks from handlersmiddlewareto prioritize password auth when both auth types are providedconnectionOptions.test.ts) and command integration tests (commands.test.ts)How to test
pnpm testto verify unit tests passrecord import --base-url ... --app 1 --file-path ...→ Either username or API token is requiredrecord delete --base-url ... --app 1 --yes→ API token is requiredplugin upload --base-url ... --input ...→ Username and password are requiredChecklist
pnpm lintandpnpm teston the root directory.