feat(ci): enforce lowerCamelCase and max depth in reference.conf#6795
feat(ci): enforce lowerCamelCase and max depth in reference.conf#6795bladehan1 wants to merge 3 commits into
Conversation
Add a CI gate that scans common/src/main/resources/reference.conf and fails the build when any key violates lowerCamelCase (^[a-z][a-zA-Z0-9]*$ per dot-separated segment) or exceeds the maximum hierarchy depth (5). Array element keys are validated the same way; each array step counts as one depth level — e.g. an inner field at `rate.limiter.rpc[].component` is depth 5. Parsing is delegated to pyhocon, the reference Python HOCON implementation. It returns a fully-merged ConfigTree where dotted-form keys expand into nested objects — the same canonical key set Typesafe Config and ConfigBeanFactory see at runtime — and handles triple-strings, substitutions, includes, +=, and block comments without us re-implementing the grammar. Four legacy PBFT* keys are grandfathered via an in-script allowlist so the gate fails only on new violations. A consolidated GHA error annotation lists every offending key, and sys.exit(1) drives step failure. The script also accepts `--debug` to print every parsed key with its depth (trailing `/` marks namespace intermediates) for manual verification against the source file. Runs as a new step in the existing checkstyle job of pr-check.yml (setup-python + `pip install pyhocon`), so no extra runner spin-up.
Pin pyhocon via .github/scripts/requirements.txt so the gate is not exposed to silent behavior changes in upstream releases, and enable actions/setup-python's pip cache (keyed off the requirements file) so subsequent CI runs skip the PyPI round-trip.
|
Solid CI gate, with two small items to address in this PR plus a recommendation on how to land the broader gate strategy. Two issues to address in this PR1.
|
| Gate | Goal | Catches | Implementation |
|---|---|---|---|
| 1 (this commit) | Naming + depth | Foo, some_key, depth > 5 |
Python + pyhocon |
| 2 | Bean ↔ key parity + default drift | key has no bean field, field has no key, Java default ≠ reference.conf default | Python + javalang or Gradle JUnit using Class.getDeclaredFields() + ConfigFactory.defaultReference() |
| 3 | Binding chain completeness | bean field exists but applyXxxConfig doesn't assign to PARAMETER, or no production consumer reads getXxx() |
Java AST or Gradle JUnit reflection |
| 4 | Inline comment coverage | Leaf keys without #/// documentation |
Python + pyhocon + raw text scan (carry-forward inline comments) |
| 5 | XxxConfigTest coverage |
Bean field present but no test assertion references it | Gradle JUnit (reflection on test bytecode) |
(Gate 6 — deprecation policy via git history scan — is qualitatively different and lower priority; defer.)
Why one PR + multi-commit rather than 5 follow-up PRs:
- Shared infrastructure — Gates 1/2/4 all use pyhocon, share GHA annotation emission, live in
.github/scripts/. Keeping them together avoids cross-PR refactoring. - Coherent design surface — reviewer can spot rules that interact (e.g. comment-coverage gate's line-attribution must match how naming gate walks segments).
- Avoids the "follow-up never happens" trap — multi-PR roadmaps in OSS projects often stall after Gate 1 lands.
- Independent rollback preserved —
git revert <gate-N-commit-sha>is as clean as reverting a standalone PR. - Project velocity context — the config series (refactor(config): simplify applyEventConfig (follow-up to #6615) #6735, fix(config): optimizes config binding for node #6755, refactor(config): merge test config files #6788, refactor(config): overhaul config docs, fix defaults, remove dead params #6790, refactor(config): remove unused storage index and json parsing config #6794, this PR) is concentrated in a few contributors; one focused PR avoids ×5 scoping/CI/review overhead.
Why Gate 2 is the highest-leverage follow-up: it's exactly the gate that would have caught the recent class of issues this series has been fixing manually:
- refactor(config): remove unused storage index and json parsing config #6794 removed
receiveTcpMinDataLength,maxNestingDepth,maxTokenCount,storage.index.directory/switch— all dead config keys discovered by manualgrep. Parity gate would have flagged them. - fix(config): optimizes config binding for node #6755 / refactor(config): overhaul config docs, fix defaults, remove dead params #6790 needed to fix several
allowShieldedTransactionApi/ dns defaults inconsistencies between Java field initializers andreference.conf. Drift gate would have caught them.
Suggested commit layout:
1. feat(ci): add reference.conf naming + depth gate ← current PR content
2. feat(ci): add bean ↔ key parity + default drift gate ← Gate 2 (highest ROI)
3. feat(ci): add config binding chain completeness gate
4. feat(ci): add inline comment coverage gate
5. feat(ci): add XxxConfigTest field coverage gate
For Gate 2 implementation, Gradle JUnit using Java reflection is likely simpler than Python + AST parsing — Class.getDeclaredFields() walks bean fields naturally, no Java AST parser needed. ~4–6 hr starting cost.
TL;DR
- ✅ Approve in principle — Gate 1 is solid, regex/depth logic verified empirically (296 keys, max depth 5)
- 🟠 Fix 2 small items before merge — add
shutdown.BlockTime/Height/Countto ALLOWLIST; tighten regex description in script - 📋 Suggest extending to all 5 gates in this PR as separate commits, rather than 5 follow-up PRs — shared infrastructure, coherent design surface, avoids the "follow-up never happens" trap
Add three PascalCase node.shutdown.{BlockTime,BlockHeight,BlockCount}
entries to ALLOWLIST. They are documented exceptions handled manually
in NodeConfig.fromConfig (not via ConfigBeanFactory) and are currently
commented out in reference.conf, so the gate does not see them today;
pre-listing keeps CI green if a future change uncomments them with
defaults.
Tighten the docstring for KEY_REGEX: the rule accepts acronyms from
position 1 onward (e.g. httpPBFTEnable, openHistoryQueryWhenLiteFN,
allowShieldedTRC20Transaction). That matches what
java.beans.Introspector / ConfigBeanFactory actually require — only
the first character must be lowercase — and avoids future reviewers
mistaking already-conformant keys for violations.
|
Two changes have been implemented. Gates 3/4/5 overlap with Gate 2.Gate 2's scope must be clarified first before we can determine whether 3/4/5 still stand independently |
|
Good gate — the naming-convention and depth ceiling cover two of the most common drift vectors. A few additional constraints worth considering for this script or follow-up PRs, roughly in order of implementation cost: 1. Duplicate-key detection HOCON silently keeps the last value when the same key appears twice, making typo-overwrites invisible. A text-level scan (before import collections
seen = collections.Counter()
for line in path.read_text().splitlines():
m = re.match(r'^\s*([\w.]+)\s*[={]', line)
if m:
seen[m.group(1)] += 1
duplicates = [k for k, c in seen.items() if c > 1]2. Port-uniqueness check reference.conf currently defines 10+ default ports. Two services sharing a port is a hard startup failure with a confusing error message. Extracting all *[Pp]ort leaf values and asserting they are distinct costs ~5 lines and 3. No non-empty default for sensitive keys Keys whose path contains password, secret, privateKey, or token should default to "" or be absent. A regex scan over leaf values would catch any accidental weak-credential default before it reaches a release tag. 4. Numeric-range sanity Many leaf types have well-known valid ranges implied by their names: These can be checked with a small name→constraint dispatch table using isinstance(value, int) after pyhocon parsing. 5. Bean-field ↔ key parity (the "Gate 2" mentioned in the PR description) This is the highest-leverage follow-up: reflect each *Config class to enumerate declared fields and cross-check against reference.conf keys in both directions — orphan keys (in conf, no field) and unbound fields (field |
4 participants
What does this PR do?
Adds a CI gate that validates
common/src/main/resources/reference.confon every PR / push:^[a-z][a-zA-Z0-9]*$: starts with a lowercase ASCII letter, then ASCII letters/digits only. Acronym runs after position 1 (e.g.httpPBFTEnable,openHistoryQueryWhenLiteFN,allowShieldedTRC20Transaction) are accepted — only the first character is constrained, which matches whatjava.beans.Introspector/ConfigBeanFactoryactually require for bean-property auto-binding.MAX_DEPTHvia a reviewed change).node.shutdown.*).[]= +1 level); nested arrays (HOCON list-of-list) are supported.Implementation:
.github/scripts/check_reference_conf.pyusingpyhocon(pinned via.github/scripts/requirements.txt), wired into the existingcheckstylejob in.github/workflows/pr-check.yml.actions/setup-pythonenables pip caching keyed on the requirements file. Violations produce per-line stdout output plus one consolidated::error file=...,title=reference.conf::...GHA annotation so failures show up in the PR check summary.Why are these changes required?
reference.confis the single source of default values for every config key in java-tron, and key names must auto-bind toXxxConfig.javabean fields via Typesafe Config'sConfigBeanFactory. This gate is stricter than typical Java/Spring projects (Spring Boot'sBinder/ Quarkus / Micronaut tolerate kebab-case, snake_case, UPPER_CASE, and camelCase as equivalent forms) becauseConfigBeanFactoryusesjava.beans.Introspectorand requires the HOCON key to match the JavaBean property name exactly — no case normalization. Without a CI gate, non-conforming keys silently fail to bind and accumulate.A depth ceiling also prevents deeply nested hierarchies from creeping in. The gate is set exactly at the current max with no buffer: silent drift is unacceptable for a mature project, so any new key that needs to go deeper has to bump
MAX_DEPTHexplicitly and be reviewed.This PR is the
release_v4.8.2counterpart of #6792 (which targetsdevelop), so the same gate runs on hotfix PRs against the release branch.This PR has been tested by:
release_v4.8.2'sreference.conf— passes with all keys, max depth 5.::errorannotation; a single user-declared deep key produces exactly one depth violation line (leaf-only filtering).Follow up
config.conffiles).Class.getDeclaredFields()reflection vs Java-AST parsing; default-value comparison rules across types; what counts as a config bean) warrant a separate PR.Extra details
config.confcompatible:node.http.PBFTEnable,node.http.PBFTPort,node.rpc.PBFTEnable,node.rpc.PBFTPort, plusnode.shutdown.BlockTime/BlockHeight/BlockCount(PascalCase exceptions handled manually inNodeConfig.fromConfig; currently commented out inreference.conf— pre-listed so the gate stays green if ever uncommented with defaults).pyhoconover Typesafe Config because the gate runs before Gradle/JDK setup in CI (~3 steps: setup-python + pip install + invoke). Both implement the HOCON spec; outputs are equivalent.