Skip to content

refactor(cli): replace bind() with explicit field assignment#6254

Merged
gustavosbarreto merged 1 commit intomasterfrom
refactor/cli-remove-bind-reflection
Apr 29, 2026
Merged

refactor(cli): replace bind() with explicit field assignment#6254
gustavosbarreto merged 1 commit intomasterfrom
refactor/cli-remove-bind-reflection

Conversation

@geovannewashington
Copy link
Copy Markdown
Member

What changed?

Removed the reflection-based bind() helper from cmd/ and replaced
it with direct struct field assignment in each command handler. Also
validates the role argument early in memberAdd and fixes an incorrect
RangeArgs bound in namespaceCreate.

Why

bind() used reflection to map positional CLI arguments onto input
structs by field index order. This introduced three problems:

A hidden coupling between struct field declaration order and CLI
argument position, with no compile-time enforcement. If anyone reorders
fields in an inputs.* struct in the future, arguments will be
silently mapped to the wrong fields - no compile error, no runtime
error, just wrong behavior.

A latent runtime panic: reflect.Value.SetString requires the field
kind to be exactly string, but authorizer.Role is a named type.
Any call to cli namespace member add would have panicked.

Poor readability - tracing what args[0] maps to required following
reflection logic and struct declarations simultaneously. bind() is
the kind of solution that looks clever but becomes a liability. Go
favors explicit over clever - a few extra lines of direct assignment
are immediately readable, compiler-checked, and carry no hidden
contracts or runtime risks.

Explicit assignment makes the mapping immediately obvious, is checked
by the compiler, and eliminates the panic entirely.

The role validation fix (RoleInvalid check in memberAdd) was
discovered during this refactor and is included here since it directly
replaces the broken bind() behavior for that command. Similarly,
cobra.RangeArgs(2, 4) in namespaceCreate was corrected to
cobra.RangeArgs(2, 3) since --type is a flag, not a positional
argument.

How to test

  1. Start the environment with ./bin/docker-compose up -d
  2. Exercise the affected commands:
./bin/cli user create testuser Secret123! test@test.com
./bin/cli namespace create testns testuser
./bin/cli namespace member add testuser testns observer
./bin/cli namespace member add testuser testns invalidrole  # must error

Remove the reflection-based bind() helper and replace it with direct
struct field assignment in each command handler. This eliminates a
hidden coupling between struct field declaration order and CLI argument
position, and fixes a latent runtime panic caused by calling
SetString() on authorizer.Role, a named string type that reflection
does not treat as a plain string.

Also validates the role argument early in memberAdd, returning a clear
error message instead of silently passing RoleInvalid downstream, and
fixes cobra.RangeArgs(2, 4) to cobra.RangeArgs(2, 3) in
namespaceCreate since --type is a flag, not a positional argument.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 29, 2026

Claude finished @geovannewashington's task in 4m 40s —— View job


Code Review Complete

Reviewed 3 files across code quality, security, testing, Go patterns, and architecture — no issues found. The code looks good as-is.

The refactor correctly replaces the reflection-based bind() with explicit field assignments that are type-safe, compiler-checked, and immediately readable. The RangeArgs(2, 3) fix and the RoleFromString validation are both correct improvements. All field-to-argument mappings match the input struct definitions, and there are no remaining references to the deleted bind() function anywhere in the codebase (including cloud/).


If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

@gustavosbarreto gustavosbarreto merged commit 6df3dba into master Apr 29, 2026
22 of 23 checks passed
@gustavosbarreto gustavosbarreto deleted the refactor/cli-remove-bind-reflection branch April 29, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants