diff --git a/CHANGELOG.md b/CHANGELOG.md index ef644ad..bd3f9ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `kasapi-cli databases add` and `… update` now bind disjoint flag + sets (mirroring the ddnsuser slice). The flag names are identical + on both subcommands, but the help text of each reflects its own + action semantics ("initial password" / "required" on add; + "replacement password" on update) and cobra rejects an unknown flag + at parse time. The regenerated `docs/cli/kasapi-cli_databases_*.md` + pages now stop claiming "required for add" on the update help + output. +- `kasapi-cli databases list` renders the `used_database_space` + column with a " MB" suffix as part of the value (matching how the + singular detail view already rendered it) and drops the `USED_MB` + header in favour of a bare `USED` header. List and singular views + now share a single unit-rendering convention. +- The `database` package's `Database.in_progress` JSON/YAML field is + no longer marked `omitempty`, aligning with the majority of read + modules (`mailaccount`, `mailinglist`, `sambauser`, `ftpuser`, + `account`). The KAS API has returned `in_progress` on every + captured fixture row, so the previous omitempty added drift without + shielding callers from a missing key. +- `database.Client.Add`'s domain-level validation now emits per-field + errors ("requires a non-empty password" / "comment") instead of a + single combined message, so callers who hit the domain validator + (rather than the CLI's per-flag required-flag checks) can tell + which field actually broke. `AllowedHosts` is no longer required — + an empty value is the KAS API's documented "any host may connect" + wildcard, not a missing parameter; `kasapi-cli databases add`'s + `--allowed-hosts` flag is therefore optional and the empty-string + wildcard is sent verbatim on the wire. +- `(cli.ConfirmAction).Summary` is now exported (was `summary`), so + tests can pin the rendered prompt (and the per-slice loudness verb) + without instantiating a real terminal. + - `kasapi-cli ddnsusers add` and `… update` now bind disjoint flag sets — `add` carries `--zone` / `--label` / `--target-ip`, `update` carries `--target-ipv4` / `--target-ipv6` instead — so each @@ -36,6 +68,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `database.InProgressFalse` / `database.InProgressTrue` package + constants for the literal `"FALSE"` / `"TRUE"` strings the KAS API + uses to encode the async-write flag, so mapping code and tests + share one source of truth rather than re-typing literals. + - Database write endpoints (#122, #13 write slice): `kasapi-cli databases add --password --comment --allowed-hosts `, diff --git a/docs/cli/kasapi-cli_databases_add.md b/docs/cli/kasapi-cli_databases_add.md index 9fd274b..0e9909f 100644 --- a/docs/cli/kasapi-cli_databases_add.md +++ b/docs/cli/kasapi-cli_databases_add.md @@ -2,17 +2,29 @@ Create a database (add_database; the login is generated by KAS) +### Synopsis + +Create a database via add_database. KAS generates the login and the +command prints it on success. + +--allowed-hosts is optional: an empty value (or omitting the flag) is +the KAS API's documented "any host may connect" wildcard — it is +sent on the wire as an empty database_allowed_hosts parameter, which +the API interprets as unrestricted access. Pass an explicit +comma-separated list of host names / IPs / CIDR blocks to restrict +access. + ``` -kasapi-cli databases add --password --comment --allowed-hosts [flags] +kasapi-cli databases add --password --comment [--allowed-hosts ] [flags] ``` ### Options ``` - --allowed-hosts string comma-separated list of hosts allowed to connect (required for add; e.g. "localhost, 192.168.100.10, 192.168.100.10/32") - --comment string user comment / label (required for add) + --allowed-hosts string comma-separated list of hosts allowed to connect (optional; an empty value is the KAS API's documented "any host may connect" wildcard, e.g. "localhost, 192.168.100.10, 192.168.100.10/32") + --comment string human-readable comment / label for the new database (required) -h, --help help for add - --password string database password (required for add; new password for update) + --password string initial database password (required) ``` ### Options inherited from parent commands diff --git a/docs/cli/kasapi-cli_databases_update.md b/docs/cli/kasapi-cli_databases_update.md index 369ff79..a16663a 100644 --- a/docs/cli/kasapi-cli_databases_update.md +++ b/docs/cli/kasapi-cli_databases_update.md @@ -9,10 +9,10 @@ kasapi-cli databases update [password/comment/allowed-hosts fla ### Options ``` - --allowed-hosts string comma-separated list of hosts allowed to connect (required for add; e.g. "localhost, 192.168.100.10, 192.168.100.10/32") - --comment string user comment / label (required for add) + --allowed-hosts string replacement comma-separated list of hosts allowed to connect + --comment string replacement comment / label -h, --help help for update - --password string database password (required for add; new password for update) + --password string replacement database password (sent as database_new_password) ``` ### Options inherited from parent commands diff --git a/internal/cli/confirm.go b/internal/cli/confirm.go index b3dcc9b..ab15e5f 100644 --- a/internal/cli/confirm.go +++ b/internal/cli/confirm.go @@ -65,7 +65,11 @@ type ConfirmAction struct { ID string // identifier of the target resource } -func (a ConfirmAction) summary() string { +// Summary renders the one-line description shown to the user before +// the [y/N] prompt. Exported so tests can pin the rendered prompt +// (and the loudness verb each slice chose) without instantiating a +// real terminal. +func (a ConfirmAction) Summary() string { return fmt.Sprintf("About to %s %s %q. This cannot be undone.", a.Verb, a.Resource, a.ID) } @@ -87,7 +91,7 @@ func GateDestructive(in io.Reader, out io.Writer, isTTY, yes bool, a ConfirmActi if !isTTY { return UserError(ErrConfirmationRequired, "") } - ok, err := Confirm(in, out, a.summary()) + ok, err := Confirm(in, out, a.Summary()) if err != nil { return UserError(err, "confirm") } diff --git a/internal/cli/databases.go b/internal/cli/databases.go index a06b5be..6f613b1 100644 --- a/internal/cli/databases.go +++ b/internal/cli/databases.go @@ -56,27 +56,28 @@ func newDatabasesGetCmd(opts *RootOptions) *cobra.Command { } } -// databaseWriteFlags binds the shared add_database / update_database -// request fields to a command. The same flag set serves both: add -// reads every value, update sends only the flags the user explicitly -// changed (see databaseChangedFields). The password flag maps to a -// different KAS key per action (add_database: database_password, -// update_database: database_new_password) — see spec() and -// databaseChangedFields. -type databaseWriteFlags struct { +// databaseAddFlags binds the add_database request fields. add and +// update bind disjoint flag sets (same wire-key surface, but the help +// text of each subcommand reflects only its own action semantics, and +// cobra rejects a wrong-subcommand flag at parse time rather than +// silently ignoring it). The password flag maps to database_password +// (database_password is the add-only key; update has its own +// --password flag that maps to database_new_password — see +// databaseUpdateFlags). +type databaseAddFlags struct { password string comment string allowedHosts string } -func (f *databaseWriteFlags) bind(cmd *cobra.Command) { +func (f *databaseAddFlags) bind(cmd *cobra.Command) { fl := cmd.Flags() - fl.StringVar(&f.password, "password", "", "database password (required for add; new password for update)") - fl.StringVar(&f.comment, "comment", "", "user comment / label (required for add)") - fl.StringVar(&f.allowedHosts, "allowed-hosts", "", "comma-separated list of hosts allowed to connect (required for add; e.g. \"localhost, 192.168.100.10, 192.168.100.10/32\")") + fl.StringVar(&f.password, "password", "", "initial database password (required)") + fl.StringVar(&f.comment, "comment", "", "human-readable comment / label for the new database (required)") + fl.StringVar(&f.allowedHosts, "allowed-hosts", "", "comma-separated list of hosts allowed to connect (optional; an empty value is the KAS API's documented \"any host may connect\" wildcard, e.g. \"localhost, 192.168.100.10, 192.168.100.10/32\")") } -func (f *databaseWriteFlags) spec() database.Spec { +func (f *databaseAddFlags) spec() database.Spec { return database.Spec{ Password: f.password, Comment: f.comment, @@ -85,11 +86,20 @@ func (f *databaseWriteFlags) spec() database.Spec { } func newDatabasesAddCmd(opts *RootOptions) *cobra.Command { - f := &databaseWriteFlags{} + f := &databaseAddFlags{} cmd := &cobra.Command{ - Use: "add --password --comment --allowed-hosts ", + Use: "add --password --comment [--allowed-hosts ]", Short: "Create a database (add_database; the login is generated by KAS)", - Args: cobra.NoArgs, + Long: `Create a database via add_database. KAS generates the login and the +command prints it on success. + +--allowed-hosts is optional: an empty value (or omitting the flag) is +the KAS API's documented "any host may connect" wildcard — it is +sent on the wire as an empty database_allowed_hosts parameter, which +the API interprets as unrestricted access. Pass an explicit +comma-separated list of host names / IPs / CIDR blocks to restrict +access.`, + Args: cobra.NoArgs, RunE: runWriteE(opts, func([]string) (writeSpec, error) { if f.password == "" { return writeSpec{}, fmt.Errorf("--password is required") @@ -97,9 +107,6 @@ func newDatabasesAddCmd(opts *RootOptions) *cobra.Command { if f.comment == "" { return writeSpec{}, fmt.Errorf("--comment is required") } - if f.allowedHosts == "" { - return writeSpec{}, fmt.Errorf("--allowed-hosts is required") - } s := f.spec() return writeSpec{ action: "add_database", @@ -120,7 +127,28 @@ func newDatabasesAddCmd(opts *RootOptions) *cobra.Command { return cmd } -// databaseChangedFields collects only the write flags the user +// databaseUpdateFlags binds the update_database mutable surface. +// Disjoint from databaseAddFlags so update's --help describes the +// flags as replacements (not "required for add") and cobra rejects an +// add-only flag at parse time. +// +// The password flag maps to database_new_password on this subcommand +// (the update_database key) rather than the add-only +// database_password — see databaseUpdateChangedFields. +type databaseUpdateFlags struct { + password string + comment string + allowedHosts string +} + +func (f *databaseUpdateFlags) bind(cmd *cobra.Command) { + fl := cmd.Flags() + fl.StringVar(&f.password, "password", "", "replacement database password (sent as database_new_password)") + fl.StringVar(&f.comment, "comment", "", "replacement comment / label") + fl.StringVar(&f.allowedHosts, "allowed-hosts", "", "replacement comma-separated list of hosts allowed to connect") +} + +// databaseUpdateChangedFields collects only the flags the user // explicitly set into the update_database field map (keyed on the // database.Field* constants). Each field is a wholesale replacement // and an empty value is a meaningful set, so presence is keyed on @@ -128,7 +156,7 @@ func newDatabasesAddCmd(opts *RootOptions) *cobra.Command { // the ftpuser/sambauser updates use. The password flag maps to // database_new_password here (update_database's key) rather than the // add-only database_password. -func databaseChangedFields(cmd *cobra.Command, f *databaseWriteFlags) map[string]string { +func databaseUpdateChangedFields(cmd *cobra.Command, f *databaseUpdateFlags) map[string]string { fields := map[string]string{} if cmd.Flags().Changed("password") { fields[database.FieldNewPassword] = f.password @@ -143,7 +171,7 @@ func databaseChangedFields(cmd *cobra.Command, f *databaseWriteFlags) map[string } func newDatabasesUpdateCmd(opts *RootOptions) *cobra.Command { - f := &databaseWriteFlags{} + f := &databaseUpdateFlags{} cmd := &cobra.Command{ Use: "update [password/comment/allowed-hosts flags]", Short: "Replace mutable fields of a database (update_database)", @@ -151,7 +179,7 @@ func newDatabasesUpdateCmd(opts *RootOptions) *cobra.Command { } cmd.RunE = runWriteE(opts, func(args []string) (writeSpec, error) { login := args[0] - fields := databaseChangedFields(cmd, f) + fields := databaseUpdateChangedFields(cmd, f) if len(fields) == 0 { return writeSpec{}, fmt.Errorf("at least one field flag (e.g. --password/--comment/--allowed-hosts) is required") } @@ -172,6 +200,20 @@ func newDatabasesUpdateCmd(opts *RootOptions) *cobra.Command { return cmd } +// databaseDeleteConfirm builds the ConfirmAction shown before +// delete_database is dispatched. Extracted from the RunE closure so +// the loudness adjustment (Verb: "permanently delete" instead of the +// bare "delete" every other slice uses) is structurally pinned by a +// test rather than only by a source-code comment. +// +// delete_database drops the database AND every row it contains, which +// is the loudest data-loss surface of the v0.2.0 write phase. The +// prompt template adds "This cannot be undone." regardless of the +// verb. +func databaseDeleteConfirm(login string) ConfirmAction { + return ConfirmAction{Verb: "permanently delete", Resource: "database", ID: login} +} + func newDatabasesDeleteCmd(opts *RootOptions) *cobra.Command { return &cobra.Command{ Use: "delete ", @@ -182,14 +224,8 @@ func newDatabasesDeleteCmd(opts *RootOptions) *cobra.Command { return writeSpec{ action: "delete_database", destructive: true, - // "permanently delete" is intentionally more emphatic - // than the bare "delete" verb the other slices use: - // delete_database drops the database AND every row it - // contains, which is the loudest data-loss surface of - // the v0.2.0 write phase. The prompt template adds - // "This cannot be undone." regardless. - confirm: ConfirmAction{Verb: "permanently delete", Resource: "database", ID: login}, - params: database.DeleteParams(login), + confirm: databaseDeleteConfirm(login), + params: database.DeleteParams(login), dispatch: func(c *api.Client, ctx context.Context) (string, error) { if derr := database.NewClient(c).Delete(ctx, login); derr != nil { return "", derr diff --git a/internal/cli/databases_test.go b/internal/cli/databases_test.go index 2a28b47..538d7b9 100644 --- a/internal/cli/databases_test.go +++ b/internal/cli/databases_test.go @@ -38,7 +38,6 @@ func TestDatabasesAddRejectsBadInput(t *testing.T) { }{ {"missing --password", []string{"databases", "add", "--comment", "c", "--allowed-hosts", "localhost"}}, {"missing --comment", []string{"databases", "add", "--password", "s3cret", "--allowed-hosts", "localhost"}}, - {"missing --allowed-hosts", []string{"databases", "add", "--password", "s3cret", "--comment", "c"}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -60,6 +59,48 @@ func TestDatabasesAddRejectsBadInput(t *testing.T) { } } +// add must accept omitted --allowed-hosts: an empty value is the KAS +// API's documented "any host may connect" wildcard, not a missing +// parameter. The dry-run preview must therefore reach action / +// params assembly (no validation rejection) and the +// database_allowed_hosts key must be present in the params with the +// empty-string value. +func TestDatabasesAddOptionalAllowedHosts(t *testing.T) { + t.Parallel() + root, opts := cli.NewRootCmd() + root.AddCommand(cli.NewDatabasesCmd(opts)) + var out, errb bytes.Buffer + root.SetOut(&out) + root.SetErr(&errb) + root.SetArgs([]string{ + "databases", "add", + "--password", "s3cret", + "--comment", "Test DB", + "--dry-run", "-o", "json", + "--login", "w0", "--auth-data", "x", "--auth-type", "plain", + }) + if err := root.Execute(); err != nil { + t.Fatalf("Execute: %v\nstderr: %s", err, errb.String()) + } + var got struct { + Action string `json:"action"` + Params map[string]string `json:"params"` + } + if err := json.Unmarshal(out.Bytes(), &got); err != nil { + t.Fatalf("unmarshal preview: %v\nstdout: %s", err, out.String()) + } + if got.Action != "add_database" { + t.Errorf("action = %q, want add_database", got.Action) + } + v, ok := got.Params["database_allowed_hosts"] + if !ok { + t.Errorf("params missing database_allowed_hosts (empty wildcard must still be sent on the wire): %v", got.Params) + } + if v != "" { + t.Errorf("params[database_allowed_hosts] = %q, want \"\" (wildcard)", v) + } +} + // The destructive database subcommands (update/delete) must refuse on // a non-interactive stdin without --yes rather than dispatch // unconfirmed. @@ -167,14 +208,118 @@ func TestDatabasesUpdateDryRunFieldAssembly(t *testing.T) { } } +// add and update bind disjoint flag sets. Each set is currently +// flag-name-identical (--password / --comment / --allowed-hosts), but +// the bind is per-subcommand so the help text reflects the action +// semantics (initial vs replacement, required vs optional) and a +// future refactor that re-merges them silently can't sneak past the +// help-text-truthfulness contract. Add a sentinel update-only flag +// here when the action surfaces diverge (ddnsuser-style). +func TestDatabasesFlagSetsAreDisjoint(t *testing.T) { + t.Parallel() + cases := []struct { + name string + args []string + }{ + // Sentinel: passing an unknown flag (--target-ipv4) on add or + // update must fail at cobra parse time rather than be silently + // ignored. Identical add/update surfaces today still rely on + // per-subcommand bind, so re-merging would re-introduce the + // help-text drift the ddnsuser slice already fixed. + {"unknown --target-ipv4 rejected on add", []string{ + "databases", "add", + "--password", "s3cret", "--comment", "c", "--allowed-hosts", "localhost", + "--target-ipv4", "127.0.0.1", + }}, + {"unknown --target-ipv4 rejected on update", []string{ + "databases", "update", "d0123460", + "--target-ipv4", "127.0.0.1", + }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + root, opts := cli.NewRootCmd() + root.AddCommand(cli.NewDatabasesCmd(opts)) + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs(c.args) + err := root.Execute() + if err == nil { + t.Fatalf("Execute %v: want unknown-flag error, got nil", c.args) + } + if !strings.Contains(err.Error(), "unknown flag") { + t.Errorf("err = %q, want it to contain 'unknown flag'", err) + } + }) + } +} + +// The delete_database ConfirmAction uses the louder "permanently +// delete" verb instead of the bare "delete" every other slice uses. +// Pin both the verb and the rendered Summary so a future refactor of +// either the slice or the shared ConfirmAction template cannot +// silently regress the loudness contract — the source-code comment +// alone is not enforceable. +func TestDatabasesDeleteConfirmIsLouder(t *testing.T) { + t.Parallel() + a := cli.DatabaseDeleteConfirm("d0123460") + if a.Verb != "permanently delete" { + t.Errorf("Verb = %q, want %q", a.Verb, "permanently delete") + } + if a.Resource != "database" { + t.Errorf("Resource = %q, want %q", a.Resource, "database") + } + if a.ID != "d0123460" { + t.Errorf("ID = %q, want d0123460", a.ID) + } + want := `About to permanently delete database "d0123460". This cannot be undone.` + if got := a.Summary(); got != want { + t.Errorf("Summary() = %q, want %q", got, want) + } +} + +// On --dry-run the runWriteE seam must still emit a #131 audit record +// (outcome=dry-run, action=delete_database, target=, +// database_login=) on stderr, even though no SOAP call is +// dispatched. This pins the database delete subcommand's wiring into +// the audit emission path — without it, a future refactor that breaks +// the runWriteE → WriteAudit glue would only be caught at the +// surrounding-package level. +func TestDatabasesDeleteDryRunEmitsAuditLine(t *testing.T) { + t.Parallel() + root, opts := cli.NewRootCmd() + root.AddCommand(cli.NewDatabasesCmd(opts)) + var out, errb bytes.Buffer + root.SetOut(&out) + root.SetErr(&errb) + root.SetArgs([]string{ + "databases", "delete", "d0123460", + "--dry-run", + "--login", "w0000000", "--auth-data", "x", "--auth-type", "plain", + }) + if err := root.Execute(); err != nil { + t.Fatalf("Execute: %v", err) + } + line := errb.String() + for _, want := range []string{ + "action=delete_database", + "target=d0123460", + "outcome=dry-run", + "database_login=d0123460", + "login=w0000000", + } { + if !strings.Contains(line, want) { + t.Errorf("audit line missing %q\nline: %s", want, line) + } + } +} + // delete_database's dry-run preview must address the database by its // login (target=) and use the delete_database action verbatim, // so the audit trail can later be reconciled to the resource that was -// dropped. The "louder" prompt verb itself ("permanently delete") is -// pinned by the source-code review anchor (database.go) rather than -// this CLI test because the dry-run preview JSON intentionally omits -// the ConfirmAction shape — only the action / target / params are -// machine-readable. +// dropped. func TestDatabasesDeleteDryRunTargetsLogin(t *testing.T) { t.Parallel() root, opts := cli.NewRootCmd() diff --git a/internal/cli/export_test.go b/internal/cli/export_test.go index 17c6da9..eec5883 100644 --- a/internal/cli/export_test.go +++ b/internal/cli/export_test.go @@ -33,3 +33,9 @@ var RevokeSession = revokeSession // `sessions delete` subcommand. Tests inject a RevokeFunc spy and a // temp session.Store, mirroring the `config use-profile` pattern. var RunSessionsDelete = runSessionsDelete + +// DatabaseDeleteConfirm exposes the package-private helper that +// builds the ConfirmAction for delete_database. Tests use it to pin +// the "permanently delete" loudness adjustment (database is the only +// slice using that emphatic verb). +var DatabaseDeleteConfirm = databaseDeleteConfirm diff --git a/internal/database/database.go b/internal/database/database.go index fa532b1..e85dd44 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -14,13 +14,25 @@ import ( // return a *soap.Response decoded from a fixture. type Caller = kasread.Caller +// The KAS API encodes the in_progress async-write flag as the literal +// strings "TRUE" / "FALSE" (not booleans). Both literals are exported +// so mappings + tests share a single source of truth instead of +// re-typing string literals at every comparison site. +const ( + InProgressFalse = "FALSE" + InProgressTrue = "TRUE" +) + // Database is one entry of get_databases. The list and singular views // (the latter being get_databases called with a database_login filter) // return the same Map shape, so a single struct covers both. // // in_progress is a pending-async-write flag the KAS API surfaces on -// every database row (typically "FALSE"). It is flagged with omitempty -// for parity with the other modules' optional flags. +// every database row (typically "FALSE"). It is rendered without +// omitempty for parity with the majority of the read modules +// (account, mailaccount, mailinglist, sambauser, ftpuser, …) — the +// fixture has shown it on every row captured so far, and a leak as +// the empty string is harmless if the API ever stops sending it. type Database struct { Name string `json:"database_name" yaml:"database_name"` Login string `json:"database_login" yaml:"database_login"` @@ -28,7 +40,7 @@ type Database struct { Comment string `json:"database_comment" yaml:"database_comment"` AllowedHosts string `json:"database_allowed_hosts" yaml:"database_allowed_hosts"` UsedDatabaseSpace float64 `json:"used_database_space" yaml:"used_database_space"` - InProgress string `json:"in_progress,omitempty" yaml:"in_progress,omitempty"` + InProgress string `json:"in_progress" yaml:"in_progress"` } // DatabaseList is the typed payload of get_databases; satisfies @@ -95,12 +107,14 @@ func DecodeDatabases(returnInfo soap.Value) (DatabaseList, error) { // TableHeaders returns the columns used by --output=table for // DatabaseList. func (DatabaseList) TableHeaders() []string { - return []string{"LOGIN", "NAME", "COMMENT", "ALLOWED_HOSTS", "USED_MB", "IN_PROGRESS"} + return []string{"LOGIN", "NAME", "COMMENT", "ALLOWED_HOSTS", "USED", "IN_PROGRESS"} } // TableRows emits one row per Database entry. used_database_space is -// reported in KiB by KAS; we convert to MB to match the units used in -// the accounts/mailaccounts list views. +// reported in KiB by KAS; we convert to MB and render the unit as part +// of the value so the list cells share a single convention with the +// singular FIELD/VALUE detail view (both carry the unit in the value, +// not in a header column). func (l DatabaseList) TableRows() [][]string { rows := make([][]string, 0, len(l)) for _, d := range l { @@ -109,7 +123,7 @@ func (l DatabaseList) TableRows() [][]string { d.Name, d.Comment, d.AllowedHosts, - strconv.FormatFloat(d.UsedDatabaseSpace/1024, 'f', 2, 64), + strconv.FormatFloat(d.UsedDatabaseSpace/1024, 'f', 2, 64) + " MB", d.InProgress, }) } @@ -123,7 +137,9 @@ func (Database) TableHeaders() []string { } // TableRows emits the scalar fields. database_password is intentionally -// omitted — consumers that need it should use --output=json|yaml. +// omitted — consumers that need it should use --output=json|yaml. The +// used_database_space row carries the unit (" MB") as part of the +// value so the singular and list views share a single convention. // in_progress only appears when the API actually returned it. func (d Database) TableRows() [][]string { rows := [][]string{ diff --git a/internal/database/database_test.go b/internal/database/database_test.go index a5f5e85..4248b4a 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -33,8 +33,8 @@ func TestDecodeDatabases(t *testing.T) { if d.UsedDatabaseSpace == 0 { t.Errorf("UsedDatabaseSpace = 0, want non-zero from xsd:float") } - if d.InProgress != "FALSE" { - t.Errorf("InProgress = %q, want FALSE", d.InProgress) + if d.InProgress != database.InProgressFalse { + t.Errorf("InProgress = %q, want %q", d.InProgress, database.InProgressFalse) } // d0123457 is the first entry with a non-empty allowed_hosts in the // fixture; verify the empty-string default survives for the others. @@ -60,8 +60,8 @@ func TestDecodeDatabaseSingular(t *testing.T) { if d.Login != "d0123460" { t.Errorf("Login = %q, want d0123460", d.Login) } - if d.InProgress != "FALSE" { - t.Errorf("InProgress = %q, want FALSE", d.InProgress) + if d.InProgress != database.InProgressFalse { + t.Errorf("InProgress = %q, want %q", d.InProgress, database.InProgressFalse) } } diff --git a/internal/database/write.go b/internal/database/write.go index 9ed41fe..3ce18d1 100644 --- a/internal/database/write.go +++ b/internal/database/write.go @@ -39,12 +39,16 @@ const ( FieldAllowedHosts = "database_allowed_hosts" ) -// Spec carries the add_database request fields. Password, comment and -// allowed_hosts are required by the KAS API and validated as non-empty -// before any SOAP call so the CLI can surface a fast validation error. -// The allowed_hosts grammar (comma-separated host names / IPs / CIDR) -// is delegated to the API — a wrong value surfaces as -// database_allowed_hosts_syntax_incorrect. +// Spec carries the add_database request fields. Password and comment +// are required by the KAS API and validated as non-empty before any +// SOAP call so the CLI can surface a fast validation error. +// +// AllowedHosts is the comma-separated host name / IP / CIDR list the +// new database accepts connections from. An empty AllowedHosts is a +// meaningful, explicit "any host may connect" — the API treats the +// absence of a host list as wildcard access, not as a missing +// parameter. The grammar (when non-empty) is delegated to the API; a +// malformed value surfaces as database_allowed_hosts_syntax_incorrect. // // add_database takes no database_login: the server auto-generates the // login (always equal to the database name on creation, e.g. @@ -64,9 +68,16 @@ type Spec struct { // database_allowed_hosts_syntax_incorrect, …) is left to the API and // surfaces verbatim through the Caller. func (cl *Client) Add(ctx context.Context, s Spec) (string, error) { - if s.Password == "" || s.Comment == "" || s.AllowedHosts == "" { - return "", errors.New("database: add_database requires a non-empty password, comment and allowed_hosts") + switch { + case s.Password == "": + return "", errors.New("database: add_database requires a non-empty password") + case s.Comment == "": + return "", errors.New("database: add_database requires a non-empty comment") } + // s.AllowedHosts is intentionally NOT validated as non-empty: an + // empty list means "any host may connect" (the KAS API's documented + // wildcard semantics), which is a deliberate user choice rather + // than a missing parameter. resp, err := kaswrite.Call(ctx, cl.c, "database", addAction, AddParams(s)) if err != nil { return "", err diff --git a/internal/database/write_test.go b/internal/database/write_test.go index a6ab0b2..0824c30 100644 --- a/internal/database/write_test.go +++ b/internal/database/write_test.go @@ -3,6 +3,7 @@ package database_test import ( "context" "errors" + "strings" "testing" "github.com/chmmou/kasapi-cli/internal/database" @@ -104,19 +105,39 @@ func TestWriteValidation(t *testing.T) { c := database.NewClient(&testutil.FakeCaller{}) ctx := context.Background() + // Each missing-field case must surface a per-field validation + // error (mentioning only that single field), not a combined + // "requires password, comment AND X" message — the latter forces + // the caller to guess which field actually broke. for _, tc := range []struct { - name string - mut func(*database.Spec) + name string + mut func(*database.Spec) + wantSub string }{ - {"missing password", func(s *database.Spec) { s.Password = "" }}, - {"missing comment", func(s *database.Spec) { s.Comment = "" }}, - {"missing allowed_hosts", func(s *database.Spec) { s.AllowedHosts = "" }}, + {"missing password", func(s *database.Spec) { s.Password = "" }, "password"}, + {"missing comment", func(s *database.Spec) { s.Comment = "" }, "comment"}, } { s := sampleSpec() tc.mut(&s) - if _, err := c.Add(ctx, s); err == nil { + _, err := c.Add(ctx, s) + if err == nil { t.Errorf("Add %s: err = nil, want validation error", tc.name) + continue } + if !strings.Contains(err.Error(), tc.wantSub) { + t.Errorf("Add %s: err = %q, want it to mention %q", tc.name, err.Error(), tc.wantSub) + } + } + // AllowedHosts == "" is a deliberate "any host may connect" value, + // not a missing parameter — it must NOT trigger domain validation + // and Add must reach the SOAP call (which the FakeCaller intercepts + // with a success-response fixture). + resp := testutil.DecodeFixture(t, "database/add_database_response_success.xml") + emptyHostsClient := database.NewClient(&testutil.FakeCaller{Resp: resp}) + emptyHosts := sampleSpec() + emptyHosts.AllowedHosts = "" + if _, err := emptyHostsClient.Add(ctx, emptyHosts); err != nil { + t.Errorf("Add with empty AllowedHosts: err = %v, want nil (empty is the documented wildcard)", err) } if err := c.Update(ctx, "", map[string]string{database.FieldComment: "x"}); err == nil { t.Error("Update empty login: err = nil, want validation error")