From 4bac6607b4e24adc7527fae41106e45fe769a369 Mon Sep 17 00:00:00 2001 From: Ralph Schmieder Date: Wed, 18 Mar 2026 11:17:27 +0100 Subject: [PATCH 1/5] feat(auth): add static-token authentication method - implement StaticTokenProvider returning consistent token - introduce WithStaticToken option in client configuration - ensure no calls to /api/v0/auth_extended with retries on 401 - enhance AuthProvider to error on missing credentials --- gocmlclient_test.go | 4 +- internal/auth/provider.go | 5 +++ internal/auth/provider_test.go | 26 +++++++++++ internal/auth/providers.go | 3 ++ internal/auth/static_token_provider.go | 32 ++++++++++++++ internal/auth/static_token_provider_test.go | 37 ++++++++++++++++ internal/auth/transport_test.go | 48 +++++++++++++++++++++ pkg/client/client.go | 25 ++++++----- pkg/client/client_test.go | 48 +++++++++++++++++++++ pkg/client/options.go | 12 ++++++ pkg/client/options_test.go | 7 +++ 11 files changed, 235 insertions(+), 12 deletions(-) create mode 100644 internal/auth/static_token_provider.go create mode 100644 internal/auth/static_token_provider_test.go diff --git a/gocmlclient_test.go b/gocmlclient_test.go index 821d237..9efa9a7 100644 --- a/gocmlclient_test.go +++ b/gocmlclient_test.go @@ -22,6 +22,6 @@ func TestNewFunction(t *testing.T) { // Test the actual New function by calling it // This will fail due to no server, but it will exercise the code path _, err := New("http://invalid-server:12345") - assert.Error(t, err) // Expected to fail due to invalid server - assert.Contains(t, err.Error(), "dial tcp") // Network error expected + assert.Error(t, err) // Expected to fail due to invalid server + assert.NotEmpty(t, err.Error()) } diff --git a/internal/auth/provider.go b/internal/auth/provider.go index badf9e4..80cef1b 100644 --- a/internal/auth/provider.go +++ b/internal/auth/provider.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/http" + "strings" "time" "github.com/rschmied/gocmlclient/internal/httputil" @@ -78,6 +79,10 @@ func (p *AuthProvider) FetchToken(ctx context.Context) (string, time.Time, error return token, expiry, nil } + if strings.TrimSpace(p.username) == "" || strings.TrimSpace(p.password) == "" { + return "", time.Time{}, fmt.Errorf("authentication requires username/password but none configured") + } + return p.authenticateWithPassword(ctx) } diff --git a/internal/auth/provider_test.go b/internal/auth/provider_test.go index ec2708c..cbc5fa4 100644 --- a/internal/auth/provider_test.go +++ b/internal/auth/provider_test.go @@ -165,6 +165,32 @@ func TestFetchTokenAuthentication(t *testing.T) { } } +func TestFetchTokenMissingCredentials(t *testing.T) { + called := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + config := AuthConfig{ + BaseURL: server.URL, + Client: &http.Client{Timeout: 10 * time.Second}, + } + + provider := NewAuthProvider(config) + _, _, err := provider.FetchToken(context.Background()) + if err == nil { + t.Fatal("expected error") + } + if err.Error() != "authentication requires username/password but none configured" { + t.Fatalf("unexpected error: %v", err) + } + if called { + t.Fatal("expected no server call") + } +} + func TestFetchTokenAuthenticationFailure(t *testing.T) { // Create a test server that returns authentication failure server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/auth/providers.go b/internal/auth/providers.go index 26ff8a9..7635d71 100644 --- a/internal/auth/providers.go +++ b/internal/auth/providers.go @@ -18,6 +18,9 @@ type TokenProvider interface { // Ensure AuthProvider implements TokenProvider var _ TokenProvider = (*AuthProvider)(nil) +// Ensure StaticTokenProvider implements TokenProvider +var _ TokenProvider = (*StaticTokenProvider)(nil) + // Type implements TokenProvider for AuthProvider func (p *AuthProvider) Type() string { return "password" diff --git a/internal/auth/static_token_provider.go b/internal/auth/static_token_provider.go new file mode 100644 index 0000000..08778df --- /dev/null +++ b/internal/auth/static_token_provider.go @@ -0,0 +1,32 @@ +package auth + +import ( + "context" + "time" +) + +// StaticTokenProvider returns the same bearer token for every request. +// +// This is intended for token-only configurations where the token is managed +// outside of gocmlclient (e.g. Terraform provider config). It never attempts to +// authenticate via username/password. +type StaticTokenProvider struct { + token string +} + +// NewStaticTokenProvider returns a TokenProvider that always yields the given +// token. +func NewStaticTokenProvider(token string) *StaticTokenProvider { + return &StaticTokenProvider{token: token} +} + +// FetchToken implements TokenProvider. +func (p *StaticTokenProvider) FetchToken(ctx context.Context) (string, time.Time, error) { + // Use a long horizon so the manager does not try to refresh proactively. + return p.token, time.Now().Add(10 * 365 * 24 * time.Hour), nil +} + +// Type implements TokenProvider. +func (p *StaticTokenProvider) Type() string { + return "static" +} diff --git a/internal/auth/static_token_provider_test.go b/internal/auth/static_token_provider_test.go new file mode 100644 index 0000000..1c5ecf5 --- /dev/null +++ b/internal/auth/static_token_provider_test.go @@ -0,0 +1,37 @@ +package auth + +import ( + "context" + "testing" + "time" +) + +func TestStaticTokenProvider(t *testing.T) { + p := NewStaticTokenProvider("t") + + tok1, exp1, err := p.FetchToken(context.Background()) + if err != nil { + t.Fatalf("FetchToken: %v", err) + } + if tok1 != "t" { + t.Fatalf("expected token 't', got %q", tok1) + } + if time.Until(exp1) <= 0 { + t.Fatalf("expected expiry in the future, got %v", exp1) + } + + tok2, exp2, err := p.FetchToken(context.Background()) + if err != nil { + t.Fatalf("FetchToken: %v", err) + } + if tok2 != "t" { + t.Fatalf("expected token 't', got %q", tok2) + } + if exp2.Before(time.Now()) { + t.Fatalf("expected expiry in the future, got %v", exp2) + } + + if p.Type() != "static" { + t.Fatalf("expected type 'static', got %q", p.Type()) + } +} diff --git a/internal/auth/transport_test.go b/internal/auth/transport_test.go index 5015819..ab9c2c7 100644 --- a/internal/auth/transport_test.go +++ b/internal/auth/transport_test.go @@ -201,6 +201,54 @@ func TestTransportRoundTrip401Retry(t *testing.T) { } } +func TestTransportStaticTokenNoAuthExtended(t *testing.T) { + var apiCalls int + var authCalls int + callCount := 0 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v0/auth_extended": + authCalls++ + w.WriteHeader(http.StatusOK) + return + case "/api/data": + apiCalls++ + callCount++ + if callCount == 1 { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + return + default: + w.WriteHeader(http.StatusNotFound) + return + } + })) + defer server.Close() + + manager := NewManager(NewStaticTokenProvider("t"), DefaultConfig()) + transport := NewTransport(http.DefaultTransport, manager, nil) + + req, _ := http.NewRequest("GET", server.URL+"/api/data", nil) + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip: %v", err) + } + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } + if authCalls != 0 { + t.Fatalf("expected no /api/v0/auth_extended calls, got %d", authCalls) + } + if apiCalls != 2 { + t.Fatalf("expected 2 /api/data calls (initial + retry), got %d", apiCalls) + } +} + func TestTransportRoundTripManagerError(t *testing.T) { manager := createFailingManager() transport := NewTransport(http.DefaultTransport, manager, nil) diff --git a/pkg/client/client.go b/pkg/client/client.go index a27ad61..ac0d82e 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -183,16 +183,21 @@ func newAPIClient(c *Config) (*api.Client, error) { // 4. create token provider - it will use the SAME http client but the auth // transport will skip auth endpoints - provider := auth.NewAuthProvider(auth.AuthConfig{ - BaseURL: c.baseURL, - Username: c.username, - Password: c.password, - PresetToken: c.token, - Client: c.httpClient, - ClientID: httputil.ClientID, - ClientUUID: clientUUID, - Version: clientVersion, - }) + var provider auth.TokenProvider + if c.staticToken != "" { + provider = auth.NewStaticTokenProvider(c.staticToken) + } else { + provider = auth.NewAuthProvider(auth.AuthConfig{ + BaseURL: c.baseURL, + Username: c.username, + Password: c.password, + PresetToken: c.token, + Client: c.httpClient, + ClientID: httputil.ClientID, + ClientUUID: clientUUID, + Version: clientVersion, + }) + } // 5. create manager with token storage (default is memory storage) config := auth.DefaultConfig() diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index b9864ac..66fbefe 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -1,6 +1,7 @@ package client import ( + "context" "net/http" "net/http/httptest" "testing" @@ -63,6 +64,18 @@ func TestNew(t *testing.T) { assert.Equal(t, "test-token", client.config.token) }, }, + { + name: "with static token", + baseURL: "https://api.example.com", + opts: []Option{ + WithStaticToken("test-token"), + SkipReadyCheck(), + }, + wantErr: false, + validate: func(t *testing.T, client *Client) { + assert.Equal(t, "test-token", client.config.staticToken) + }, + }, { name: "with insecure TLS", baseURL: "https://api.example.com", @@ -311,3 +324,38 @@ func TestClient_Stats(t *testing.T) { assert.NotNil(t, stats.StatusCounts()) assert.Equal(t, 0, stats.TotalCalls()) // Should be 0 since no calls made yet } + +func TestClient_StaticToken401ThenSuccess_NoAuthExtended(t *testing.T) { + var authCalls int + var dataCalls int + dataCount := 0 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v0/auth_extended": + authCalls++ + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"id":"user-123","username":"testuser","token":"mock-token-12345","admin":false}`)) //nolint:errcheck + case "/api/v0/users": + dataCalls++ + dataCount++ + if dataCount == 1 { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`[]`)) //nolint:errcheck + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + c, err := New(server.URL, WithStaticToken("t"), SkipReadyCheck()) + assert.NoError(t, err) + + _, err = c.User.Users(context.Background()) + assert.NoError(t, err) + assert.Equal(t, 0, authCalls) + assert.Equal(t, 2, dataCalls) +} diff --git a/pkg/client/options.go b/pkg/client/options.go index b27ff98..7851b3a 100644 --- a/pkg/client/options.go +++ b/pkg/client/options.go @@ -15,6 +15,7 @@ type Config struct { username string password string token string + staticToken string tokenStorageFile string insecureSkipVerify bool caCertPEM []byte @@ -64,6 +65,17 @@ func WithToken(token string) Option { } } +// WithStaticToken sets a static bearer token for authentication. +// +// Unlike WithToken, this token is not treated as a one-shot bootstrap token. +// The client will never attempt username/password authentication when this is +// configured. +func WithStaticToken(token string) Option { + return func(c *Config) { + c.staticToken = token + } +} + // WithCACertPEM sets a custom CA certificate bundle (PEM). The certificates // are added to the system cert pool. func WithCACertPEM(certPEM []byte) Option { diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go index 9399e4a..e99047a 100644 --- a/pkg/client/options_test.go +++ b/pkg/client/options_test.go @@ -65,6 +65,13 @@ func TestWithToken(t *testing.T) { assert.Equal(t, "testtoken", c.token) } +func TestWithStaticToken(t *testing.T) { + c := &Config{} + opt := WithStaticToken("statictoken") + opt(c) + assert.Equal(t, "statictoken", c.staticToken) +} + func TestWithTokenStorageFile(t *testing.T) { c := &Config{} opt := WithTokenStorageFile("/path/to/file") From c87bbf75c093b5fc671157b6e88c46fad8293cab Mon Sep 17 00:00:00 2001 From: Ralph Schmieder Date: Wed, 18 Mar 2026 11:30:36 +0100 Subject: [PATCH 2/5] test(logging): add tests for SetDefault(nil) and log wrapper methods - add tests for SetDefault(nil) using slog.Default - verify emitted slog records with Debug/Info/Warn/Error wrappers - enhance tests in pkg/models for error handling in annotations - ensure proper behavior of DiskImage1 unmarshal and omit optional fields --- internal/logging/logging_test.go | 67 +++++++++++++++++++++++++++++++ pkg/models/annotation_test.go | 68 ++++++++++++++++++++++++++++++++ pkg/models/imagedef_test.go | 36 +++++++++++++++++ 3 files changed, 171 insertions(+) create mode 100644 internal/logging/logging_test.go diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go new file mode 100644 index 0000000..0048c83 --- /dev/null +++ b/internal/logging/logging_test.go @@ -0,0 +1,67 @@ +package logging + +import ( + "context" + "log/slog" + "sync" + "testing" +) + +type captureHandler struct { + mu sync.Mutex + lastMsg string + lastLvl slog.Level + hasEntry bool +} + +func (h *captureHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h *captureHandler) Handle(_ context.Context, r slog.Record) error { + h.mu.Lock() + defer h.mu.Unlock() + h.lastMsg = r.Message + h.lastLvl = r.Level + h.hasEntry = true + return nil +} + +func (h *captureHandler) WithAttrs([]slog.Attr) slog.Handler { return h } +func (h *captureHandler) WithGroup(string) slog.Handler { return h } + +func TestSetDefault_NilUsesSlogDefault(t *testing.T) { + old := L() + t.Cleanup(func() { SetDefault(old) }) + + SetDefault(nil) + if L() != slog.Default() { + t.Fatalf("expected slog.Default()") + } +} + +func TestSetDefault_CustomLoggerAndWrappers(t *testing.T) { + old := L() + t.Cleanup(func() { SetDefault(old) }) + + h := &captureHandler{} + SetDefault(slog.New(h)) + + Debug("d") + if !h.hasEntry || h.lastMsg != "d" || h.lastLvl != slog.LevelDebug { + t.Fatalf("expected debug record, got msg=%q level=%v", h.lastMsg, h.lastLvl) + } + + Info("i") + if h.lastMsg != "i" || h.lastLvl != slog.LevelInfo { + t.Fatalf("expected info record, got msg=%q level=%v", h.lastMsg, h.lastLvl) + } + + Warn("w") + if h.lastMsg != "w" || h.lastLvl != slog.LevelWarn { + t.Fatalf("expected warn record, got msg=%q level=%v", h.lastMsg, h.lastLvl) + } + + Error("e") + if h.lastMsg != "e" || h.lastLvl != slog.LevelError { + t.Fatalf("expected error record, got msg=%q level=%v", h.lastMsg, h.lastLvl) + } +} diff --git a/pkg/models/annotation_test.go b/pkg/models/annotation_test.go index 8b1cb77..fd9563b 100644 --- a/pkg/models/annotation_test.go +++ b/pkg/models/annotation_test.go @@ -182,3 +182,71 @@ func TestAnnotationUpdate_Marshal_Errors(t *testing.T) { _, err = json.Marshal(AnnotationUpdate{Type: "nope"}) assert.Error(t, err) } + +func TestAnnotation_Unmarshal_Null(t *testing.T) { + var a Annotation + err := json.Unmarshal([]byte(`null`), &a) + assert.NoError(t, err) + assert.Equal(t, AnnotationType(""), a.Type) + assert.Nil(t, a.Text) + assert.Nil(t, a.Rectangle) + assert.Nil(t, a.Ellipse) + assert.Nil(t, a.Line) +} + +func TestAnnotation_Unmarshal_UnknownType(t *testing.T) { + var a Annotation + err := json.Unmarshal([]byte(`{"id":"x","type":"nope"}`), &a) + assert.Error(t, err) +} + +func TestAnnotation_Unmarshal_MissingType(t *testing.T) { + var a Annotation + err := json.Unmarshal([]byte(`{"id":"x"}`), &a) + assert.Error(t, err) +} + +func TestAnnotationCreate_Marshal_Errors(t *testing.T) { + _, err := json.Marshal(AnnotationCreate{Type: AnnotationTypeText}) + assert.Error(t, err) + _, err = json.Marshal(AnnotationCreate{Type: AnnotationTypeLine}) + assert.Error(t, err) + _, err = json.Marshal(AnnotationCreate{Type: "nope"}) + assert.Error(t, err) +} + +func TestLineAnnotationCreate_Marshal_EmitsNullLineEnds(t *testing.T) { + in := AnnotationCreate{Type: AnnotationTypeLine, Line: &LineAnnotation{Type: AnnotationTypeLine, BorderColor: "#000", BorderStyle: "", Color: "#fff", Thickness: 1, X1: 1, Y1: 2, X2: 3, Y2: 4, ZIndex: 0, LineStart: nil, LineEnd: nil}} + b, err := json.Marshal(in) + assert.NoError(t, err) + var m map[string]any + assert.NoError(t, json.Unmarshal(b, &m)) + assert.Contains(t, m, "line_start") + assert.Contains(t, m, "line_end") + assert.Nil(t, m["line_start"]) + assert.Nil(t, m["line_end"]) +} + +func TestAnnotation_Unmarshal_ClearsOtherUnionFields(t *testing.T) { + // Start with a non-empty Annotation value. + a := Annotation{ + Type: AnnotationTypeText, + Text: &TextAnnotationResponse{ID: "a1", TextAnnotation: TextAnnotation{Type: AnnotationTypeText, BorderColor: "#000", BorderStyle: "", Color: "#fff", Thickness: 1, X1: 1, Y1: 2, ZIndex: 0, Rotation: 0, TextBold: false, TextContent: "hi", TextFont: "sans", TextItalic: false, TextSize: 12, TextUnit: "px"}}, + } + assert.NotNil(t, a.Text) + + // Unmarshal a rectangle over it; ensure old fields are cleared. + err := json.Unmarshal([]byte(`{"id":"r1","type":"rectangle","border_color":"#000","border_style":"","color":"#fff","thickness":1,"x1":1,"y1":2,"x2":3,"y2":4,"z_index":0,"rotation":0,"border_radius":1}`), &a) + assert.NoError(t, err) + assert.Equal(t, AnnotationTypeRectangle, a.Type) + assert.Nil(t, a.Text) + assert.NotNil(t, a.Rectangle) + assert.Nil(t, a.Ellipse) + assert.Nil(t, a.Line) +} + +func TestAnnotation_Unmarshal_NonObjectJSON(t *testing.T) { + var a Annotation + err := json.Unmarshal([]byte(`123`), &a) + assert.Error(t, err) +} diff --git a/pkg/models/imagedef_test.go b/pkg/models/imagedef_test.go index f544291..0275001 100644 --- a/pkg/models/imagedef_test.go +++ b/pkg/models/imagedef_test.go @@ -37,3 +37,39 @@ func TestImageDefinition_Unmarshal_Schema210Fields(t *testing.T) { assert.Equal(t, "IOSv", id.Label) assert.Equal(t, "d1.qcow2", id.DiskImage1()) } + +func TestImageDefinition_DiskImage1_EmptyWhenNil(t *testing.T) { + id := ImageDefinition{ID: "img1"} + assert.Equal(t, "", id.DiskImage1()) +} + +func TestImageDefinition_Unmarshal_MissingDiskImage(t *testing.T) { + var id ImageDefinition + err := json.Unmarshal([]byte(`{"id":"img1","node_definition_id":"iosv","label":"IOSv","read_only":false}`), &id) + assert.NoError(t, err) + assert.Nil(t, id.DiskImage) + assert.Equal(t, "", id.DiskImage1()) +} + +func TestImageDefinition_Unmarshal_NullDiskImage(t *testing.T) { + var id ImageDefinition + err := json.Unmarshal([]byte(`{"id":"img1","node_definition_id":"iosv","label":"IOSv","disk_image":null,"read_only":false}`), &id) + assert.NoError(t, err) + assert.Nil(t, id.DiskImage) + assert.Equal(t, "", id.DiskImage1()) +} + +func TestImageDefinition_Unmarshal_OmitsOptionalFields(t *testing.T) { + var id ImageDefinition + err := json.Unmarshal([]byte(`{ + "id":"img1", + "node_definition_id":"iosv", + "label":"IOSv", + "disk_image":"d1.qcow2", + "read_only":false + }`), &id) + assert.NoError(t, err) + assert.Equal(t, "d1.qcow2", id.DiskImage1()) + assert.Equal(t, "", id.SchemaVersion) + assert.Equal(t, "", id.Description) +} From b76a253d258c7e523190cbecfaa617df97bc294f Mon Sep 17 00:00:00 2001 From: Ralph Schmieder Date: Wed, 18 Mar 2026 11:36:59 +0100 Subject: [PATCH 3/5] ci: enhance CI workflow with linting support - add `lint` job in GitHub Actions after build - integrate `go vet` and `golangci-lint-action v6` for linting - update Makefile with phony targets `test`, `vet`, `lint` - revise README.md with new linting instructions --- .github/workflows/go.yml | 20 ++++++++++++++++++++ Makefile | 11 +++++++++++ README.md | 4 ++++ 3 files changed, 35 insertions(+) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d5d23ec..39da99f 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -39,6 +39,26 @@ jobs: - run: go mod download - run: go build -v ./... + lint: + name: Lint + needs: build + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v5 + - uses: actions/setup-go@v6 + with: + go-version-file: go.mod + cache: true + - run: go mod download + - name: go vet + run: go vet ./... + - name: golangci-lint + uses: golangci/golangci-lint-action@v6 + with: + version: v2.11.3 + args: --timeout=5m + tidy: name: go mod tidy runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 19bafba..d77cbfc 100644 --- a/Makefile +++ b/Makefile @@ -3,6 +3,17 @@ VERSION ?= .PHONY: cover clean covero coversum +.PHONY: test lint vet + +test: + go test ./... + +vet: + go vet ./... + +lint: + golangci-lint run --timeout=5m + cover: # TEST_LIVE=1 go test -v -cover -coverprofile $(NAME).out -coverpkg=./internal/...,./pkg/... ./... # go test -v -cover -coverprofile $(NAME).out -covermode=atomic -coverpkg=./internal/...,./pkg/... ./... diff --git a/README.md b/README.md index c66609f..e26d49a 100644 --- a/README.md +++ b/README.md @@ -553,7 +553,11 @@ go test ./... go test -race ./... # Run linting +make lint + +# Or, without make: go vet ./... +golangci-lint run ``` ### Code Style From 998b8c5540acb9c38744c7b738a10968faac7720 Mon Sep 17 00:00:00 2001 From: Ralph Schmieder Date: Wed, 18 Mar 2026 11:46:30 +0100 Subject: [PATCH 4/5] (fix) golangci-lint-action v6->v7 --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 39da99f..feee62b 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -54,7 +54,7 @@ jobs: - name: go vet run: go vet ./... - name: golangci-lint - uses: golangci/golangci-lint-action@v6 + uses: golangci/golangci-lint-action@v7 with: version: v2.11.3 args: --timeout=5m From c7f9ab0f936fd58a4c136d0d2de05b87c65298da Mon Sep 17 00:00:00 2001 From: Ralph Schmieder Date: Wed, 18 Mar 2026 14:09:51 +0100 Subject: [PATCH 5/5] feat(annotation): introduce LineStyle constants and JSON handling - add LineStyle constants: LineStyleArrow, LineStyleSquare, LineStyleCircle - implement LineStyle.MarshalJSON to handle unknown values - update LineAnnotationPartial JSON tags to include null keys for PATCH requests - add tests for LineStyle marshalling and validation - bump version constraint to >=2.9.0 in system services - adjust version strings in tests and documentation - update README with compatibility notes and examples --- README.md | 36 ++++++++++++++-------- integration/system_it_test.go | 2 +- integration/topology_triangle_test.go | 18 +++++++++-- internal/services/system.go | 2 +- internal/services/system_test.go | 44 +++++++++++++-------------- internal/testutil/client.go | 2 +- pkg/client/client_test.go | 4 +-- pkg/models/annotation.go | 31 +++++++++++++++++-- pkg/models/annotation_test.go | 31 +++++++++++++++++++ 9 files changed, 126 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index e26d49a..e37f863 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ A comprehensive Go client library for Cisco Modeling Labs (CML) 2.x, providing m ## Features - 🚀 **Modern Service Architecture**: Clean, modular design with dedicated services for each resource type -- 🔄 **Full Backward Compatibility**: Drop-in replacement for existing gocmlclient code +- 🔄 **Focused API**: Modern, service-based client (not a drop-in replacement for older gocmlclient versions) - 🔐 **Flexible Authentication**: Support for username/password, tokens, and custom providers - 🛡️ **Production Ready**: Comprehensive error handling, retries, and connection management - 📊 **Built-in Monitoring**: Request/response statistics and health checks @@ -46,7 +46,7 @@ go get github.com/rschmied/gocmlclient **Requirements:** - Go 1.25 or later -- Access to a CML 2.x controller (version 2.4.0+) +- Access to a CML 2.x controller (version 2.9.0+ recommended; 2.9/2.10 tested) ## Quick Start @@ -86,11 +86,11 @@ func main() { By default, the client automatically performs a system readiness check during initialization to ensure the CML server is compatible and ready. This check: - Verifies the server is running and accessible -- Validates version compatibility (>=2.4.0, <3.0.0) +- Validates version compatibility (>=2.9.0, <3.0.0) - Caches version information for subsequent operations - Checks for named configuration support (>=2.7.0) -If you need to skip this check (e.g., for testing or when working with servers that don't support the system_information endpoint): +If you need to skip this check (e.g., for testing or when working with servers that don't support the system_information endpoint) or when working with older versions (might or might not work, untested): ```go client, err := gocmlclient.New("https://cml-controller.example.com", @@ -378,16 +378,26 @@ create := models.AnnotationCreate{ } ann, err := client.Annotation.Create(ctx, "lab-uuid", create) -// List annotations -anns, err := client.Annotation.List(ctx, "lab-uuid") + // List annotations + anns, err := client.Annotation.List(ctx, "lab-uuid") -// Patch an annotation (OpenAPI requires `type`) -updated := "hello-updated" -upd := models.AnnotationUpdate{Type: models.AnnotationTypeText, Text: &models.TextAnnotationPartial{Type: models.AnnotationTypeText, TextContent: &updated}} -ann, err = client.Annotation.Update(ctx, "lab-uuid", ann.Text.ID, upd) + // Patch an annotation (OpenAPI requires `type`) + updated := "hello-updated" + upd := models.AnnotationUpdate{Type: models.AnnotationTypeText, Text: &models.TextAnnotationPartial{Type: models.AnnotationTypeText, TextContent: &updated}} + ann, err = client.Annotation.Update(ctx, "lab-uuid", ann.Text.ID, upd) -// Delete -err = client.Annotation.Delete(ctx, "lab-uuid", ann.Text.ID) + // Line annotations: line_start/line_end are required but may be null. + // On PATCH, gocmlclient always includes these keys so callers can send explicit nulls. + arrow := models.LineStyleArrow + lineCreate := models.AnnotationCreate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotation{Type: models.AnnotationTypeLine, BorderColor: "#000000", BorderStyle: "", Color: "#ffffff", Thickness: 1, X1: 10, Y1: 10, X2: 100, Y2: 10, ZIndex: 0, LineStart: &arrow, LineEnd: &arrow}} + line, err := client.Annotation.Create(ctx, "lab-uuid", lineCreate) + if line.Line != nil { + // Clear both line ends (explicit JSON null) + _, err = client.Annotation.Update(ctx, "lab-uuid", line.Line.ID, models.AnnotationUpdate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotationPartial{Type: models.AnnotationTypeLine, LineStart: nil, LineEnd: nil}}) + } + + // Delete + err = client.Annotation.Delete(ctx, "lab-uuid", ann.Text.ID) // Smart annotations smart, err := client.SmartAnnotation.List(ctx, "lab-uuid") @@ -405,7 +415,7 @@ Access system-level information and configuration. version := client.System.Version() // Check version compatibility -compatible, err := client.System.VersionCheck(ctx, ">=2.4.0") +compatible, err := client.System.VersionCheck(ctx, ">=2.9.0") // Check system readiness err = client.System.Ready(ctx) diff --git a/integration/system_it_test.go b/integration/system_it_test.go index f85f47d..cbfbb39 100644 --- a/integration/system_it_test.go +++ b/integration/system_it_test.go @@ -16,5 +16,5 @@ func TestIntegration_System(t *testing.T) { } _ = c.System.Version() - _, _ = c.System.VersionCheck(ctx, ">=2.4.0") + _, _ = c.System.VersionCheck(ctx, ">=2.9.0") } diff --git a/integration/topology_triangle_test.go b/integration/topology_triangle_test.go index adcdd6a..3f633d7 100644 --- a/integration/topology_triangle_test.go +++ b/integration/topology_triangle_test.go @@ -186,17 +186,31 @@ func TestIntegration_TriangleWithExtConnAndUnmanagedSwitch(t *testing.T) { requireNoErrorOrSkipStatus(t, err, 400, 403, 404) } - arrow := models.LineStyle("arrow") - _, err = annSvc.Create(ctx, lab.ID, models.AnnotationCreate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotation{Type: models.AnnotationTypeLine, BorderColor: "#0b132bff", BorderStyle: "", Color: "#5bc0becc", Thickness: 1, X1: 100, Y1: 250, X2: 300, Y2: 250, ZIndex: 7, LineStart: &arrow, LineEnd: &arrow}}) + arrow := models.LineStyleArrow + circle := models.LineStyleCircle + square := models.LineStyleSquare + createdLine, err := annSvc.Create(ctx, lab.ID, models.AnnotationCreate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotation{Type: models.AnnotationTypeLine, BorderColor: "#0b132bff", BorderStyle: "", Color: "#5bc0becc", Thickness: 1, X1: 100, Y1: 250, X2: 300, Y2: 250, ZIndex: 7, LineStart: &arrow, LineEnd: &circle}}) if err != nil { requireNoErrorOrSkipStatus(t, err, 400, 403, 404) } + if createdLine.Line != nil { + // Exercise update semantics: explicit null line_start/line_end clears markers. + _, err = annSvc.Update(ctx, lab.ID, createdLine.Line.ID, models.AnnotationUpdate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotationPartial{Type: models.AnnotationTypeLine, LineStart: nil, LineEnd: nil}}) + if err != nil { + requireNoErrorOrSkipStatus(t, err, 400, 403, 404) + } + } _, err = annSvc.Create(ctx, lab.ID, models.AnnotationCreate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotation{Type: models.AnnotationTypeLine, BorderColor: "#0b132bff", BorderStyle: "", Color: "#6fffe9cc", Thickness: 1, X1: 100, Y1: 270, X2: 300, Y2: 270, ZIndex: 6, LineStart: nil, LineEnd: nil}}) if err != nil { requireNoErrorOrSkipStatus(t, err, 400, 403, 404) } + _, err = annSvc.Create(ctx, lab.ID, models.AnnotationCreate{Type: models.AnnotationTypeLine, Line: &models.LineAnnotation{Type: models.AnnotationTypeLine, BorderColor: "#0b132bff", BorderStyle: "", Color: "#ffd166cc", Thickness: 1, X1: 100, Y1: 290, X2: 300, Y2: 290, ZIndex: 5, LineStart: &square, LineEnd: nil}}) + if err != nil { + requireNoErrorOrSkipStatus(t, err, 400, 403, 404) + } + // start the lab err = c.Lab.Start(ctx, lab.ID) if err != nil { diff --git a/internal/services/system.go b/internal/services/system.go index eb351d8..473b25b 100644 --- a/internal/services/system.go +++ b/internal/services/system.go @@ -49,7 +49,7 @@ func NewSystemService(apiClient *api.Client) *SystemService { // 2.5.0-dev0+build.3.2f7875762 const ( - versionConstraint = ">=2.4.0,<3.0.0" + versionConstraint = ">=2.9.0,<3.0.0" namedConfigsConstraint = ">=2.7.0" versionRegexPattern = `^(\d+\.\d+\.\d+)((-dev0)?\+build.*)?$` ) diff --git a/internal/services/system_test.go b/internal/services/system_test.go index e945aec..940d6d1 100644 --- a/internal/services/system_test.go +++ b/internal/services/system_test.go @@ -66,15 +66,15 @@ func TestVersionCheck(t *testing.T) { }) t.Run("unknown version", func(t *testing.T) { - compatible, err := service.VersionCheck(ctx, ">=2.4.0") + compatible, err := service.VersionCheck(ctx, ">=2.9.0") assert.False(t, compatible) assert.Error(t, err) assert.Contains(t, err.Error(), "version unknown") }) t.Run("valid version and constraint", func(t *testing.T) { - service.version = "2.5.0" - compatible, err := service.VersionCheck(ctx, ">=2.4.0") + service.version = "2.10.0" + compatible, err := service.VersionCheck(ctx, ">=2.9.0") assert.NoError(t, err) assert.True(t, compatible) }) @@ -99,35 +99,35 @@ func TestCheckVersionConstraint(t *testing.T) { }{ { name: "valid version satisfies constraint", - version: "2.5.0", - constraint: ">=2.4.0", + version: "2.10.0", + constraint: ">=2.9.0", expectError: false, expected: true, }, { name: "valid version does not satisfy constraint", - version: "2.3.0", - constraint: ">=2.4.0", + version: "2.8.0", + constraint: ">=2.9.0", expectError: false, expected: false, }, { name: "dev version", - version: "2.5.0-dev0+build.abc123", - constraint: ">=2.4.0", + version: "2.10.0-dev0+build.abc123", + constraint: ">=2.9.0", expectError: false, expected: true, }, { name: "invalid version format", version: "invalid", - constraint: ">=2.4.0", + constraint: ">=2.9.0", expectError: true, expected: false, }, { name: "invalid constraint", - version: "2.5.0", + version: "2.10.0", constraint: "invalid", expectError: true, expected: false, @@ -151,7 +151,7 @@ func TestVersionError(t *testing.T) { err := versionError("1.0.0") assert.Error(t, err) assert.Contains(t, err.Error(), "server not compatible") - assert.Contains(t, err.Error(), ">=2.4.0,<3.0.0") + assert.Contains(t, err.Error(), ">=2.9.0,<3.0.0") assert.Contains(t, err.Error(), "1.0.0") } @@ -227,7 +227,7 @@ func TestSystemServiceIntegration(t *testing.T) { assert.Equal(t, "/api/v0/system_information", r.URL.Path) w.WriteHeader(http.StatusOK) - w.Write([]byte(`{"version": "2.5.0", "ready": true}`)) //nolint:errcheck + w.Write([]byte(`{"version": "2.10.0", "ready": true}`)) //nolint:errcheck })) defer server.Close() @@ -240,10 +240,10 @@ func TestSystemServiceIntegration(t *testing.T) { assert.NoError(t, err) // Verify version is set - assert.Equal(t, "2.5.0", service.Version()) + assert.Equal(t, "2.10.0", service.Version()) // Test VersionCheck - compatible, err := service.VersionCheck(ctx, ">=2.4.0") + compatible, err := service.VersionCheck(ctx, ">=2.9.0") assert.NoError(t, err) assert.True(t, compatible) @@ -264,18 +264,18 @@ func TestSystemService_VersionConstraints(t *testing.T) { service := NewSystemService(client) // Set version manually for testing - service.version = "2.5.0" + service.version = "2.10.0" // Test valid version constraints - compatible, err := service.VersionCheck(context.Background(), ">=2.4.0") + compatible, err := service.VersionCheck(context.Background(), ">=2.9.0") assert.NoError(t, err) assert.True(t, compatible) - compatible, err = service.VersionCheck(context.Background(), ">=2.6.0") + compatible, err = service.VersionCheck(context.Background(), ">=2.11.0") assert.NoError(t, err) assert.False(t, compatible) - compatible, err = service.VersionCheck(context.Background(), ">=2.4.0,<3.0.0") + compatible, err = service.VersionCheck(context.Background(), ">=2.9.0,<3.0.0") assert.NoError(t, err) assert.True(t, compatible) @@ -299,7 +299,7 @@ func TestSystemService_Ready_Success(t *testing.T) { // Mock successful system info response httpmock.RegisterResponder("GET", "https://mock/api/v0/system_information", httpmock.NewStringResponder(200, `{ - "version": "2.5.0", + "version": "2.10.0", "ready": true }`)) @@ -309,7 +309,7 @@ func TestSystemService_Ready_Success(t *testing.T) { err := service.Ready(ctx) assert.NoError(t, err) - assert.Equal(t, "2.5.0", service.Version()) + assert.Equal(t, "2.10.0", service.Version()) } func TestSystemService_Ready_SystemNotReady(t *testing.T) { @@ -323,7 +323,7 @@ func TestSystemService_Ready_SystemNotReady(t *testing.T) { // Mock system not ready response httpmock.RegisterResponder("GET", "https://mock/api/v0/system_information", httpmock.NewStringResponder(200, `{ - "version": "2.5.0", + "version": "2.10.0", "ready": false }`)) diff --git a/internal/testutil/client.go b/internal/testutil/client.go index b529e5d..9ab352f 100644 --- a/internal/testutil/client.go +++ b/internal/testutil/client.go @@ -148,5 +148,5 @@ func SetupCommonMocks() { httpmock.NewStringResponder(200, `{"id":"user-123","username":"testuser","token":"mock-token-12345","admin":false}`)) httpmock.RegisterResponder("GET", "https://mock/api/v0/system_information", - httpmock.NewStringResponder(200, `{"ready":true,"version":"2.8.1","build":"123"}`)) + httpmock.NewStringResponder(200, `{"ready":true,"version":"2.10.0","build":"123"}`)) } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 66fbefe..be756fd 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -156,7 +156,7 @@ func TestReadyCheckIntegration(t *testing.T) { switch r.URL.Path { case "/api/v0/system_information": w.WriteHeader(http.StatusOK) - w.Write([]byte(`{"version": "2.5.0", "ready": true}`)) //nolint:errcheck + w.Write([]byte(`{"version": "2.10.0", "ready": true}`)) //nolint:errcheck case "/api/v0/auth_extended": w.WriteHeader(http.StatusOK) w.Write([]byte(`{"id":"user-123","username":"testuser","token":"mock-token-12345","admin":false}`)) //nolint:errcheck @@ -170,7 +170,7 @@ func TestReadyCheckIntegration(t *testing.T) { client, err := New(server.URL, WithToken("test-token")) assert.NoError(t, err) assert.NotNil(t, client) - assert.Equal(t, "2.5.0", client.System.Version()) + assert.Equal(t, "2.10.0", client.System.Version()) } func TestNewAPIClient(t *testing.T) { diff --git a/pkg/models/annotation.go b/pkg/models/annotation.go index b008ed1..af7e49a 100644 --- a/pkg/models/annotation.go +++ b/pkg/models/annotation.go @@ -29,6 +29,31 @@ type BorderStyle string // Values observed in schema: "arrow", "square", "circle". type LineStyle string +const ( + // LineStyleArrow represents the Arrow line end + LineStyleArrow LineStyle = "arrow" + // LineStyleSquare represents the Square line end + LineStyleSquare LineStyle = "square" + // LineStyleCircle represents the Circle line end + LineStyleCircle LineStyle = "circle" +) + +// MarshalJSON validates LineStyle values during marshaling. +// +// The server treats line markers as an enum and rejects empty strings. +// Enforcing this client-side prevents accidentally emitting invalid payloads. +func (s LineStyle) MarshalJSON() ([]byte, error) { + if s == "" { + return nil, fmt.Errorf("line style cannot be empty") + } + switch s { + case LineStyleArrow, LineStyleSquare, LineStyleCircle: + return json.Marshal(string(s)) + default: + return nil, fmt.Errorf("invalid line style %q", s) + } +} + // Annotation is a discriminated union wrapper. // Exactly one of Text/Rectangle/Ellipse/Line is set after unmarshaling. type Annotation struct { @@ -334,8 +359,10 @@ type LineAnnotationPartial struct { X1 *float64 `json:"x1,omitempty"` Y1 *float64 `json:"y1,omitempty"` ZIndex *float64 `json:"z_index,omitempty"` - LineStart *LineStyle `json:"line_start,omitempty"` - LineEnd *LineStyle `json:"line_end,omitempty"` + // line_start/line_end are required by the schema but may be null. + // For updates, we always include these keys so callers can send explicit null. + LineStart *LineStyle `json:"line_start"` + LineEnd *LineStyle `json:"line_end"` } // LineAnnotationResponse represents a line annotation with ID. diff --git a/pkg/models/annotation_test.go b/pkg/models/annotation_test.go index fd9563b..f5dcf96 100644 --- a/pkg/models/annotation_test.go +++ b/pkg/models/annotation_test.go @@ -227,6 +227,37 @@ func TestLineAnnotationCreate_Marshal_EmitsNullLineEnds(t *testing.T) { assert.Nil(t, m["line_end"]) } +func TestLineAnnotationUpdate_Marshal_EmitsNullLineEnds(t *testing.T) { + in := AnnotationUpdate{Type: AnnotationTypeLine, Line: &LineAnnotationPartial{Type: AnnotationTypeLine, LineStart: nil, LineEnd: nil}} + b, err := json.Marshal(in) + assert.NoError(t, err) + var m map[string]any + assert.NoError(t, json.Unmarshal(b, &m)) + assert.Contains(t, m, "line_start") + assert.Contains(t, m, "line_end") + assert.Nil(t, m["line_start"]) + assert.Nil(t, m["line_end"]) +} + +func TestLineAnnotationUpdate_Marshal_LineEndsWithValues(t *testing.T) { + start := LineStyleArrow + end := LineStyleCircle + in := AnnotationUpdate{Type: AnnotationTypeLine, Line: &LineAnnotationPartial{Type: AnnotationTypeLine, LineStart: &start, LineEnd: &end}} + b, err := json.Marshal(in) + assert.NoError(t, err) + var m map[string]any + assert.NoError(t, json.Unmarshal(b, &m)) + assert.Equal(t, "arrow", m["line_start"]) + assert.Equal(t, "circle", m["line_end"]) +} + +func TestLineStyle_MarshalJSON_RejectsEmptyOrUnknown(t *testing.T) { + _, err := json.Marshal(LineStyle("")) + assert.Error(t, err) + _, err = json.Marshal(LineStyle("bogus")) + assert.Error(t, err) +} + func TestAnnotation_Unmarshal_ClearsOtherUnionFields(t *testing.T) { // Start with a non-empty Annotation value. a := Annotation{