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. 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 + } + } + } + } + } +}