fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

#113: derive the config guard from the record tree, both directions
CI / contract (push) Successful in 53s
Details
CI / build (push) Successful in 1m48s
Details

Browse Source 
The existing guard walks KNOWN_TOP_LEVEL_KEYS and anchors its regex at
column 0, so it sees only top-level keys. Every nested key was outside
its scope and nothing said so, which is the shape #113 collects: a
checker narrower than it looks, whose green run stops anyone looking.

Two derived guards replace the assumption:

  everyNestedConfigKeyIsDocumentedInTheExample
      walks FleetConfig's record components (17 records, 83 distinct
      key names) and requires each to be documented in the example.

  everyLiveKeyInTheExampleBindsToARecordComponent
      resolves every live key path in the example against the record
      tree, so a documented key that binds to nothing fails here
      instead of being silently ignored in production.

Neither carries a list, so a key added to any nested record is covered
the moment it compiles (criterion 2).

Both mutations run through the real caller, not the helper (criterion 1,
which asks for exactly that):

  removed every mention of paneProbeIntervalSeconds from the example
      -> FAILS, naming health.paneProbeIntervalSeconds
  added a live bind.totallyMadeUpKnob to the example
      -> FAILS, naming bind.totallyMadeUpKnob

0 compile errors in both; both reverted and confirmed with diff -q.
The first attempt at mutation 1 removed only the `key:` line and the
run stayed green — correctly, because the key was still documented in
prose. An incomplete mutation proves nothing, so it was redone.

Denominators (criterion 3): both guards print how many keys they
checked, and the floor for "did the walk descend?" is derived from
KNOWN_TOP_LEVEL_KEYS.size() rather than being a literal.

Scope is stated in the javadoc rather than implied: the guards do not
check a key sits at the right path, do not parse commented prose for
the reverse direction, and do not prove a parsed key is read by
anything. paneProbeIntervalSeconds is parsed and read by nothing, and
these guards pass it -- the example already says so in its own text.

everyOptionalKnobDocumentedInTheExampleBinds keeps its hand-written
list but is re-documented as a value-binding spot check, explicitly
not a coverage guard; coverage now comes from the two derived tests.

broker.uri is documented only in the example's prose convention
(`#  uri  -> ...`), never as a copy-pasteable `uri:` key, because
writing it out invites pasting a password into a file -- the thing
uriEnv exists to avoid. The matcher accepts that convention rather
than pushing the file toward doing it.

