Skip to content

Commit 87ed65b

Browse files
jcogilvieclaude
jcogilvie
and
claude
committed
refactor(validate): inject Renderer via Kong's MapperValue interface
Reviewer pointed out that even with the polymorphic registry, the OutputFormat string was still flowing through the command — we were dispatching at every Render call site instead of resolving once at the boundary. Subsequent passes worked through the implications: the renderer types are stateless empty structs, so the map cache was needless ceremony; the dispatch can collapse to a single fallible boundary that takes a string-shaped format identifier and returns a typed Renderer; and the natural place for that boundary is Kong itself, which has first-class support for typed flag decoding. Final shape: * render: a single typed boundary. * OutputFormat is a defined string type with three named constants (OutputFormatText, OutputFormatJSON, OutputFormatYAML) so call sites use symbolic names rather than embedded "json"/"yaml" string literals. * RendererFor(OutputFormat) (Renderer, error) is the only public factory. Empty maps to text for ergonomics with zero-valued config; any other unrecognised value returns an error. * No format identifier or wrapper type is otherwise exposed; downstream code receives only the Renderer interface and works against the typed dependency. * validate.Cmd: gained a private rendererFlag wrapper that implements Kong's MapperValue interface, decoding --output straight into a typed render.Renderer at parse time. Cmd.Output is now of type rendererFlag (which embeds render.Renderer), so Cmd.Run calls c.Output.Render(...) directly. AfterApply only sets the filesystem; the renderer is already resolved by the time it runs. The wrapper lives in this package, not in render, so the render package stays free of any kong dependency and remains importable from non-CLI consumers like crossplane-diff. * Kong's enum:"" tag doesn't apply to MapperValue-backed fields, so rendererFlag.Decode performs the validation itself; --help text enumerates the valid values for users. * render tests: switched to OutputFormat constants throughout. The format-boundary test (TestRendererFor_FormatBoundary) covers the named constants, the empty-string-as-text ergonomics, and unknown rejection via OutputFormat("xml"). Smoke: `crossplane resource validate ... --output=json` emits valid JSON; `--output=xml` is rejected by Kong with the wrapped Decode error "--output: unknown output format: \"xml\"". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
1 parent 050dafc commit 87ed65b

8 files changed

Lines changed: 296 additions & 283 deletions

File tree

‎cmd/crossplane/pkg/validate/render/render.go‎

