Declared validation rules that no test exercises

[retr0h]

Most domains declare validation the tests never exercise. Removing one of those tags from a spec would change nothing a test could see.

Corrected method

This issue originally counted test methods named ValidationHTTP and concluded log had none. That was wrong: log tested valid_target and the priority oneof inside a method named TestGetNodeLogHTTP. Counting names measures naming, not coverage.

The right comparison is the validate: tags each spec declares against the rule names the tests assert:

python3 - <<'PY'
import pathlib, re
for d in sorted(pathlib.Path('internal/controller/api/node').iterdir()):
    spec = d/'gen'/'api.yaml'
    if not spec.is_file(): continue
    rules = {p.strip().split('=')[0]
             for m in re.findall(r'validate:\s*"?([^"\n]+)"?', spec.read_text())
             for p in m.split(',')
             if p.strip() and p.strip().split('=')[0] not in ('omitempty','required')}
    asserted = {m for t in d.glob('*_test.go')
                  for m in re.findall(r'Body\.String\(\), "([a-z_]+)"', t.read_text())}
    print(f"{d.name:14} {len(rules):5} {len(rules & asserted):6}  {' '.join(sorted(rules - asserted)) or '-'}")
PY

What it finds

domain         rules tested  untested
hostname           3      0  max min valid_target
schedule           7      1  cron_schedule excluded_with max min oneof required_without
file               5      1  file_mode max min oneof
user               4      1  account_name dive min
container          6      3  dive min oneof
network           12      6  dive ipv4 ipv6 max min required_without
process            3      1  min oneof
certificate        2      1  min
command            3      2  min
ntp                2      1  min
package            2      1  min
power              2      1  min
service            2      1  min
sysctl             4      3  min
timezone           2      1  min
log                4      4  -

log is covered as of #578. Everything else has a gap.

Priority

hostname is the worst at 0 of 3, and it is the one the original method-name count missed completely, because it has a method named ValidationHTTP that asserts no rule by name.

After that, by how much a missed rule would cost:

1. valid_target, oneof, ipv4, ipv6, file_mode, cron_schedule, account_name reject input that would otherwise reach a provider and fail somewhere less legible.
2. excluded_with and required_without encode a relationship between two fields, which is the kind of rule that quietly stops working when a field is renamed.
3. min and max are the most commonly untested and the least costly. min=1 beside required is close to redundant; max on a bounded query parameter is not.

The obligation

osapi-io/specs#244 states it: the test asserts 400 and that the body names the rule that rejected it. A test checking only the status code passes when the handler rejects for an unrelated reason, which is the failure it exists to catch.

name: "when lines below the minimum returns 400",
path: "/api/node/server1/log?lines=0",
    s.Equal(http.StatusBadRequest, rec.Code)
    s.Contains(rec.Body.String(), "min")

Asserting the rule name is also what makes the audit above possible, since it is what the script greps for.

[retr0h]

Progress. Two domains done, five to go.

domain         rules tested  untested
hostname           3      2  min          <- #579
schedule           7      6  min          <- #581
log                4      4  -
network           12      6  dive ipv4 ipv6 max min required_without
file               5      1  file_mode max min oneof
user               4      1  account_name dive min
container          6      3  dive min oneof
process            3      1  min oneof
certificate        2      1  min
command            3      2  min
ntp                2      1  min
package            2      1  min
power              2      1  min
service            2      1  min
sysctl             4      3  min
timezone           2      1  min

min is unreachable everywhere and should come out of the denominator

It appears in the untested column for every domain and no test can ever assert it. On a string, required fails first on an empty value, so min=1 is never the reported tag:

empty body: ok=false msg="Key: 'probeBody.Hostname' Error:Field validation for 'Hostname' failed on the 'required' tag"

Where the tag is omitempty,min=1, omitempty skips the empty case instead. And on a path parameter an empty segment does not match the route at all.

required,min=1 is in 60-plus declarations, so this is its own pass rather than something to fix one domain at a time. Subtract one from most of the untested counts above: certificate, command, ntp, package, power, service, sysctl and timezone are already complete, and hostname and schedule are done.

What is actually left

• network — ipv4, ipv6, required_without, max, dive. Biggest remaining gap, and the IP checks are the ones most worth proving.
• file — file_mode, oneof, max. file_mode matters: an unparsable mode used to become 0644 further down.
• user — account_name, dive.
• container — oneof, dive.
• process — oneof.

Two things found on the way

#580, fixed in #582 — three path parameters declared a pattern that nothing at the API layer enforced.

In schedule's update test, a case named "when invalid cron schedule" asserted the body contained Schedule, which is the field name. A 400 from required_without on that same field contains Schedule too, so the case passed whether or not cron_schedule was the rule that rejected it. Worth watching for while working the rest: a case asserting a field name proves nothing about which rule fired.

[retr0h]

Done. Every declared validation rule that a test can assert is now asserted by name, across all 20 domains.

PR	What
#579	hostname, 0 of 3
#581	schedule, 1 of 7
#583	network, 6 of 12
#584	file, user, container, process
#586	job, audit, agent and controller-level file, which this issue never measured

Two code defects came out of it, both fixed:

• The schedule name path parameter declares rules nothing enforces #580, in fix(api): enforce the name patterns the specs already declare #582 — schedule, certificate and container declared a pattern on a path parameter that the API layer did not enforce, so a request the controller already knew was invalid became a job and crossed NATS before anything rejected it.
• The agent accept fingerprint query parameter declares rules nothing enforces #585, in fix(agent): validate the accept endpoint's query parameters #588 — the agent accept endpoint never validated its query parameters, so fingerprint's declared bound was decoration.

Five things the script in this issue mismeasures

Keeping these here because the next person to run it will otherwise re-derive them.

1. min=1 after required can never fire. required fails first on the zero value. 58 declarations, now tracked in 58 declarations say required,min=1, where min=1 can never fire #587.
2. dive is a directive, not a rule. A failing element reports the inner tag against an indexed field: Addresses[0] ... failed on the 'cidr' tag.
3. uuid never runs. A non-UUID path value fails in the generated binding, which returns Invalid format for parameter id.
4. The regex [a-z_]+ cannot see a digit, so ipv4 and ipv6 read as untested however many cases they have.
5. It only walked internal/controller/api/node, missing job, audit, agent and controller file entirely. job had 5 rules and 0 asserted.

Corrected, the script's exclusion set is omitempty, required, dive, uuid, its regex is [a-z0-9_]+, and it walks both directories.

The habit worth keeping

Nearly every gap was a case that asserted a 400 and either "error" or the field name. Both are true of rejections the rule had nothing to do with. The clearest instance: schedule's update test asserted the body contained Schedule, which a 400 from required_without on that same field also contains. The one that hid a real bug: file's upload handler used one message for both required and max, so the empty-name case and the 256-character case asserted the same string and neither proved which rule fired.

A case that names the rule fails when the rule is deleted. A case that names the field does not.