From ccc55050d8358fcb51db9dd39d66a8ca9d99c251 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Tue, 25 Aug 2026 20:22:57 +0200 Subject: [PATCH 1/2] fix: clone a specification through JSON, not gob cloneSpec went through gob, which omits a struct field holding the zero value for its type and flattens a pointer to what it points at. An optional number that was present and zero therefore came back nil: "minimum": 0, "maximum": 0, "multipleOf": 0, "maxLength": 0, "maxItems": 0 and "maxProperties": 0 were all dropped from the copy, at any depth and with no error. OrigSpec returns that copy, and ResetDefinitions writes it back over the live definitions. go-swagger marshals OrigSpec into SwaggerJSON, which it embeds in every generated server and loads again at runtime, so those constraints were missing from the specification a generated server serves. The clone now goes through jsonutils.FromDynamicJSON, as validate's own clone already does. It is cheaper too: measured in go-openapi/spec, round-tripping clickmeter.json costs 38.8ms, 11.8MB and 150k allocations through JSON against 46.6ms, 25.6MB and 222k through gob. The gob.Register calls in the package init go with it. spec registers the same two types in its own init, and spec is imported here, so a caller who encodes these types with gob still finds them registered. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- clone_test.go | 100 ++++++++++++++++++++++++++ go.mod | 2 +- spec.go | 21 +++--- testdata/json/zero-valued-bounds.json | 82 +++++++++++++++++++++ 4 files changed, 192 insertions(+), 13 deletions(-) create mode 100644 clone_test.go create mode 100644 testdata/json/zero-valued-bounds.json diff --git a/clone_test.go b/clone_test.go new file mode 100644 index 0000000..d215dea --- /dev/null +++ b/clone_test.go @@ -0,0 +1,100 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package loads + +import ( + _ "embed" + "encoding/json" + "strings" + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +//go:embed testdata/json/zero-valued-bounds.json +var zeroValuedBoundsJSON []byte + +// TestCloneSpecCarriesZeroValuedBounds covers what gob used to drop from the copy Analyzed takes. +// +// gob omits a struct field holding the zero value for its type and flattens a pointer to what it +// points at, so an optional number that was present and zero came back nil. "minimum": 0, +// "maximum": 0, "maxLength": 0, "maxItems": 0 and "maxProperties": 0 all disappeared, at any +// depth and with no error - from OrigSpec, which go-swagger marshals into the specification it +// embeds in a generated server, and from the definitions ResetDefinitions copies back. +func TestCloneSpecCarriesZeroValuedBounds(t *testing.T) { + t.Parallel() + + document, err := Analyzed(json.RawMessage(zeroValuedBoundsJSON), "") + require.NoError(t, err) + + t.Run("the copy agrees with the document it was taken from", func(t *testing.T) { + assert.JSONMarshalAsT(t, zeroValuedBoundsJSON, document.OrigSpec()) + }) + + t.Run("resetting the definitions keeps them", func(t *testing.T) { + document.ResetDefinitions() + assert.JSONMarshalAsT(t, zeroValuedBoundsJSON, document.Spec()) + }) + + t.Run("every bound in the fixture is a zero", func(t *testing.T) { + // guards the fixture itself: a bound respelled to a non-zero value would make the + // test pass without exercising anything + var raw map[string]any + require.NoError(t, json.Unmarshal(zeroValuedBoundsJSON, &raw)) + + bounds := countZeroBounds(raw) + require.Positive(t, bounds.total, "the fixture holds no bound at all") + assert.EqualTf(t, bounds.total, bounds.zero, + "%d of the fixture's %d bounds are not zero", bounds.total-bounds.zero, bounds.total) + }) +} + +type boundCount struct{ total, zero int } + +// countZeroBounds walks raw JSON and counts the optional numbers gob elides. +func countZeroBounds(node any) boundCount { + var count boundCount + + switch value := node.(type) { + case map[string]any: + for key, child := range value { + if isBoundKeyword(key) { + number, ok := child.(float64) + if ok { + count.total++ + if number == 0 { + count.zero++ + } + + continue + } + } + + nested := countZeroBounds(child) + count.total += nested.total + count.zero += nested.zero + } + case []any: + for _, child := range value { + nested := countZeroBounds(child) + count.total += nested.total + count.zero += nested.zero + } + } + + return count +} + +func isBoundKeyword(key string) bool { + switch key { + case "minimum", "maximum", "multipleOf", + "minLength", "maxLength", + "minItems", "maxItems", + "minProperties", "maxProperties": + return true + default: + return strings.HasPrefix(key, "min") || strings.HasPrefix(key, "max") + } +} diff --git a/go.mod b/go.mod index 34a5fe3..0fff8ee 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/go-openapi/loads require ( github.com/go-openapi/analysis v0.26.0 github.com/go-openapi/spec v0.22.9 + github.com/go-openapi/swag/jsonutils v0.29.0 github.com/go-openapi/swag/loading v0.29.0 github.com/go-openapi/swag/yamlutils v0.29.0 github.com/go-openapi/testify/enable/yaml/v2 v2.6.1 @@ -16,7 +17,6 @@ require ( github.com/go-openapi/jsonreference v1.0.0 // indirect github.com/go-openapi/strfmt v0.27.0 // indirect github.com/go-openapi/swag/conv v0.29.0 // indirect - github.com/go-openapi/swag/jsonutils v0.29.0 // indirect github.com/go-openapi/swag/mangling v0.28.0 // indirect github.com/go-openapi/swag/pools v0.29.0 // indirect github.com/go-openapi/swag/stringutils v0.28.0 // indirect diff --git a/spec.go b/spec.go index e9e4b57..ea64717 100644 --- a/spec.go +++ b/spec.go @@ -5,21 +5,16 @@ package loads import ( "bytes" - "encoding/gob" "encoding/json" "fmt" "maps" "github.com/go-openapi/analysis" "github.com/go-openapi/spec" + "github.com/go-openapi/swag/jsonutils" "github.com/go-openapi/swag/yamlutils" ) -func init() { - gob.Register(map[string]any{}) - gob.Register([]any{}) -} - // Document represents a swagger spec document. type Document struct { // specAnalyzer @@ -276,14 +271,16 @@ func (d *Document) SpecFilePath() string { return d.specFilePath } +// cloneSpec deep-copies a specification through its JSON representation. +// +// It used to go through gob, which omits a field holding the zero value for its type and +// flattens a pointer to what it points at, so an optional number that was present and zero came +// back nil: "minimum": 0, "maximum": 0, "maxLength": 0, "maxItems": 0 and "maxProperties": 0 were +// dropped from the copy, at any depth and with no error. That copy is what OrigSpec returns and +// what ResetDefinitions writes back over the live definitions. func cloneSpec(src *spec.Swagger) (*spec.Swagger, error) { - var b bytes.Buffer - if err := gob.NewEncoder(&b).Encode(src); err != nil { - return nil, err - } - var dst spec.Swagger - if err := gob.NewDecoder(&b).Decode(&dst); err != nil { + if err := jsonutils.FromDynamicJSON(src, &dst); err != nil { return nil, err } diff --git a/testdata/json/zero-valued-bounds.json b/testdata/json/zero-valued-bounds.json new file mode 100644 index 0000000..d1deb0b --- /dev/null +++ b/testdata/json/zero-valued-bounds.json @@ -0,0 +1,82 @@ +{ + "swagger": "2.0", + "info": { + "title": "every optional number a swagger document may hold, set to zero", + "version": "1.0.0" + }, + "basePath": "/", + "paths": { + "/things": { + "get": { + "operationId": "listThings", + "parameters": [ + { + "name": "count", + "in": "query", + "type": "integer", + "minimum": 0, + "maximum": 0, + "multipleOf": 0 + }, + { + "name": "tags", + "in": "query", + "type": "array", + "items": { + "type": "string", + "maxLength": 0, + "minLength": 0 + }, + "maxItems": 0, + "minItems": 0 + } + ], + "responses": { + "200": { + "description": "ok", + "schema": { + "$ref": "#/definitions/thing" + }, + "headers": { + "X-Rate": { + "type": "integer", + "minimum": 0, + "maximum": 0 + } + } + } + } + } + } + }, + "definitions": { + "thing": { + "type": "object", + "maxProperties": 0, + "minProperties": 0, + "properties": { + "size": { + "type": "integer", + "minimum": 0, + "maximum": 0, + "multipleOf": 0 + }, + "name": { + "type": "string", + "maxLength": 0, + "minLength": 0 + }, + "parts": { + "type": "array", + "maxItems": 0, + "minItems": 0, + "items": { + "type": "number", + "minimum": 0, + "maximum": 0 + } + } + } + } + } +} From 862dc2b1fbbbfc8966857eabf0c244e0422a5cdb Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Tue, 25 Aug 2026 21:19:20 +0200 Subject: [PATCH 2/2] doc: add technical writing instructions for agents Signed-off-by: Frederic BIDON --- .claude/rules/technical-writing.md | 67 ++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 .claude/rules/technical-writing.md diff --git a/.claude/rules/technical-writing.md b/.claude/rules/technical-writing.md new file mode 100644 index 0000000..3f0ae36 --- /dev/null +++ b/.claude/rules/technical-writing.md @@ -0,0 +1,67 @@ +--- +paths: + - "**/*.go" + - "**/*.md" +--- + +# Technical writing style (go-openapi) + +Applies to every committed comment, commit message, README and doc-site page. + +The standard is Ernest Gowers, *Plain Words*: **be short, be simple, be human.** +His worked example is the whole rule: + + DON'T Was this the realisation of an anticipated liability? + DO Did you expect to have to do this? + +The abstract nouns carry no information; the concrete verb carries all of it. + +## Two tests + +**The grep test.** Does the sentence contain something a reader can search for — an +identifier, a file, a flag, an error, a number with a unit? Prose that names nothing has +described the code without pointing at it. + +**The quotability test.** A sentence that would survive being quoted on its own is too +pleased with itself. Rewrite it until it merely sounds true. + +## Never define by inversion + +The worst and most frequent fault. A copula whose subject or predicate is a wh-clause +promises a definition and delivers a metaphor. Both directions are banned: + + DON'T Coverage is what says which templates a suite never reaches. + DON'T What is lost is the doc comment. + DO Coverage records which templates the suite never executed. + DO A synthesized type loses its doc comment. + +The rewrite is mechanical: find the verb hiding inside the wh-clause and make it the main +verb of the sentence. + +`which is why` pointing back at a fact just stated is legitimate, and rationed — one per +comment is plenty. + +## The rest + +- **Name the thing.** `WithRoots`, not "the option that scopes a repository". Name the + error, the file, the flag, the upstream package, the constant. +- **Statement, not aphorism.** State mechanism and effect. Never close a paragraph on a + maxim: the reflex lands hardest on a closing sentence. +- **Keep a subject.** "New returns an error if the source is unreadable", not "What a + source leaves out is settled where it is declared". +- **Plain verbs.** add, fix, return, parse, reject, cap, prune, record. Code does not say, + judge, grant, refuse, know, mean to, or reach for. `report` is fine when something + genuinely reports. +- **Keep the numbers.** Sizes with units, counts, ratios, advisory ids. `286 -> 178 KiB`, + `GHSA-v2xp-g8xf-22pf`. Dropping them for a smoother sentence loses information. +- **Be human.** Address the reader where there is advice: "Use `WithRoot` to confine local + loading." Admit the awkward thing rather than smoothing it over. + +## Self-check + + # definition by inversion, both directions + grep -rnE '\b(is|are) (what|where) [a-z]' --include='*.go' --include='*.md' . + grep -rnE '(^|\. )What [a-z][a-z ,-]{3,50} (is|are) ' --include='*.go' --include='*.md' . + +Subtract the legitimate `which/that/this/it is what` before judging the first one. +Neither grep is a verdict — they find one fault out of six. The others need reading.