Full build: 1236 tests, 0 failures, 0 compile errors.
This commit is contained in:
Dai Ha
2026-09-03 13:18:50 +07:00
parent 01a840cc14
commit 21c539f22e
1 changed files with 243 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java
+243 -3
View File
@@ -6,11 +6,18 @@ import dev.ltms.fleet.peer.MemberRole;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.RecordComponent;
import java.lang.reflect.Type;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import static org.junit.jupiter.api.Assertions.*;
@@ -1366,9 +1373,18 @@ class FleetConfigTest {
}
/**
* Every optional knob the example documents must bind under the exact spelling used there.
* Keep this list in step with {@code fleetd.example.yaml}: a rename that updates the record
* but not the example (or vice versa) fails here instead of silently no-op'ing in production.
* A spot check that the knobs listed below bind under the exact spelling the example uses —
* it asserts real VALUES arrive in the record, which no name-matching guard can do.
*
* <p><b>This is NOT a coverage guard, and must not be read as one</b> (fleetd #113). The list
* inside it is hand-written, so it only ever covers what someone remembered to add. Coverage
* — "is every key the code reads documented, and does every documented key bind?" — comes
* from {@link #everyNestedConfigKeyIsDocumentedInTheExample} and
* {@link #everyLiveKeyInTheExampleBindsToARecordComponent}, both of which derive their key
* set from the record tree and therefore cannot drift.
*
* <p>Adding a knob here is optional. Leaving one out is not a coverage gap, because the two
* derived guards above already fail on an undocumented or unbindable key.
*/
@Test
void everyOptionalKnobDocumentedInTheExampleBinds(@TempDir Path dir) throws Exception {
@@ -1525,6 +1541,230 @@ class FleetConfigTest {
return p.matcher(yaml).find();
}
/**
* The nested half of {@link #everyKnownTopLevelKeyIsDocumentedInTheExample} (fleetd #113).
*
* <p>That guard walks {@link FleetConfig#KNOWN_TOP_LEVEL_KEYS} and anchors its regex at
* column 0, so it sees ONLY top-level keys. Every nested key — {@code profiles.<name>.model},
* {@code health.paneProbeIntervalSeconds} and a hundred others — is outside its scope, and
* neither its name nor its output says so. A green run then reads as "the example documents
* the schema" when most of the schema was never looked at.
*
* <p>This walks the record tree rather than a name list, so a key added to any nested record
* is covered the moment it compiles, with no edit here. That is the point: a hand-maintained
* second copy of a list always drifts from the thing it mirrors.
*
* <p><b>Scope, stated on purpose</b> (fleetd #113 criterion 3 — every check reports what it
* did and did not look at):
* <ul>
* <li>It checks each key NAME appears somewhere in the example as a YAML key, live or
* commented out. It does NOT check the key sits at the right path.</li>
* <li>It does NOT check a documented key is read by anything. {@code paneProbeIntervalSeconds}
* is parsed into {@link FleetConfig.Health} and used nowhere, and this guard passes it.
* Proving a key is live code needs a call graph, which this is not.</li>
* </ul>
*/
@Test
void everyNestedConfigKeyIsDocumentedInTheExample() throws Exception {
Path example = Path.of("fleetd.example.yaml");
assertTrue(Files.exists(example), "fleetd.example.yaml must ship next to the pom");
String text = Files.readString(example);
Map<String, String> pathByName = configKeyPaths();
// The denominator. An under-counting walk passes every subset check vacuously, which is
// the exact shape fleetd #113 collects — so the walk has to prove it descended at all.
// The floor is DERIVED, not a literal: the nested walk must find substantially more keys
// than the top-level set the old guard used, or it has not gone below the first level.
int topLevel = FleetConfig.KNOWN_TOP_LEVEL_KEYS.size();
assertTrue(pathByName.size() > topLevel * 2,
"the record walk found " + pathByName.size() + " config key(s) against "
+ topLevel + " top-level key(s) — it has stopped descending into the "
+ "nested records, so this guard would pass vacuously. Fix the walk "
+ "before trusting a green run.");
List<String> undocumented = pathByName.entrySet().stream()
.filter(e -> !keyDocumentedAnywhere(text, e.getKey()))
.map(Map.Entry::getValue)
.sorted()
.toList();
assertTrue(undocumented.isEmpty(), () -> "checked " + pathByName.size()
+ " config key(s) that FleetConfig can bind; " + undocumented.size()
+ " appear nowhere in fleetd.example.yaml: " + undocumented
+ " — document each one there, commented out if optional. fleetd.yaml is "
+ "gitignored, so the example is the only committed description of the schema.");
}
/**
* The other direction: a LIVE key in the example that {@link FleetConfig} cannot bind. That is
* a key an operator would copy into {@code fleetd.yaml} expecting it to do something, where it
* would be silently ignored.
*
* <p><b>Scope, stated on purpose:</b> only live (uncommented) keys are checked. Most of the
* example is commented-out prose, and that prose contains lines like {@code # mode: token}
* that are indistinguishable from keys by text alone. Parsing them would produce false
* failures, so they are deliberately out of scope — and saying so here is the point, rather
* than letting a reader assume the whole file was validated.
*/
@Test
void everyLiveKeyInTheExampleBindsToARecordComponent() throws Exception {
Path example = Path.of("fleetd.example.yaml");
String text = Files.readString(example);
List<List<String>> paths = liveKeyPaths(text);
assertTrue(paths.size() >= 20,
"only " + paths.size() + " live key path(s) were parsed out of the example — the "
+ "parser is not seeing the file, so this guard would pass vacuously.");
List<String> unbindable = paths.stream()
.filter(path -> !pathBinds(path))
.map(path -> String.join(".", path))
.distinct()
.sorted()
.toList();
assertTrue(unbindable.isEmpty(), () -> "checked " + paths.size()
+ " live key path(s) in fleetd.example.yaml; " + unbindable.size()
+ " bind to nothing in FleetConfig: " + unbindable
+ " — an operator copying one of these into fleetd.yaml gets silence, not an error.");
}
/**
* Every configuration key {@link FleetConfig} can bind, at every depth, as
* {@code name -> a dotted path to one place it appears}. Derived from the record components,
* so it cannot drift from the code.
*/
private static Map<String, String> configKeyPaths() {
Map<String, String> out = new TreeMap<>();
collectConfigKeys(FleetConfig.class, "", new HashSet<>(), out);
return out;
}
private static void collectConfigKeys(Class<?> type, String prefix, Set<String> seen,
Map<String, String> out) {
if (!type.isRecord() || !seen.add(type.getName())) {
return;
}
for (RecordComponent rc : type.getRecordComponents()) {
String path = prefix.isEmpty() ? rc.getName() : prefix + "." + rc.getName();
out.putIfAbsent(rc.getName(), path);
Class<?> nested = rc.getType();
if (nested.isRecord()) {
collectConfigKeys(nested, path, seen, out);
} else if (Map.class.isAssignableFrom(nested) || List.class.isAssignableFrom(nested)) {
Class<?> element = elementRecord(rc);
if (element != null) {
String childPrefix = Map.class.isAssignableFrom(nested)
? path + ".<name>" : path + "[]";
collectConfigKeys(element, childPrefix, seen, out);
}
}
}
}
/** The record type inside a {@code Map<String, X>} or {@code List<X>} component, else null. */
private static Class<?> elementRecord(RecordComponent rc) {
if (rc.getGenericType() instanceof ParameterizedType pt) {
Type[] args = pt.getActualTypeArguments();
if (args.length > 0 && args[args.length - 1] instanceof Class<?> c && c.isRecord()) {
return c;
}
}
return null;
}
/**
* True when {@code key} is documented in the example, in either of the two conventions that
* file actually uses:
* <ol>
* <li>as a YAML key at any indentation, live or commented out ({@code key:}); or</li>
* <li>in a prose block that describes a section's sub-keys, one per line, as
* {@code # key → what it does}.</li>
* </ol>
*
* <p>The second form is not decoration. {@code broker.uri} is documented ONLY that way, on
* purpose: writing it out as a copy-pasteable {@code uri: amqp://user:pass@host} invites an
* operator to paste a password into a file, which is the very thing {@code uriEnv} exists to
* avoid. A guard that demanded the key form would push the file toward doing that. So this
* encodes the convention the example really uses rather than imposing a new one.
*/
private static boolean keyDocumentedAnywhere(String yaml, String key) {
String quoted = Pattern.quote(key);
Pattern asYamlKey = Pattern.compile("(?m)^\\s*(?:#\\s*)?" + quoted + ":");
Pattern asProseEntry = Pattern.compile("(?m)^\\s*#\\s*" + quoted + "\\s+\u2192");
return asYamlKey.matcher(yaml).find() || asProseEntry.matcher(yaml).find();
}
/** Every live (uncommented) key in {@code yaml}, as a path from the document root. */
private static List<List<String>> liveKeyPaths(String yaml) {
Pattern keyLine = Pattern.compile("^(\\s*)([A-Za-z][A-Za-z0-9_]*):(\\s.*)?$");
List<String> stack = new ArrayList<>();
List<Integer> indents = new ArrayList<>();
List<List<String>> paths = new ArrayList<>();
for (String line : yaml.split("\n", -1)) {
if (line.isBlank() || line.stripLeading().startsWith("#")) {
continue;
}
Matcher m = keyLine.matcher(line);
if (!m.matches()) {
continue;
}
int indent = m.group(1).length();
while (!indents.isEmpty() && indents.get(indents.size() - 1) >= indent) {
indents.remove(indents.size() - 1);
stack.remove(stack.size() - 1);
}
indents.add(indent);
stack.add(m.group(2));
paths.add(List.copyOf(stack));
}
return paths;
}
/** True when a dotted YAML path resolves to something {@link FleetConfig} can bind. */
private static boolean pathBinds(List<String> path) {
Class<?> type = FleetConfig.class;
boolean nextSegmentIsAFreeFormName = false;
for (int i = 0; i < path.size(); i++) {
if (nextSegmentIsAFreeFormName) {
nextSegmentIsAFreeFormName = false;
continue;
}
RecordComponent rc = componentNamed(type, path.get(i));
if (rc == null) {
return false;
}
Class<?> t = rc.getType();
if (t.isRecord()) {
type = t;
} else if (Map.class.isAssignableFrom(t)) {
Class<?> element = elementRecord(rc);
if (element == null) {
return true; // Map<String,String>: its entries are data, not schema
}
type = element;
nextSegmentIsAFreeFormName = true;
} else {
// A scalar or a list of scalars: nothing may legitimately nest under it.
return i == path.size() - 1;
}
}
return true;
}
private static RecordComponent componentNamed(Class<?> type, String name) {
if (type == null || !type.isRecord()) {
return null;
}
for (RecordComponent rc : type.getRecordComponents()) {
if (rc.getName().equals(name)) {
return rc;
}
}
return null;
}
@Test
void placementDefaultsToFixedForExistingConfigs(@TempDir Path dir) throws Exception {
Path f = dir.resolve("no-placement.yaml");