Validate bootstrap strategy at the API edge, not deep in planning · Entire
Validate bootstrap strategy at the API edge, not deep in planning
524a25c→main· Soph·2mo ago·2 files·+93 added/-0 removed
The previous arrangement only rejected unknown values inside bootstrap planning, which meant typos sat unnoticed on non-bootstrap code paths (Probe, Fetch) and surfaced confusingly late on bootstrap paths after CLI parse and config plumbing had already accepted them. Reject at AdvancedOptions.Validate(), called as the first thing in every Client method that accepts options. Define exported BootstrapStrategyFirstParent / BootstrapStrategyTopo constants so callers don't have to repeat string literals.
Sessions
a9979c82ce75View transcript
Changes
2
unstable
Mclient.go+40
Mclient_test.go+53
44 unmodified lines
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
35 unmodified lines
108
109
110
111
112
113
114
115
116
6 unmodified lines
123
124
125
126
127
128
129
130
131
8 unmodified lines
140
141
142
143
144
145
146
147
148
6 unmodified lines
155
156
157
158
159
160
161
162
163
7 unmodified lines
171
172
173
174
175
176
177
178
179
6 unmodified lines
186
187
188
189
190
191
192
193
194
44 unmodified lines
BootstrapStrategy string `json:"bootstrapStrategy,omitempty"`
}
// BootstrapStrategy values accepted by AdvancedOptions.BootstrapStrategy.
// Empty is treated as the default (first-parent).
const (
BootstrapStrategyFirstParent = "first-parent"
BootstrapStrategyTopo = "topo"
)
// Validate rejects unknown values up front so callers don't have to
// wait for the deep bootstrap planning path to surface them. It runs
// on every request method regardless of whether the value will end
// up being consulted, since silently accepting a typo on a
// non-bootstrap path is worse than failing fast.
func (o AdvancedOptions) Validate() error {
switch o.BootstrapStrategy {
case "", BootstrapStrategyFirstParent, BootstrapStrategyTopo:
return nil
default:
return fmt.Errorf("unsupported bootstrap strategy %q (want %q or %q)",
o.BootstrapStrategy, BootstrapStrategyFirstParent, BootstrapStrategyTopo)
}
}
type ProbeRequest struct {
Source gitsync.Endpoint
Target *gitsync.Endpoint
35 unmodified lines
}
func (c *Client) Probe(ctx context.Context, req ProbeRequest) (ProbeResult, error) {
if err := req.Options.Validate(); err != nil {
return ProbeResult{}, fmt.Errorf("probe: %w", err)
}
cfg, err := c.buildProbeConfig(ctx, req)
if err != nil {
return ProbeResult{}, err
}
6 unmodified lines
}
func (c *Client) Plan(ctx context.Context, req SyncRequest) (Result, error) {
if err := req.Options.Validate(); err != nil {
return Result{}, fmt.Errorf("plan: %w", err)
}
planReq := req
planReq.DryRun = true
cfg, err := c.buildSyncConfig(ctx, planReq)
8 unmodified lines
}
func (c *Client) Sync(ctx context.Context, req SyncRequest) (Result, error) {
if err := req.Options.Validate(); err != nil {
return Result{}, fmt.Errorf("sync: %w", err)
}
cfg, err := c.buildSyncConfig(ctx, req)
if err != nil {
return Result{}, err
}
6 unmodified lines
}
func (c *Client) Replicate(ctx context.Context, req SyncRequest) (Result, error) {
if err := req.Options.Validate(); err != nil {
return Result{}, fmt.Errorf("replicate: %w", err)
}
req.Policy.Mode = gitsync.ModeReplicate
cfg, err := c.buildSyncConfig(ctx, req)
if err != nil {
7 unmodified lines
}
func (c *Client) Bootstrap(ctx context.Context, req BootstrapRequest) (Result, error) {
if err := req.Options.Validate(); err != nil {
return Result{}, fmt.Errorf("bootstrap: %w", err)
}
cfg, err := c.buildBootstrapConfig(ctx, req)
if err != nil {
return Result{}, err
6 unmodified lines
}
func (c *Client) Fetch(ctx context.Context, req FetchRequest) (FetchResult, error) {
if err := req.Options.Validate(); err != nil {
return FetchResult{}, fmt.Errorf("fetch: %w", err)
}
cfg, err := c.buildFetchConfig(ctx, req)
if err != nil {
return FetchResult{}, err
Munstable/client.go+40
46 unmodified lines
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
46 unmodified lines
}
}
func TestAdvancedOptionsValidate(t *testing.T) {
t.Parallel()
cases := []struct {
name string
opts AdvancedOptions
wantErr bool
}{
{name: "empty strategy is the default", opts: AdvancedOptions{}, wantErr: false},
{name: "first-parent accepted", opts: AdvancedOptions{BootstrapStrategy: BootstrapStrategyFirstParent}, wantErr: false},
{name: "topo accepted", opts: AdvancedOptions{BootstrapStrategy: BootstrapStrategyTopo}, wantErr: false},
{name: "typo rejected at API edge", opts: AdvancedOptions{BootstrapStrategy: "topographic"}, wantErr: true},
{name: "case-sensitive: TOPO is not topo", opts: AdvancedOptions{BootstrapStrategy: "TOPO"}, wantErr: true},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
t.Parallel()
err := c.opts.Validate()
if (err != nil) != c.wantErr {
t.Errorf("Validate() err=%v, wantErr=%v", err, c.wantErr)
}
})
}
}
func TestClientRejectsUnknownStrategyBeforeIO(t *testing.T) {
t.Parallel()
// The reviewer's concern: an invalid value should fail at the
// API edge, not silently slip through on a non-bootstrap path
// (e.g. Probe) where bootstrap planning never runs.
c := New(Options{HTTPClient: &http.Client{}})
bad := AdvancedOptions{BootstrapStrategy: "unsupported"}
if _, err := c.Probe(context.Background(), ProbeRequest{
Source: gitsync.Endpoint{URL: "https://source.example/repo.git"},
Options: bad,
}); err == nil {
t.Errorf("Probe with invalid bootstrap strategy should error")
}
if _, err := c.Sync(context.Background(), SyncRequest{
Source: gitsync.Endpoint{URL: "https://source.example/repo.git"},
Target: gitsync.Endpoint{URL: "https://target.example/repo.git"},
Options: bad,
}); err == nil {
t.Errorf("Sync with invalid bootstrap strategy should error")
}
if _, err := c.Bootstrap(context.Background(), BootstrapRequest{
Source: gitsync.Endpoint{URL: "https://source.example/repo.git"},
Target: gitsync.Endpoint{URL: "https://target.example/repo.git"},
Options: bad,
}); err == nil {
t.Errorf("Bootstrap with invalid bootstrap strategy should error")
}
}
func TestBuildFetchConfigCopiesHaveHashesAtCallSite(t *testing.T) {
req := FetchRequest{
Source: gitsync.Endpoint{URL: "https://source.example/repo.git"},