Fix dynamic-reflection and structural-discovery correctness issues found in the 2026-07-15 audit - #153
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes tighten dynamic method filtering, extend dynamic field traversal to interfaces, add JavaParser-based discovery for nested and additional top-level classes, improve structural parameter matching, and expand unit and integration coverage. ChangesDynamic API behaviour
Structural test discovery and matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClassNameScanner
participant JavaParser
participant StructuralTestProvider
ClassNameScanner->>JavaParser: parse Java source
JavaParser-->>ClassNameScanner: return discovered type names
ClassNameScanner->>StructuralTestProvider: register discovered classes
StructuralTestProvider->>StructuralTestProvider: convert nested names for Class.forName
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 4❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LukaPetrovicTUM
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java`:
- Around line 60-91: Update the shared parser used by
ClassNameScanner.qualifiedTypeNamesOf(...) so concurrent scans cannot invoke
JAVA_PARSER.parse(...) on the same mutable JavaParser instance; use a per-thread
JavaParser with the existing Java 17 ParserConfiguration or synchronize every
parse call, while preserving the current parsing behavior and cache flow.
- Around line 329-344: Update parseQualifiedTypeNames to also catch
StackOverflowError from JavaParser and use the existing fallback path: log the
parse failure and return fileNameDerivedTypeName(node.getName()). Preserve the
current handling for IOException and ParseProblemException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2232cd45-c348-432b-8d02-b530ee95a57a
📒 Files selected for processing (13)
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/resources/de/tum/cit/ase/ares/integration/testuser/test.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build
- GitHub Check: Analyse Java
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ CodeRabbit configuration file
Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.
Files:
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/test/resources/de/tum/cit/ase/ares/integration/testuser/test.jsonsrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.
Files:
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java
**/*Test.java
📄 CodeRabbit inference engine (AGENTS.md)
**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness
Files:
src/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.java
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.
Files:
src/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.java
🔇 Additional comments (17)
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.java (1)
208-208: LGTM!src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.java (1)
111-130: LGTM!src/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.java (1)
1-48: LGTM!src/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.java (1)
1-58: LGTM!src/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.java (1)
200-200: LGTM!src/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.java (3)
125-146: LGTM!
298-354: LGTM!The switch from simple-name arrays/hash-map occurrence counting to
checkExpectedType-based comparison plus Kuhn's algorithm for the unordered case is correct and well covered byStructuralTestProviderTest(duplicate-type rejection, canonical-name acceptance regardless of position, etc.).Also applies to: 356-401
527-538: LGTM!src/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java (3)
87-91: 🩺 Stability & AvailabilityVerify deployment model for the static path+mtime cache.
JAVA_FILE_TYPE_CACHEis a JVM-static map keyed only by absolute path and last-modified time. If this harness JVM is ever reused across separate student submissions that occupy the same workspace path, a coincidental mtime match (e.g., a checkout/extraction tool stamping a fixed reference time) could serve a stale discovery result from a different submission's file, silently corrupting grading output — a correctness concern squarely in "global mutable state" territory for a framework that tests untrusted code.Could you confirm whether each structural-test run executes in its own fresh JVM (in which case this is safe, as the accompanying comment implies for "a single structural test run"), or whether the harness JVM/process can be reused across submissions?
Also applies to: 317-327
266-286: LGTM!
287-328: LGTM!
qualifiedNameWithinFile's handling of local/anonymous classes and the dot-separated qualified naming is correct and matches the accompanyingClassNameScannerTestcoverage.Also applies to: 359-416
src/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.java (1)
28-66: LGTM!Good targeted coverage of top-level, additional top-level, nested, doubly-nested, and local-class cases.
src/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.java (1)
1-79: LGTM!Solid regression coverage for both strict-order canonical-name acceptance and unordered bipartite matching, including the duplicate-type rejection edge case that a naive greedy pairing would get wrong.
src/test/java/de/tum/cit/ase/ares/integration/StructuralTest.java (1)
22-26: LGTM!Also applies to: 36-72, 114-117, 200-208, 238-246
src/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.java (1)
36-36: LGTM!Also applies to: 46-46, 116-123
src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.java (1)
6-27: LGTM!src/test/resources/de/tum/cit/ase/ares/integration/testuser/test.json (1)
165-197: LGTM!
…und in the 2026-07-15 audit
…und in the 2026-07-15 audit
10d1e8f to
5e14b76
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.java (1)
458-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
createParametersHashMaphelper.
createParametersHashMaphas no Java callers, so drop it unless it is needed for another supported purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.java` around lines 458 - 469, Remove the unused createParametersHashMap method from StructuralTestProvider, including its parameter-counting implementation, since it has no Java callers or other supported purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.java`:
- Around line 208-209: Update the exemption condition in DynamicClass to
recognize main(java.lang.String[]) only when the declared method is public
static void, preserving normal validation for instance or non-void main methods.
Add regression cases covering public instance and non-void variants to verify
they are not exempted.
In `@src/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.java`:
- Around line 19-47: Add separate test fixtures for a public instance void
main(String[]) and a public static int main(String[]) alongside
MainSuffixFixture, then add tests using
DynamicClass.toDynamic(...).checkForPublicOrProtectedMethods() that assert each
throws AssertionFailedError and identifies main(java.lang.String[]).
In `@src/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.java`:
- Around line 52-56: Update
lookupStartingFromAnInterfaceDoesNotThrowOnNullSuperclass to capture the boolean
result of the missing-field lookup, assert that it is false, and retain the
no-exception assertion so the test verifies both safe traversal and absence
semantics.
---
Outside diff comments:
In
`@src/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.java`:
- Around line 458-469: Remove the unused createParametersHashMap method from
StructuralTestProvider, including its parameter-counting implementation, since
it has no Java callers or other supported purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af4a35c7-ba37-47e8-9287-03cd711b388b
📒 Files selected for processing (13)
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/resources/de/tum/cit/ase/ares/integration/testuser/test.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Build
- GitHub Check: Analyse Java
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ CodeRabbit configuration file
Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.
Files:
src/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/resources/de/tum/cit/ase/ares/integration/testuser/test.jsonsrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.java
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.
Files:
src/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.javasrc/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.javasrc/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.java
**/*Test.java
📄 CodeRabbit inference engine (AGENTS.md)
**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness
Files:
src/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicClassTest.javasrc/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.javasrc/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.javasrc/test/java/de/tum/cit/ase/ares/integration/StructuralTest.java
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.
Files:
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicClass.javasrc/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.javasrc/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.javasrc/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java
🔇 Additional comments (16)
src/main/java/de/tum/cit/ase/ares/api/dynamic/DynamicField.java (1)
111-130: LGTM!src/test/java/de/tum/cit/ase/ares/api/dynamic/DynamicFieldTest.java (1)
18-50: LGTM!src/test/java/de/tum/cit/ase/ares/integration/testuser/DynamicsUser.java (1)
200-200: LGTM!src/test/java/de/tum/cit/ase/ares/integration/StructuralTest.java (2)
36-51: Substring-matching fragility is transparently documented; no action needed now.The comment correctly identifies that
EventConditions#test(String)matches dynamic-test IDs by substring, so"dynamic-test:#1"would also match"dynamic-test:#10"once a container has 10 or more entries. None of the current containers cross that boundary, so this is not a live defect, but it is a latent fragility worth keeping in mind for future fixture additions totestConstructors()ortestMethods().
114-118: New dynamic-test assertions are consistent with the fixture and oracle.I cross-checked the new
testAttributesSomeClassNested,testConstructorsSomeClassNested,testConstructorsAdditionalTopLevelType,testMethodsSomeClassNested, andtestMethodsAdditionalTopLevelTypeIDs againsttest.json's new entries andMisspelledClass.java'sAdditionalTopLevelType. The numbering, class filtering rationale (notestClasses()entry for either fixture, since neither declares class-level properties), and expected outcomes all align.Also applies to: 200-209, 238-247
src/main/java/de/tum/cit/ase/ares/api/structural/StructuralTestProvider.java (2)
336-401: Bipartite matching implementation verified correct.I traced
hasMatchingPairingForEachExpectedParameter/tryPairExpectedParameterthrough several scenarios, including the augmenting-path case exercised byunorderedMatchAcceptsRepeatedTypeinStructuralTestProviderTest.java. The algorithm correctly implements Kuhn's augmenting-path search and produces the expected results for duplicate and ambiguous parameter types.
130-133: Nested-class name translation is consistent end-to-end.The dot-to-
$translation ingetQualifiedClassName()usesString.replace(literal), notreplaceAll(regex), which correctly avoids mangling every character. I cross-checked this againstClassNameScanner.qualifiedNameWithinFile()(dot-separatedOuter.Innerregistration) andtest.json's"SomeClass.Nested"entry: the naming convention is consistent across discovery, storage, and class loading.Also applies to: 302-308, 528-537
src/main/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScanner.java (4)
60-91: Thread-safety fix confirmed correct.The
ThreadLocal<JavaParser>correctly addresses the previously flagged concurrency concern. A web search confirms aJavaParserinstance "is not thread safe", so per-thread instantiation is the right fix, and the accompanying comment correctly documents the rationale.
330-345:StackOverflowErrorhandling fix confirmed correct.The
catch (IOException | ParseProblemException | StackOverflowError e)clause correctly addresses the previously flagged gap and falls back to the filename-derived type name, consistent with the fallback behaviour used elsewhere in this method.
318-328: Cache correctness verified for the common case.
qualifiedTypeNamesOfkeys the cache by normalised absolute path and invalidates onlastModified()change. A concurrent get-then-put is not atomic, so two threads could redundantly re-parse the same unchanged file, but this cannot corrupt the cached result since both threads compute the same value from the same file content. This is a minor, self-recovering inefficiency, not a correctness defect.
360-374:qualifiedNameWithinFilelogic verified against test coverage.The parent-chain walk correctly distinguishes top-level, member, and doubly nested types from local classes by checking whether the chain terminates at a
CompilationUnit. This matches all five new test cases inClassNameScannerTest.java.src/test/java/de/tum/cit/ase/ares/api/structural/testutils/ClassNameScannerTest.java (1)
28-65: LGTM!src/test/java/de/tum/cit/ase/ares/api/structural/StructuralTestProviderTest.java (1)
1-79: Good regression coverage for the new matching behaviour.The tests cover strict-order and unordered matching, canonical vs. simple names, duplicate types, mismatched counts, and the empty-parameters case. I traced the bipartite-matching implementation against
unorderedMatchAcceptsRepeatedTypeandunorderedMatchStillRejectsAWrongDuplicateTypeand confirmed the algorithm behaves as asserted.src/test/java/de/tum/cit/ase/ares/integration/testuser/StructuralUser.java (1)
36-36:StrucuralTestSet→StructuralTestSetrename andabstractmodifier are correct.Marking the shared nested test base
abstractis the right fix for Surefire's stray-test-class heuristic, since the class declares@Nested/@TestFactorymembers but is itself neither static,@Nested, nor abstract before this change.Also applies to: 46-46, 116-123
src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/structural/MisspelledClass.java (1)
6-27:AdditionalTopLevelTypefixture matches its oracle entry.The constructor and
acceptCanonicalParametermethod signatures match thejava.lang.Stringcanonical parameter types declared intest.json, and the package-private visibility is valid Java syntax alongside the publicMisspelledClassin the same file.src/test/resources/de/tum/cit/ase/ares/integration/testuser/test.json (1)
165-197: New oracle entries are consistent with the fixture classes.The
SomeClass.NestedandAdditionalTopLevelTypeentries match their corresponding Java fixtures and the dot-separated nested-class naming convention used elsewhere in this file.
…und in the 2026-07-15 audit
SedaOran
left a comment
SedaOran
left a comment
There was a problem hiding this comment.
Approving, code LGTM.
Cleanly fixes four real correctness bugs from the audit (the main endsWith check, the DynamicField interface NPE, canonical/bipartite parameter matching, and nested-type discovery), each with solid regression tests. All 13 CI checks green, including Core Integration Tests.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@LukaPetrovicTUM The fixes correctly tighten dynamic reflection validation, traverse interface fields safely, discover nested and additional top-level types, and match canonical parameter names without weakening mismatch detection. The regression coverage is meaningful, all captured CI checks pass, and the previously raised review concerns are resolved on this head.
|
The pull request template now bounds each section: 500 characters for Summary, 1000 for This description was written before those limits and exceeds 1 of them, so I have 1. Problem, as it read before (3981 characters, limit 1000)This PR bundles four independent findings on one branch (explicit exception to one-branch-per-issue, I-094 — I-052 — I-099 — structural discovery and parameter matching. Observed: TD-043 — non-asserting |
4 participants
Summary
Fixes four correctness issues from the 2026-07-15 audit (I-052, I-094, I-099, TD-043): two
reflection-validation bugs in the dynamic test-authoring API, a misconfigured Surefire test-container
class, and structural type-discovery/parameter-matching gaps that could fail correct submissions.
Linked issues
None. (Findings I-052, I-094, I-099 and TD-043 come from an internal 2026-07-15 audit report, not
GitHub issues.)
1. Problem
Four independent findings on one branch, each with its own cause. The original wording is kept in a comment below.
I-094, the dynamic reflection API.
checkForPublicOrProtectedMethodsmatched themainsignature with
endsWithrather thanequals, so a public method whose signature was a suffixof it was wrongly exempt.
DynamicField.fieldsOfwalked superclasses only, so it threw on aninterface and never saw superinterface fields.
I-099, structural discovery.
ClassNameScannertook a file's one type from its filename,so member and extra top-level types were undiscoverable, and
checkParameterscomparedparameters by simple name, so
java.lang.Stringcould never match. A structurally correctsubmission failed: a false positive.
I-052 and TD-043, hygiene. A test set was neither
staticnor@Nested, so Surefirewarned about a stray class, and an
assertThatwith no predicate made a fixture line a no-op.None of the four sits in the runtime sandbox.
2. Improvement from the user's perspective
Students: a submission using a member/nested class, or declaring a parameter with its fully-qualified
type name, is no longer incorrectly failed by a structural exercise's
test.jsonoracle checks(I-099). Instructors: dynamic-reflection-based structural assertions against interface-based fixtures
no longer crash with
NullPointerException(I-094); Maven/Gradle build output for structural testmodules no longer carries a spurious Surefire stray-test-class warning (I-052).
3. Improvement from the maintainer's perspective
New unit test coverage for
api/dynamic(previously zero unit tests, only indirect coverage via theDynamicsTestintegration suite).checkParameters's matching is now a correct bipartite algorithminstead of a simple-name-only multiset comparison — every structural test provider
(
ClassTestProvider,MethodTestProvider,ConstructorTestProvider) that compares parametersbenefits from the same fix. TD-043's non-asserting test now genuinely protects the behaviour it
claims to. A related latent fragility (
ast/model/JavaFile.java's implicit dependence onStaticJavaParser's shared global configuration rather than an explicitly-configured instance) wasfound while debugging this PR's own
ClassNameScannerchange and has been raised with the maintainerseparately — it is not touched here.
4. Testing manual
Prerequisites
pom.xml'sRequireMavenVersionenforcer rule) and JDK 17+ (CI builds/testson JDK 21). No external exercise repository or policy file is needed: every fixture this PR
touches lives inside this repo's own simulated exercise at
src/test/resources/de/tum/cit/ase/ares/integration/testuser/(apom.xml+build.gradlepair,a
test.jsonstructural oracle, and fixture "student" classes undertestuser/subject/structural/), which theStructuralTest/DynamicsTestintegration suitesalready drive end-to-end exactly as an instructor's real exercise repository would.
Steps
mvn test -Punit-core-tests -f pom.xml -Dtest=DynamicClassTest,DynamicFieldTest,ClassNameScannerTest,StructuralTestProviderTestmvn test -Pintegration-core-tests -f pom.xml -Dtest=de.tum.cit.ase.ares.integration.DynamicsTestmvn test -Pintegration-core-tests -f pom.xml -Dtest=de.tum.cit.ase.ares.integration.StructuralTestExpected result
DynamicsTestcases pass, includingtest_class_searchPublicOrProtectedMethodsandevery
test_field_*case.StructuralTestcases pass, including the newtest_testAttributesSomeClassNested,test_testConstructorsSomeClassNested,test_testConstructorsAdditionalTopLevelType,test_testMethodsSomeClassNestedandtest_testMethodsAdditionalTopLevelTypeassertions provingthe previously-undiscoverable
SomeClass.Nestedmember class and the canonical-parameterAdditionalTopLevelTypefixture are now found and checked correctly.Negative case (what must still be rejected)
test_testClassMisspelledClas/test_testClassMisspelledclassmust still fail with the sametypo/case-mismatch messages as before — the discovery rewrite must not become lenient about
genuinely wrong class names.
test_class_searchPublicOrProtectedMethodsmust still fail (reject) a real public-API violation(
doSomething(String)wrongly declared public) — confirms themain-suffix fix didn'tover-correct into exempting unrelated public methods.
test_testConstructorsSomeFailingClass/test_testMethodsSomeFailingClassmust still fail ongenuinely mismatched parameter lists — confirms the new bipartite parameter matching didn't become
permissive about real mismatches.
Modes exercised
No mode-specific behaviour changed — this PR touches only
api/dynamicandapi/structural(reflection/structural-oracle utilities behind Ares' own test-authoring API), not
aop/orarchitecture/enforcement code.5. Test case coverage regarding this PR
Every counter JaCoCo produces per class, read from
site/jacoco/jacoco.csvin thecoverage-reportartefact of the Coverage Report job on this branch (workflow run30800342411, the most recent green run). Each cell is COVERED out of MISSED plus COVERED, so a reviewer can recompute it. Nested classes are listed as their own rows.n/a (0/0)means the counter has no members at all for that class.DynamicClassDynamicFieldStructuralTestProviderStructuralTestProvider.ExpectedClassStructureStructuralTestProvider.ModifierSpecificationClassNameScannerClassNameScanner.CachedFileTypesConfirmation basis:
DynamicClassTest,DynamicFieldTest,StructuralTestProviderTestandClassNameScannerTest(all changed by this pull request) assert the resolved members and the rejection messages themselves, so the reflection and structural-oracle defects fixed here fail an assertion rather than merely changing a covered line.Breaking changes and migration
None. None of these four fixes change the shape of the public API under
de.tum.cit.ase.ares.api.dynamic/de.tum.cit.ase.ares.api.structural(no signatures added orremoved), the security policy file format/schema, or the generated security test code.
checkParametersaccepting canonical type names is strictly more permissive than before (nothingthat previously matched now fails to match); the
DynamicField/DynamicClassfixes correct a crashand a false exemption without altering any existing method signature. Minimum JDK/Maven/Gradle
versions are unchanged.
Checklist
docs/,README.adoc, Javadoc) was updated where the change is user-facing — nodocs/file describes the specific matching/discovery behaviour that changed (docs/Overview.md's existing high-level description ofapi/dynamic/api/structuralremains accurate), so nothing needed updating.grepagainst the actual diff, not assumed).Review progress