feat!: pass required query, header, and cookie params via a named Params struct (#26)

Author: giraffesyo

CLAUDE.md

@@ -0,0 +1,70 @@
+# CLAUDE.md
+
+This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
+
+## Overview
+
+`openapi-client-generator` is a Go code generator that turns an OpenAPI 3.1 (or 3.0) spec into a complete, idiomatic Go HTTP client package with zero external runtime dependencies.
+
+The main branch is `canary`. Releases are managed by release-please (conventional-commit driven).
+
+## Pipeline
+
+A spec flows through three stages, one per `internal/` package:
+
+1. **parse** (`internal/parser`) — load and validate the spec with libopenapi, resolve `$ref`s.
+2. **analyze** (`internal/analyzer`) — lower the spec into the intermediate representation (`internal/ir`): operations, params, types, pagination. This is where naming, disambiguation, and pagination detection happen.
+3. **generate** (`internal/generator`) — render the IR through `text/template` files (`internal/templates/*.tmpl`, embedded via `embed.go`) and post-process each file with `goimports` (`WriteFiles`).
+
+Supporting packages: `internal/naming` (OpenAPI identifier → Go identifier), `cmd/` (the Cobra CLI).
+
+The generated code is the product. It must always compile — the e2e tests in `internal/generator` generate clients from specs and run `go build`/`go test` on the output.
+
+## Commands
+
+```bash
+go build ./...                 # build
+go test ./...                  # run all tests (incl. generate-and-compile e2e tests)
+go test ./internal/generator/  # one package
+gofmt -l .                     # list unformatted files
+go vet ./...
+
+# Run the generator
+go run . generate --spec testdata/petstore.yaml --out ./gen/petstore
+```
+
+Template changes don't need a build step — templates are `go:embed`ed and read at generate time.
+
+## Code Standards
+
+### Commits
+Conventional commits; PR titles become changelog entries.
+- `feat:` new feature · `fix:` bug fix · `docs:` · `chore:` build/tooling · `refactor:`
+- `!` for breaking changes (`feat!:`); scope for context (`fix(generator):`).
+- For `fix` titles, describe the **broken behavior** the user saw, not the code action. For `feat`, don't start with "add" — the title is the new capability.
+
+### Comments
+Default to writing no comments. Only add one when the WHY is non-obvious: a hidden constraint, a subtle invariant, a workaround for a specific bug, behavior that would surprise a reader. If removing the comment wouldn't confuse a future reader, don't write it. Specifically avoid:
+- WHAT comments. Names and code already say what — `// hasRequiredQueryParams returns whether there are required query params` above a function of that name is noise.
+- Conventions that hold across the package (e.g. "wire names use OrigName" is true everywhere, so don't repeat it on each helper).
+- References to current task / fix / callers (`// so foo_test.go can exercise this`, `// matching how auth cookies are built`, `// added for the X flow`) — they rot. Such context belongs in the PR description.
+- Multi-paragraph docstrings. One short line max — if you need more, the function is doing too much.
+- Decorative section headers (`// ===== Helpers =====`). Use blank lines or split files.
+
+Note: a generated client's exported types/methods are SDK documentation for its consumers, so a concise doc comment there is appropriate. Unexported template helpers (`addQueryParam`, etc.) are internal — apply the rules above.
+
+### DRY
+When the same logic appears in multiple places, extract a shared helper. Before writing new code, check whether an existing helper already does it. When adding encoding/naming/forwarding that mirrors existing code, reuse it rather than duplicating.
+
+### Go
+- Don't `panic`; return errors.
+- Use `errors.Is`/`errors.As` for error checks, not equality, so wrapped errors match.
+- Use `time.Time` for timestamps; let the JSON marshaler format them.
+- File names: lowercase, no separators (`funcmap.go`) or underscores (`api_key.go`) — no camelCase or hyphens. Test files end `_test.go`.
+- Package directories under `internal/`: lowercase, no separators.
+- All code must pass `gofmt` and `go vet`.
+
+### Testing
+- **Test real code.** Tests import and call the actual functions under test — never reimplement the logic being tested. For the generator, the strongest tests generate a client from a spec and then compile/run it (`internal/generator/e2e_test.go`), so a broken template fails the test rather than a restated assertion.
+- Prefer `t.Context()` over `context.Background()` in tests.
+- When changing a template, add or extend an e2e test that compiles the affected operation shape.

internal/analyzer/analyzer.go

@@ -3,6 +3,7 @@ package analyzer
 import (
 	"fmt"
 
+	highbase "github.com/pb33f/libopenapi/datamodel/high/base"
 	v3high "github.com/pb33f/libopenapi/datamodel/high/v3"
 
 	"github.com/parallelworks/openapi-client-generator/internal/ir"
@@ -83,26 +84,34 @@ func (a *Analyzer) analyzeComponentSchemas(pkg *ir.Package) error {
 		return nil
 	}
 
+	// Register every component's Go type name before converting any schema, so an
+	// enum constant that sanitizes to a schema-named type (e.g. Color.red -> const
+	// ColorRed vs a ColorRed schema) yields the numeric suffix to the const, not to
+	// the user's public API type.
+	type pendingSchema struct {
+		name   string
+		goName string
+		schema *highbase.Schema
+	}
+	var pending []pendingSchema
 	for name, schemaProxy := range a.model.Components.Schemas.FromOldest() {
-		if _, exists := a.typesBySchema[name]; exists {
-			continue
-		}
-
 		schema, err := schemaProxy.BuildSchema()
 		if err != nil {
 			return fmt.Errorf("building schema %q: %w", name, err)
 		}
 		if schema == nil {
 			continue
 		}
+		pending = append(pending, pendingSchema{name, a.namer.RegisterName(naming.ToGoName(name)), schema})
+	}
 
-		goName := a.namer.RegisterName(naming.ToGoName(name))
-		td, err := a.convertSchema(goName, name, schema)
+	for _, p := range pending {
+		td, err := a.convertSchema(p.goName, p.name, p.schema)
 		if err != nil {
-			return fmt.Errorf("converting schema %q: %w", name, err)
+			return fmt.Errorf("converting schema %q: %w", p.name, err)
 		}
 		if td != nil {
-			a.typesBySchema[name] = td
+			a.typesBySchema[p.name] = td
 		}
 	}
 

internal/analyzer/analyzer_test.go

@@ -449,13 +449,13 @@ func TestAnalyzePetstore_FieldDetails(t *testing.T) {
 	if petStatus.EnumGoType != "string" {
 		t.Errorf("PetStatus.EnumGoType = %q, want string", petStatus.EnumGoType)
 	}
-	expectedEnumVals := []string{"available", "pending", "sold"}
+	expectedEnumVals := []string{`"available"`, `"pending"`, `"sold"`}
 	if len(petStatus.EnumValues) != 3 {
 		t.Fatalf("PetStatus expected 3 enum values, got %d", len(petStatus.EnumValues))
 	}
 	for i, ev := range petStatus.EnumValues {
-		if ev.Value != expectedEnumVals[i] {
-			t.Errorf("PetStatus enum value %d = %q, want %q", i, ev.Value, expectedEnumVals[i])
+		if ev.Literal != expectedEnumVals[i] {
+			t.Errorf("PetStatus enum literal %d = %q, want %q", i, ev.Literal, expectedEnumVals[i])
 		}
 	}
 }

internal/analyzer/operations.go

@@ -100,9 +100,64 @@ func (a *Analyzer) convertOperation(httpMethod, path string, pathItem *v3high.Pa
 		opDef.SecurityReqs = convertSecurityReqs(op.Security)
 	}
 
+	disambiguateParamNames(opDef)
+
 	return opDef, nil
 }
 
+// disambiguateParamNames renames generated identifiers that would otherwise
+// collide, suffixing by kind; only Go identifiers change, never the wire OrigName.
+func disambiguateParamNames(opDef *ir.OperationDef) {
+	// Reserved: the receiver, args, and method/iterator locals a path param could
+	// shadow, the package identifiers the generated body references (e.g. a param
+	// named `url` would shadow the net/url import in `url.Values{}`), and the
+	// helper functions the method body calls (a param named `add_query_param`
+	// becomes `addQueryParam`, shadowing the helper of that name).
+	posUsed := map[string]bool{
+		"c": true, "ctx": true, "path": true, "queryValues": true,
+		"headers": true, "result": true, "err": true,
+		"cursor": true, "p": true, "next": true,
+		"context": true, "fmt": true, "http": true, "url": true,
+		"pathReplace": true, "addQueryParam": true, "encodeQuery": true,
+		"setHeader": true, "addCookieHeader": true,
+	}
+	if opDef.RequestBody != nil {
+		posUsed["body"] = true
+	}
+	if len(opDef.QueryParams) > 0 || len(opDef.HeaderParams) > 0 || len(opDef.CookieParams) > 0 {
+		posUsed["params"] = true
+		posUsed["opts"] = true
+	}
+	for _, p := range opDef.PathParams {
+		name := p.Name
+		for posUsed[name] {
+			name += "Path"
+		}
+		posUsed[name] = true
+		p.Name = name
+	}
+
+	// Query, header, and cookie params share one struct, so dedupe field names by location.
+	fieldUsed := map[string]bool{}
+	dedupeField := func(p *ir.ParamDef, suffix string) {
+		name := p.FieldName
+		for fieldUsed[name] {
+			name += suffix
+		}
+		fieldUsed[name] = true
+		p.FieldName = name
+	}
+	for _, p := range opDef.QueryParams {
+		dedupeField(p, "Query")
+	}
+	for _, p := range opDef.HeaderParams {
+		dedupeField(p, "Header")
+	}
+	for _, p := range opDef.CookieParams {
+		dedupeField(p, "Cookie")
+	}
+}
+
 // operationName determines the Go method name for an operation.
 func (a *Analyzer) operationName(httpMethod, path string, op *v3high.Operation) string {
 	if op.OperationId != "" {
@@ -165,7 +220,7 @@ func (a *Analyzer) convertParam(param *v3high.Parameter) (*ir.ParamDef, error) {
 	}
 
 	required := param.Required != nil && *param.Required
-	explode := param.Explode != nil && *param.Explode
+	style, explode := effectiveStyleExplode(param)
 
 	return &ir.ParamDef{
 		Name:        naming.ToGoParamName(param.Name),
@@ -176,11 +231,32 @@ func (a *Analyzer) convertParam(param *v3high.Parameter) (*ir.ParamDef, error) {
 		Required:    required,
 		Description: param.Description,
 		Deprecated:  param.Deprecated,
-		Style:       param.Style,
+		Style:       style,
 		Explode:     explode,
 	}, nil
 }
 
+// effectiveStyleExplode resolves the OpenAPI serialization defaults: style is
+// form for query/cookie and simple for path/header when unset; explode defaults
+// to true only for form. The raw param.Explode is false when omitted, which would
+// wrongly collapse an ordinary form array — so the default must be applied here.
+func effectiveStyleExplode(param *v3high.Parameter) (string, bool) {
+	style := param.Style
+	if style == "" {
+		switch param.In {
+		case "query", "cookie":
+			style = "form"
+		default:
+			style = "simple"
+		}
+	}
+	explode := style == "form"
+	if param.Explode != nil {
+		explode = *param.Explode
+	}
+	return style, explode
+}
+
 // convertRequestBody converts an OpenAPI request body to an ir.RequestBodyDef.
 func (a *Analyzer) convertRequestBody(rb *v3high.RequestBody) (*ir.RequestBodyDef, error) {
 	def := &ir.RequestBodyDef{

internal/analyzer/operations_test.go

@@ -10,6 +10,37 @@ import (
 	"github.com/parallelworks/openapi-client-generator/internal/parser"
 )
 
+func TestEffectiveStyleExplode(t *testing.T) {
+	b := func(v bool) *bool { return &v }
+	tests := []struct {
+		name        string
+		in          string
+		style       string
+		explode     *bool
+		wantStyle   string
+		wantExplode bool
+	}{
+		{"query default", "query", "", nil, "form", true},
+		{"query explode=false", "query", "", b(false), "form", false},
+		{"query spaceDelimited default explode", "query", "spaceDelimited", nil, "spaceDelimited", false},
+		{"query pipeDelimited explode=true", "query", "pipeDelimited", b(true), "pipeDelimited", true},
+		{"header default", "header", "", nil, "simple", false},
+		{"header explode=true", "header", "", b(true), "simple", true},
+		{"path default", "path", "", nil, "simple", false},
+		{"cookie default", "cookie", "", nil, "form", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := &v3high.Parameter{In: tt.in, Style: tt.style, Explode: tt.explode}
+			style, explode := effectiveStyleExplode(p)
+			if style != tt.wantStyle || explode != tt.wantExplode {
+				t.Errorf("effectiveStyleExplode(in=%s style=%q explode=%v) = (%q, %v), want (%q, %v)",
+					tt.in, tt.style, tt.explode, style, explode, tt.wantStyle, tt.wantExplode)
+			}
+		})
+	}
+}
+
 func TestAnalyzeOperations_Petstore(t *testing.T) {
 	specPath := filepath.Join(projectRoot(), "testdata", "petstore.yaml")
 
@@ -310,3 +341,85 @@ func TestPathToWords(t *testing.T) {
 		}
 	}
 }
+
+// TestDisambiguateParamNames_PositionalArgs covers path params colliding with
+// the fixed positional arguments (ctx, body, and the params struct arg).
+func TestDisambiguateParamNames_PositionalArgs(t *testing.T) {
+	op := &ir.OperationDef{
+		RequestBody: &ir.RequestBodyDef{},
+		PathParams: []*ir.ParamDef{
+			// Collides with the params struct argument (op has query params).
+			{Name: "params", OrigName: "params", Location: "path", Required: true},
+			// Collides with the fixed "body" argument (request body present).
+			{Name: "body", OrigName: "body", Location: "path", Required: true},
+			// Collides with the "result" local the method body declares.
+			{Name: "result", OrigName: "result", Location: "path", Required: true},
+		},
+		QueryParams: []*ir.ParamDef{
+			{Name: "limit", FieldName: "Limit", OrigName: "limit", Location: "query", Required: false},
+		},
+	}
+
+	disambiguateParamNames(op)
+
+	if got := op.PathParams[0].Name; got != "paramsPath" {
+		t.Errorf("path param colliding with params arg: Name = %q, want %q", got, "paramsPath")
+	}
+	if got := op.PathParams[1].Name; got != "bodyPath" {
+		t.Errorf("path param colliding with body arg: Name = %q, want %q", got, "bodyPath")
+	}
+	if got := op.PathParams[2].Name; got != "resultPath" {
+		t.Errorf("path param colliding with the 'result' method local: Name = %q, want %q", got, "resultPath")
+	}
+	// Wire names must never change — only the Go identifier is disambiguated.
+	if got := op.PathParams[0].OrigName; got != "params" {
+		t.Errorf("OrigName changed to %q, want %q", got, "params")
+	}
+}
+
+// TestDisambiguateParamNames_NoStructArg confirms "params"/"opts" are only
+// reserved when the op actually has a params struct argument.
+func TestDisambiguateParamNames_NoStructArg(t *testing.T) {
+	op := &ir.OperationDef{
+		PathParams: []*ir.ParamDef{
+			{Name: "params", OrigName: "params", Location: "path", Required: true},
+		},
+	}
+	disambiguateParamNames(op)
+	if got := op.PathParams[0].Name; got != "params" {
+		t.Errorf("path param Name = %q, want %q (no params struct arg to collide with)", got, "params")
+	}
+}
+
+// TestDisambiguateParamNames_StructFields covers query/header/cookie params
+// whose Go field names collide inside the shared params struct.
+func TestDisambiguateParamNames_StructFields(t *testing.T) {
+	op := &ir.OperationDef{
+		QueryParams: []*ir.ParamDef{
+			{Name: "userId", FieldName: "UserId", OrigName: "user-id", Location: "query", Required: true},
+		},
+		HeaderParams: []*ir.ParamDef{
+			// PascalCases to the same "UserId" as the query param above.
+			{Name: "userId", FieldName: "UserId", OrigName: "user_id", Location: "header", Required: true},
+		},
+		CookieParams: []*ir.ParamDef{
+			{Name: "userId", FieldName: "UserId", OrigName: "userId", Location: "cookie", Required: false},
+		},
+	}
+
+	disambiguateParamNames(op)
+
+	if got := op.QueryParams[0].FieldName; got != "UserId" {
+		t.Errorf("first field = %q, want %q (first occurrence keeps its name)", got, "UserId")
+	}
+	if got := op.HeaderParams[0].FieldName; got != "UserIdHeader" {
+		t.Errorf("colliding header field = %q, want %q", got, "UserIdHeader")
+	}
+	if got := op.CookieParams[0].FieldName; got != "UserIdCookie" {
+		t.Errorf("colliding cookie field = %q, want %q", got, "UserIdCookie")
+	}
+	// Wire names are untouched, so encoding still uses the spec names.
+	if got := op.HeaderParams[0].OrigName; got != "user_id" {
+		t.Errorf("OrigName changed to %q, want %q", got, "user_id")
+	}
+}

internal/analyzer/pagination.go

@@ -36,10 +36,11 @@ func (a *Analyzer) detectPagination(pkg *ir.Package) {
 
 // detectCursorPagination checks if an operation uses cursor-based pagination.
 func (a *Analyzer) detectCursorPagination(op *ir.OperationDef, pkg *ir.Package) *ir.PaginationDef {
-	// Find a cursor query parameter.
+	// The cursor param must be optional: the iterator drives it as a pointer field
+	// it sets each page, so a required (value) cursor can't be paginated.
 	cursorParam := ""
 	for _, p := range op.QueryParams {
-		if containsCI(cursorParamNames, p.OrigName) {
+		if !p.Required && containsCI(cursorParamNames, p.OrigName) {
 			cursorParam = p.OrigName
 			break
 		}