Skip to content

Commit cbfeb73

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 cbfeb73

8 files changed

Lines changed: 294 additions & 288 deletions

File tree

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

Lines changed: 62 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"encoding/json"
2121
"fmt"
2222
"io"
23+
"strings"
2324

2425
"sigs.k8s.io/yaml"
2526

@@ -31,17 +32,30 @@ import (
3132
const (
3233
errCannotMarshalJSON = "cannot marshal validation result as JSON"
3334
errCannotMarshalYAML = "cannot marshal validation result as YAML"
35+
errCannotWriteOutput = "cannot write validation result"
3436
errUnknownFormat = "unknown output format"
3537
)
3638

3739
// 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.
40+
// encoding. Implementations are obtained via RendererFor; the package
41+
// exposes no other constructors so the supported set is fully closed.
4042
type Renderer interface {
41-
Render(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error
43+
Render(result *pkgvalidate.ValidationResult, w io.Writer, opts Options) error
4244
}
4345

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

4761
// OutputFormat values.
@@ -55,78 +69,48 @@ const (
5569
OutputFormatYAML OutputFormat = "yaml"
5670
)
5771

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
72+
// RendererFor returns the Renderer for the given format. The empty
73+
// string is accepted as OutputFormatText for ergonomics with
74+
// zero-valued config; any other unrecognised value returns an error.
75+
// This is the one and only boundary between a format identifier and
76+
// the typed Renderer dependency that downstream code receives.
77+
func RendererFor(format OutputFormat) (Renderer, error) {
78+
switch format {
79+
case OutputFormatText, "":
80+
return textRenderer{}, nil
81+
case OutputFormatJSON:
82+
return jsonRenderer{}, nil
83+
case OutputFormatYAML:
84+
return yamlRenderer{}, nil
85+
default:
86+
return nil, errors.Errorf("%s: %q", errUnknownFormat, format)
9587
}
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)
10488
}
10589

10690
// jsonRenderer emits indented JSON with a trailing newline.
10791
type jsonRenderer struct{}
10892

10993
// Render implements Renderer.
110-
func (jsonRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ RenderOptions) error {
94+
func (jsonRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ Options) error {
11195
out, err := json.MarshalIndent(result, "", " ")
11296
if err != nil {
11397
return errors.Wrap(err, errCannotMarshalJSON)
11498
}
11599
_, err = fmt.Fprintln(w, string(out))
116-
return err
100+
return errors.Wrap(err, errCannotWriteOutput)
117101
}
118102

119103
// yamlRenderer emits sigs.k8s.io/yaml output.
120104
type yamlRenderer struct{}
121105

122106
// Render implements Renderer.
123-
func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ RenderOptions) error {
107+
func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _ Options) error {
124108
out, err := yaml.Marshal(result)
125109
if err != nil {
126110
return errors.Wrap(err, errCannotMarshalYAML)
127111
}
128112
_, err = fmt.Fprint(w, string(out))
129-
return err
113+
return errors.Wrap(err, errCannotWriteOutput)
130114
}
131115

132116
// textRenderer emits the human-readable text format that the validate CLI
@@ -143,49 +127,51 @@ func (yamlRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, _
143127
// A trailing summary line lists totals.
144128
type textRenderer struct{}
145129

146-
// Render implements Renderer.
147-
func (textRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error {
130+
// Render implements Renderer. The outer switch over ValidationStatus
131+
// dispatches per-resource emission; the per-error switch over
132+
// FieldErrorType lives in textErrorLine (a different enumeration
133+
// covering a different concern, so it earns its own helper).
134+
func (textRenderer) Render(result *pkgvalidate.ValidationResult, w io.Writer, opts Options) error {
148135
for _, r := range result.Resources {
149136
gvk := fmt.Sprintf("%s, Kind=%s", r.APIVersion, r.Kind)
137+
var line string
150138
switch r.Status {
151139
case pkgvalidate.ValidationStatusMissingSchema:
152-
if _, err := fmt.Fprintf(w, "[!] could not find CRD/XRD for: %s\n", gvk); err != nil {
153-
return err
154-
}
140+
line = fmt.Sprintf("[!] could not find CRD/XRD for: %s\n", gvk)
155141
case pkgvalidate.ValidationStatusValid:
156142
if opts.SkipSuccessResults {
157143
continue
158144
}
159-
if _, err := fmt.Fprintf(w, "[✓] %s, %s validated successfully\n", gvk, r.Name); err != nil {
160-
return err
161-
}
145+
line = fmt.Sprintf("[✓] %s, %s validated successfully\n", gvk, r.Name)
162146
case pkgvalidate.ValidationStatusInvalid, pkgvalidate.ValidationStatusDefaultingFailed:
147+
var sb strings.Builder
163148
for _, e := range r.Errors {
164-
if err := writeTextErrorLine(w, gvk, r.Name, e); err != nil {
165-
return err
166-
}
149+
sb.WriteString(textErrorLine(gvk, r.Name, e))
167150
}
151+
line = sb.String()
152+
}
153+
if _, err := fmt.Fprint(w, line); err != nil {
154+
return errors.Wrap(err, errCannotWriteOutput)
168155
}
169156
}
170157
_, err := fmt.Fprintf(w, "Total %d resources: %d missing schemas, %d success cases, %d failure cases\n",
171158
result.Summary.Total, result.Summary.MissingSchemas, result.Summary.Valid, result.Summary.Invalid)
172-
return err
159+
return errors.Wrap(err, errCannotWriteOutput)
173160
}
174161

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.
179-
func writeTextErrorLine(w io.Writer, gvk, name string, e pkgvalidate.FieldValidationError) error {
162+
// textErrorLine returns the rendered text for a single
163+
// FieldValidationError. Defaulting failures use the [!] warning prefix;
164+
// schema, CEL, and unknown-field errors use [x].
165+
func textErrorLine(gvk, name string, e pkgvalidate.FieldValidationError) string {
180166
switch e.Type {
181167
case pkgvalidate.FieldErrorTypeDefaulting:
182-
_, err := fmt.Fprintf(w, "[!] failed to apply defaults for %s, %s: %s\n", gvk, name, e.Message)
183-
return err
168+
return fmt.Sprintf("[!] failed to apply defaults for %s, %s: %s\n", gvk, name, e.Message)
184169
case pkgvalidate.FieldErrorTypeCEL:
185-
_, err := fmt.Fprintf(w, "[x] CEL validation error %s, %s : %s\n", gvk, name, e.Message)
186-
return err
170+
return fmt.Sprintf("[x] CEL validation error %s, %s : %s\n", gvk, name, e.Message)
171+
case pkgvalidate.FieldErrorTypeSchema, pkgvalidate.FieldErrorTypeUnknownField:
172+
return fmt.Sprintf("[x] schema validation error %s, %s : %s\n", gvk, name, e.Message)
187173
default:
188-
_, err := fmt.Fprintf(w, "[x] schema validation error %s, %s : %s\n", gvk, name, e.Message)
189-
return err
174+
// Breadcrumb for an unhandled FieldErrorType added without updating this switch.
175+
return fmt.Sprintf("[x] validation error %s, %s : %s\n", gvk, name, e.Message)
190176
}
191177
}

‎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)