Lines changed: 64 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,30 @@ import (
3131
const (
3232
errCannotMarshalJSON = "cannot marshal validation result as JSON"
3333
errCannotMarshalYAML = "cannot marshal validation result as YAML"
34+
errCannotWriteOutput = "cannot write validation result"
3435
errUnknownFormat = "unknown output format"
3536
)
3637

3738
// Renderer writes a *ValidationResult to an io.Writer in some specific
38-
// encoding. New formats are added by implementing Renderer and registering
39-
// a value under an OutputFormat in renderers below.
39+
// encoding. Implementations are obtained via RendererFor; the package
40+
// exposes no other constructors so the supported set is fully closed.
4041
type Renderer interface {
41-
Render(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error
42+
Render(result *pkgvalidate.ValidationResult, w io.Writer, opts Options) error
4243
}
4344

44-
// OutputFormat names a Renderer.
45+
// Options configures how a validation result is rendered.
46+
type Options struct {
47+
// SkipSuccessResults suppresses per-resource success lines in text output.
48+
// It has no effect on JSON or YAML output, where success entries are
49+
// always part of the structured payload.
50+
SkipSuccessResults bool
51+
}
52+
53+
// OutputFormat names a Renderer. It is a defined string type rather than
54+
// a bare string so that call sites can use the symbolic constants below
55+
// (OutputFormatText, OutputFormatJSON, OutputFormatYAML) instead of
56+
// embedding raw "text"/"json"/"yaml" literals, and so the compiler
57+
// catches accidental cross-wiring of unrelated string flags.
4558
type OutputFormat string
4659

4760
// OutputFormat values.
@@ -55,78 +68,48 @@ const (
5568
OutputFormatYAML OutputFormat = "yaml"
5669
)
5770

58-
// RenderOptions configures how a validation result is rendered.
59-
type RenderOptions struct {
60-
// SkipSuccessResults suppresses per-resource success lines in text output.
61-
// It has no effect on JSON or YAML output, where success entries are
62-
// always part of the structured payload.
63-
SkipSuccessResults bool
64-
}
65-
66-
// renderers is the polymorphic registry: each known OutputFormat names a
67-
// Renderer value. Adding a new format means adding a type that implements
68-
// Renderer and one entry below — nothing else in this package needs to
69-
// change.
70-
var renderers = map[OutputFormat]Renderer{
71-
OutputFormatText: textRenderer{},
72-
OutputFormatJSON: jsonRenderer{},
73-
OutputFormatYAML: yamlRenderer{},
74-
}
75-
76-
// Renderer returns the Renderer registered for f, or an error if f is not a
77-
// known format. The empty string is treated as text, so callers passing the
78-
// zero value (e.g. struct defaults) get sensible behaviour.
79-
func (f OutputFormat) Renderer() (Renderer, error) {
80-
if f == "" {
81-
f = OutputFormatText
82-
}
83-
r, ok := renderers[f]
84-
if !ok {
85-
return nil, errors.Errorf("%s: %q", errUnknownFormat, f)
86-
}
87-
return r, nil
88-
}
89-
90-
// Render dispatches to the Renderer registered for f.
91-
func (f OutputFormat) Render(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error {
92-
r, err := f.Renderer()
93-
if err != nil {
94-
return err
71+
// RendererFor returns the Renderer for the given format. The empty
72+
// string is accepted as OutputFormatText for ergonomics with
73+
// zero-valued config; any other unrecognised value returns an error.
74+
// This is the one and only boundary between a format identifier and
75+
// the typed Renderer dependency that downstream code receives.
76+
func RendererFor(format OutputFormat) (Renderer, error) {
77+
switch format {
78+
case OutputFormatText, "":
79+
return textRenderer{}, nil
80+
case OutputFormatJSON:
81+
return jsonRenderer{}, nil
82+
case OutputFormatYAML:
83+
return yamlRenderer{}, nil
84+
default:
85+
return nil, errors.Errorf("%s: %q", errUnknownFormat, format)
9586
}
96-
return r.Render(result, w, opts)
97-
}
98-
99-
// RenderValidationResult writes the validation result to w in the requested
100-
// format. It is a free-function shim around OutputFormat.Render kept for
101-
// callers that prefer the procedural style.
102-
func RenderValidationResult(result *pkgvalidate.ValidationResult, format OutputFormat, w io.Writer, opts RenderOptions) error {
103-
return format.Render(result, w, opts)
10487
}
10588

10689
// jsonRenderer emits indented JSON with a trailing newline.
10790
type jsonRenderer struct{}
10891

10992
// Render implements Renderer.
110-
func (jsonRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ RenderOptions) error {
93+
func (jsonRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ Options) error {
11194
out, err := json.MarshalIndent(result, "", " ")
11295
if err != nil {
11396
return errors.Wrap(err, errCannotMarshalJSON)
11497
}
11598
_, err = fmt.Fprintln(w, string(out))
116-
return err
99+
return errors.Wrap(err, errCannotWriteOutput)
117100
}
118101

119102
// yamlRenderer emits sigs.k8s.io/yaml output.
120103
type yamlRenderer struct{}
121104

122105
// Render implements Renderer.
123-
func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ RenderOptions) error {
106+
func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ Options) error {
124107
out, err := yaml.Marshal(result)
125108
if err != nil {
126109
return errors.Wrap(err, errCannotMarshalYAML)
127110
}
128111
_, err = fmt.Fprint(w, string(out))
129-
return err
112+
return errors.Wrap(err, errCannotWriteOutput)
130113
}
131114

132115
// textRenderer emits the human-readable text format that the validate CLI
@@ -143,49 +126,59 @@ func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _
143126
// A trailing summary line lists totals.
144127
type textRenderer struct{}
145128

146-
// Render implements Renderer.
147-
func (textRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error {
129+
// Render implements Renderer. The outer switch over ValidationStatus
130+
// dispatches per-resource emission; the per-error switch over
131+
// FieldErrorType lives in writeTextErrorLine (a different enumeration
132+
// covering a different concern, so it earns its own helper).
133+
func (textRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, opts Options) error {
148134
for _, r := range result.Resources {
149135
gvk := fmt.Sprintf("%s, Kind=%s", r.APIVersion, r.Kind)
136+
var line string
150137
switch r.Status {
151138
case pkgvalidate.ValidationStatusMissingSchema:
152-
if _, err := fmt.Fprintf(w, "[!] could not find CRD/XRD for: %s\n", gvk); err != nil {
153-
return err
154-
}
139+
line = fmt.Sprintf("[!] could not find CRD/XRD for: %s\n", gvk)
155140
case pkgvalidate.ValidationStatusValid:
156141
if opts.SkipSuccessResults {
157142
continue
158143
}
159-
if _, err := fmt.Fprintf(w, "[✓] %s, %s validated successfully\n", gvk, r.Name); err != nil {
160-
return err
161-
}
144+
line = fmt.Sprintf("[✓] %s, %s validated successfully\n", gvk, r.Name)
162145
case pkgvalidate.ValidationStatusInvalid, pkgvalidate.ValidationStatusDefaultingFailed:
146+
// Multi-line case: writeTextErrorLine writes once per error
147+
// and handles its own I/O, so we skip the single-line Fprint
148+
// below.
163149
for _, e := range r.Errors {
164150
if err := writeTextErrorLine(w, gvk, r.Name, e); err != nil {
165151
return err
166152
}
167153
}
154+
continue
155+
}
156+
if _, err := fmt.Fprint(w, line); err != nil {
157+
return err
168158
}
169159
}
170160
_, err := fmt.Fprintf(w, "Total %d resources: %d missing schemas, %d success cases, %d failure cases\n",
171161
result.Summary.Total, result.Summary.MissingSchemas, result.Summary.Valid, result.Summary.Invalid)
172162
return err
173163
}
174164

175-
// writeTextErrorLine emits a single per-error line. Defaulting failures use
176-
// the [!] warning prefix; schema, CEL, and unknown-field errors use [x].
177-
// Kept private to the textRenderer because the per-error format is a
178-
// detail of the text output, not part of the package's public surface.
165+
// writeTextErrorLine emits one line for a single FieldValidationError.
166+
// Defaulting failures use the [!] warning prefix; schema, CEL, and
167+
// unknown-field errors use [x]. Kept private to the textRenderer
168+
// because the per-error format is a detail of the text output.
179169
func writeTextErrorLine(w io.Writer, gvk, name string, e pkgvalidate.FieldValidationError) error {
170+
var line string
180171
switch e.Type {
181172
case pkgvalidate.FieldErrorTypeDefaulting:
182-
_, err := fmt.Fprintf(w, "[!] failed to apply defaults for %s, %s: %s\n", gvk, name, e.Message)
183-
return err
173+
line = fmt.Sprintf("[!] failed to apply defaults for %s, %s: %s\n", gvk, name, e.Message)
184174
case pkgvalidate.FieldErrorTypeCEL:
185-
_, err := fmt.Fprintf(w, "[x] CEL validation error %s, %s : %s\n", gvk, name, e.Message)
186-
return err
175+
line = fmt.Sprintf("[x] CEL validation error %s, %s : %s\n", gvk, name, e.Message)
176+
case pkgvalidate.FieldErrorTypeSchema, pkgvalidate.FieldErrorTypeUnknownField:
177+
line = fmt.Sprintf("[x] schema validation error %s, %s : %s\n", gvk, name, e.Message)
187178
default:
188-
_, err := fmt.Fprintf(w, "[x] schema validation error %s, %s : %s\n", gvk, name, e.Message)
189-
return err
179+
// Breadcrumb for an unhandled FieldErrorType added without updating this switch.
180+
line = fmt.Sprintf("[x] validation error %s, %s : %s\n", gvk, name, e.Message)
190181
}
182+
_, err := fmt.Fprint(w, line)
183+
return err
191184
}

‎cmd/crossplane/pkg/validate/render/render_test.go‎

Lines changed: 69 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,25 @@ func defaultingFixture() *pkgvalidate.ValidationResult {
9494
}
9595
}
9696

97-
// renderTextLines runs Render with the given format and options and returns
98-
// the non-empty lines of the resulting output. It centralises the call so
97+
// rendererForT resolves a Renderer through the public RendererFor API
98+
// and t.Fatals on error. Used by the rendering tests below; the parse
99+
// boundary itself has its own dedicated test.
100+
func rendererForT(t *testing.T, format OutputFormat) Renderer {
101+
t.Helper()
102+
r, err := RendererFor(format)
103+
if err != nil {
104+
t.Fatalf("RendererFor(%q): %v", format, err)
105+
}
106+
return r
107+
}
108+
109+
// renderTextLines runs the named renderer against in, returning the
110+
// non-empty lines of the resulting output. It centralises the call so
99111
// individual cases can focus on assertions.
100-
func renderTextLines(t *testing.T, in *pkgvalidate.ValidationResult, format OutputFormat, opts RenderOptions) []string {
112+
func renderTextLines(t *testing.T, in *pkgvalidate.ValidationResult, format OutputFormat, opts Options) []string {
101113
t.Helper()
102114
var buf bytes.Buffer
103-
if err := format.Render(in, &buf, opts); err != nil {
115+
if err := rendererForT(t, format).Render(in, &buf, opts); err != nil {
104116
t.Fatalf("Render() unexpected error: %v", err)
105117
}
106118
raw := strings.TrimRight(buf.String(), "\n")
@@ -110,18 +122,29 @@ func renderTextLines(t *testing.T, in *pkgvalidate.ValidationResult, format Outp
110122
return strings.Split(raw, "\n")
111123
}
112124

125+
// renderBytes runs the named renderer and returns its output as a byte
126+
// slice. Used by the structural JSON and YAML tests below.
127+
func renderBytes(t *testing.T, in *pkgvalidate.ValidationResult, format OutputFormat) []byte {
128+
t.Helper()
129+
var buf bytes.Buffer
130+
if err := rendererForT(t, format).Render(in, &buf, Options{}); err != nil {
131+
t.Fatalf("Render() unexpected error: %v", err)
132+
}
133+
return buf.Bytes()
134+
}
135+
113136
// summaryLine builds the trailing summary line for the given result.
114137
func summaryLine(r *pkgvalidate.ValidationResult) string {
115138
return fmt.Sprintf("Total %d resources: %d missing schemas, %d success cases, %d failure cases",
116139
r.Summary.Total, r.Summary.MissingSchemas, r.Summary.Valid, r.Summary.Invalid)
117140
}
118141

119-
func TestRenderValidationResult_Text(t *testing.T) {
142+
func TestRendererFor_Text(t *testing.T) {
120143
cases := map[string]struct {
121144
in *pkgvalidate.ValidationResult
122145
format OutputFormat
123-
opts RenderOptions
124-
wantLineSubs []string // every entry must appear as a substring of some output line, in order
146+
opts Options
147+
wantLineSubs []string // every entry must appear as a substring of the output line at the same index
125148
}{
126149
"WithSuccess": {
127150
in: fixture(),
@@ -136,18 +159,8 @@ func TestRenderValidationResult_Text(t *testing.T) {
136159
"SkipSuccess": {
137160
in: fixture(),
138161
format: OutputFormatText,
139-
opts: RenderOptions{SkipSuccessResults: true},
140-
wantLineSubs: []string{
141-
"[x] schema validation error test.org/v1alpha1, Kind=Test, bad",
142-
"[!] could not find CRD/XRD for: other.org/v1, Kind=Unknown",
143-
summaryLine(fixture()),
144-
},
145-
},
146-
"EmptyFormatActsAsText": {
147-
in: fixture(),
148-
format: OutputFormat(""),
162+
opts: Options{SkipSuccessResults: true},
149163
wantLineSubs: []string{
150-
"[✓] test.org/v1alpha1, Kind=Test, ok",
151164
"[x] schema validation error test.org/v1alpha1, Kind=Test, bad",
152165
"[!] could not find CRD/XRD for: other.org/v1, Kind=Unknown",
153166
summaryLine(fixture()),
@@ -179,43 +192,60 @@ func TestRenderValidationResult_Text(t *testing.T) {
179192
}
180193
}
181194

182-
func TestRenderValidationResult_JSON(t *testing.T) {
195+
func TestRendererFor_JSON(t *testing.T) {
183196
in := fixture()
184-
var buf bytes.Buffer
185-
if err := RenderValidationResult(in, OutputFormatJSON, &buf, RenderOptions{}); err != nil {
186-
t.Fatalf("RenderValidationResult(JSON) err = %v", err)
187-
}
197+
out := renderBytes(t, in, OutputFormatJSON)
188198
var got pkgvalidate.ValidationResult
189-
if err := json.Unmarshal(buf.Bytes(), &got); err != nil {
190-
t.Fatalf("json.Unmarshal() err = %v; output was:\n%s", err, buf.String())
199+
if err := json.Unmarshal(out, &got); err != nil {
200+
t.Fatalf("json.Unmarshal() err = %v; output was:\n%s", err, string(out))
191201
}
192202
if diff := cmp.Diff(*in, got); diff != "" {
193203
t.Errorf("JSON round-trip mismatch (-want +got):\n%s", diff)
194204
}
195205
}
196206

197-
func TestRenderValidationResult_YAML(t *testing.T) {
207+
func TestRendererFor_YAML(t *testing.T) {
198208
in := fixture()
199-
var buf bytes.Buffer
200-
if err := RenderValidationResult(in, OutputFormatYAML, &buf, RenderOptions{}); err != nil {
201-
t.Fatalf("RenderValidationResult(YAML) err = %v", err)
202-
}
209+
out := renderBytes(t, in, OutputFormatYAML)
203210
var got pkgvalidate.ValidationResult
204-
if err := yaml.Unmarshal(buf.Bytes(), &got); err != nil {
205-
t.Fatalf("yaml.Unmarshal() err = %v; output was:\n%s", err, buf.String())
211+
if err := yaml.Unmarshal(out, &got); err != nil {
212+
t.Fatalf("yaml.Unmarshal() err = %v; output was:\n%s", err, string(out))
206213
}
207214
if diff := cmp.Diff(*in, got); diff != "" {
208215
t.Errorf("YAML round-trip mismatch (-want +got):\n%s", diff)
209216
}
210217
}
211218

212-
func TestRenderValidationResult_Unknown(t *testing.T) {
213-
var buf bytes.Buffer
214-
err := RenderValidationResult(fixture(), OutputFormat("bogus"), &buf, RenderOptions{})
215-
if err == nil {
216-
t.Fatal("RenderValidationResult(bogus) = nil; want non-nil error")
219+
// TestRendererFor_FormatBoundary covers the only failable behaviour of
220+
// RendererFor: the OutputFormat-to-Renderer mapping. Empty maps to the
221+
// text renderer; an unrecognised value returns a non-nil error.
222+
func TestRendererFor_FormatBoundary(t *testing.T) {
223+
cases := map[string]struct {
224+
in OutputFormat
225+
wantType Renderer
226+
wantErr bool
227+
}{
228+
"Text": {in: OutputFormatText, wantType: textRenderer{}},
229+
"JSON": {in: OutputFormatJSON, wantType: jsonRenderer{}},
230+
"YAML": {in: OutputFormatYAML, wantType: yamlRenderer{}},
231+
"EmptyIsText": {in: "", wantType: textRenderer{}},
232+
"UnknownFails": {in: OutputFormat("xml"), wantErr: true},
217233
}
218-
if buf.Len() != 0 {
219-
t.Errorf("Unknown format wrote %d bytes; want 0 (content: %q)", buf.Len(), buf.String())
234+
for name, tc := range cases {
235+
t.Run(name, func(t *testing.T) {
236+
got, err := RendererFor(tc.in)
237+
if (err != nil) != tc.wantErr {
238+
t.Fatalf("RendererFor(%q) err = %v, wantErr = %v", tc.in, err, tc.wantErr)
239+
}
240+
if tc.wantErr {
241+
if got != nil {
242+
t.Errorf("RendererFor(%q) returned non-nil Renderer %v on error", tc.in, got)
243+
}
244+
return
245+
}
246+
if fmt.Sprintf("%T", got) != fmt.Sprintf("%T", tc.wantType) {
247+
t.Errorf("RendererFor(%q) = %T, want %T", tc.in, got, tc.wantType)
248+
}
249+
})
220250
}
221251
}

0 commit comments

Comments
 (0)