Feat/implement real evaluator #64
Conversation
…e for challenge answers
…e SessionFactoryProvider configuration resolution
… tracking user-specific data
…ntralizing user context logic
… ChallengesServlet
…ecific DrillItems and enrollment
… challenge and user
…handling and drill item retrieval
…rvice persistence
…e in Drill and Practice servlets
ArchILLtect
added
enhancement
ArchILLtect
added
feature:practice-mode
There was a problem hiding this comment.
Pull request overview
This pull request implements an MVP evaluator service for CodeForge, enabling answer evaluation for coding challenges through a pluggable architecture. The PR introduces two distinct user flows: Practice (public, no persistence) and Drill (authenticated, with spaced repetition tracking). It adds per-user drill tracking via a userId field (Cognito sub), implements a basic string-matching evaluator with normalization, and enhances configuration management through a three-tier resolution system (system properties → environment variables → local.properties).
Key changes:
- Evaluator service with
BasicEvaluatorServicecomparing submissions againstChallenge.expectedAnswerusing normalization and fuzzy matching - Practice and Drill servlet flows with inline/flash feedback, feature flags for enabling/disabling features
- Per-user drill isolation:
userIdadded toDrillItemandSubmissionentities with user-scoped DAO queries - Configuration improvements:
LocalConfigutility, updated README with setup instructions, removed hardcoded credentials from test resources
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/resources/hibernate.properties |
Removed file to avoid committing database credentials (good security practice) |
src/test/resources/cleandb.sql |
Added expected_answer to challenges, user_id to drill_items/submissions; updated seed data |
src/test/java/.../DrillServletTest.java |
Added session mocking for userId; updated service calls to include userId parameter |
src/test/java/.../DrillServiceTest.java |
Updated all test methods to pass userId parameter to drill service methods |
src/test/java/.../DrillServicePersistenceTest.java |
New integration test verifying drill item persistence and field updates |
src/test/java/.../DrillServiceEnrollmentTest.java |
New unit test for auto-enrollment feature (creates missing drill items per user) |
src/test/java/.../ChallengeRunServiceTest.java |
Removed old heuristic tests; needs new evaluator integration tests |
src/test/java/.../persistence/*DaoTest.java |
Updated to set userId on all DrillItem and Submission instances |
src/test/java/.../BasicEvaluatorServiceTest.java |
New comprehensive tests for evaluator (exact match, punctuation/spacing, incorrect, guards) |
src/main/webapp/css/*.css |
Added alert styles for info/error feedback; practice form field styles |
src/main/webapp/WEB-INF/jsp/practice/solve.jsp |
New JSP for public practice mode with inline feedback display |
src/main/webapp/WEB-INF/jsp/drill/*.jsp |
Updated to show streak/next-due stats; flash messages; date formatting (has Instant→Date issue) |
src/main/webapp/WEB-INF/jsp/challenges/*.jsp |
Added expectedAnswer input fields; improved XSS escaping with c:out |
src/main/java/.../web/UserContext.java |
New helper to extract userId from session; centralizes auth context reading |
src/main/java/.../web/PracticeServlet.java |
Complete rewrite: handles GET/POST for practice mode, no persistence, inline feedback |
src/main/java/.../web/DrillServlet.java |
Enhanced with userId extraction, auto-enrollment, flash messages, evaluator integration |
src/main/java/.../web/ChallengesServlet.java |
Extracts expectedAnswer from form and passes to service |
src/main/java/.../web/ChallengeForm.java |
Added expectedAnswer field with validation |
src/main/java/.../web/AuthGuardFilter.java |
Exempts practice routes from authentication; unified auth check with UserContext |
src/main/java/.../service/DrillService.java |
All methods updated to require userId; added ensureEnrollmentForUser for auto-enrollment |
src/main/java/.../service/ChallengeService.java |
Sets expectedAnswer when creating/updating challenges |
src/main/java/.../service/ChallengeRunService.java |
Replaced heuristics with evaluator integration; added runWithMode and telemetry logging |
src/main/java/.../persistence/SubmissionDao.java |
Added listByChallengeIdAndUser query method |
src/main/java/.../persistence/SessionFactoryProvider.java |
Refactored to use three-tier config resolution; requires DB password from config |
src/main/java/.../persistence/DrillItemDao.java |
Added user-scoped queries: listByChallengeIdAndUser, dueQueue, soonestUpcoming, etc. |
src/main/java/.../evaluator/*.java |
New evaluator package with interface, DTO, BasicEvaluatorService, and Normalizer utility |
src/main/java/.../entity/*.java |
Added expectedAnswer to Challenge; userId to DrillItem and Submission |
src/main/java/.../controller/Auth.java |
Stores Cognito sub in session as "userSub" for drill features |
src/main/java/.../config/LocalConfig.java |
New utility to load local.properties from classpath for dev config |
src/main/resources/log4j2.properties |
Added telemetry appender with rolling file; updated root logger level |
src/main/resources/application.properties |
Added feature flags for practice and drill |
projects/mvp/README.md |
New MVP quick-start guide with config, routes, testing instructions |
docs/*.md |
Updated mvp-todo, added evaluator implementation plan and design docs |
README.md |
Enhanced with environment setup, database config instructions, local.properties template |
.gitignore |
Added local.properties to prevent committing dev credentials |
Comments suppressed due to low confidence (1)
src/test/resources/hibernate.properties:1
- Security: The entire hibernate.properties file containing database credentials has been deleted. This is good practice to avoid committing secrets to version control. However, ensure that the README instructions for creating this file locally are clear and that CI/CD pipelines have proper configuration to provide these values during testing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (path.matches("^/practice/\\d+/submit$")) { | ||
| needsAuth = false; | ||
| } |
There was a problem hiding this comment.
Security: The regex pattern ^/practice/\\d+/submit$ allows public POST submissions to practice challenges. While this is intentional per the design, ensure that PracticeServlet properly validates and sanitizes all inputs (language, code) to prevent injection attacks. Additionally, consider adding rate limiting to prevent abuse of the public evaluation endpoint.
| String dialect = resolve( | ||
| "hibernate.dialect", | ||
| "org.hibernate.dialect.MySQL8Dialect" | ||
| ); |
There was a problem hiding this comment.
Maintainability: The dialect has been changed from MySQLDialect to MySQL8Dialect on line 91. While this is likely correct for MySQL 8.x, it's a breaking change for environments still running MySQL 5.7 or earlier. Consider documenting this requirement in the README or making the dialect version-aware based on the actual MySQL version detected at runtime.
| if (language == null || language.isBlank() || code == null || code.isBlank()) { | ||
| Challenge challenge = challengeService.getById(id).orElse(null); | ||
| if (challenge == null) { resp.sendError(404); return; } | ||
| req.setAttribute("challenge", challenge); | ||
| req.setAttribute("mode", "practice"); | ||
| req.setAttribute("submittedCode", code); | ||
| req.setAttribute("outcome", me.nickhanson.codeforge.entity.Outcome.SKIPPED); | ||
| req.setAttribute("feedback", "Missing language or code. Please fill in both fields."); | ||
| req.getRequestDispatcher("/WEB-INF/jsp/practice/solve.jsp").forward(req, resp); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: When language or code is blank, the code sets outcome to SKIPPED and feedback message, but it still tries to fetch the challenge and forward to the JSP. However, if code is null, line 73 will pass null to req.setAttribute("submittedCode", code), which is fine. The real issue is that this validation logic duplicates what should happen in the evaluator. Consider calling the evaluator for all cases and letting it handle validation consistently, or at minimum, ensure the blank check for code uses code == null || code.isBlank() consistently with the evaluator's expectations.
| if (language == null || language.isBlank() || code == null || code.isBlank()) { | |
| Challenge challenge = challengeService.getById(id).orElse(null); | |
| if (challenge == null) { resp.sendError(404); return; } | |
| req.setAttribute("challenge", challenge); | |
| req.setAttribute("mode", "practice"); | |
| req.setAttribute("submittedCode", code); | |
| req.setAttribute("outcome", me.nickhanson.codeforge.entity.Outcome.SKIPPED); | |
| req.setAttribute("feedback", "Missing language or code. Please fill in both fields."); | |
| req.getRequestDispatcher("/WEB-INF/jsp/practice/solve.jsp").forward(req, resp); | |
| return; | |
| } |
Documentation: The comment syntax // e.g., localhost is not valid in .properties files. Properties files use # for comments. The correct format should be: # e.g., localhost or remove the comment entirely and use regular key=value format. This applies to all the example property comments on lines 187-191. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/test/resources/hibernate.properties:1
- This file contained a hardcoded database password (
ArchILLtect2005!) that was committed to version control. While this PR correctly deletes the file (it should be git-ignored), the password has been exposed in the git history and should be considered compromised. The database password should be rotated immediately. Verify that.gitignoreincludessrc/test/resources/hibernate.propertiesto prevent future credential leaks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -32,43 +39,32 @@ public class ChallengeRunService { | |||
| * @return A RunResult object representing the outcome of the evaluation. | |||
| */ | |||
There was a problem hiding this comment.
The run() method now just delegates to runWithMode() with mode="unknown". Consider deprecating this method with @Deprecated annotation and updating callers to use runWithMode() directly for better telemetry tracking.
| */ | |
| */ | |
| /** | |
| * @deprecated Use {@link #runWithMode(String, Long, String, String)} for better telemetry tracking. | |
| */ | |
| @Deprecated |
This pull request introduces the MVP implementation of a dedicated evaluator service for CodeForge, along with significant enhancements to the Practice and Drill flows, improved configuration and onboarding documentation, and better user/session tracking. The changes focus on enabling answer evaluation for coding challenges, wiring feedback into user flows, and supporting per-user drill features. The update also adds required model fields, configuration utilities, and documentation for a smoother setup and testing experience.
Evaluator Service & Challenge Evaluation
AnswerEvaluatorinterface,AnswerEvaluationDTO, andBasicEvaluatorServicethat compares challenge submissions to expected answers with normalization and feedback. [1] [2] [3]Challengeentity to include anexpectedAnswerfield for evaluation purposes.Practice & Drill Flow Enhancements
DrillItemandSubmissionentities now include auserIdfield (Cognito sub) for per-user drill and submission tracking. [1] [2]Configuration & Documentation Updates
LocalConfigutility for loading local properties, and updatedREADME.mdwith setup instructions for environment variables and test database configuration. [1] [2] [3]projects/mvp/README.mdand improved documentation for evaluator, Practice/Drill flows, troubleshooting, and quick start. [1] [2]Testing & MVP Completion
Closes #53
Closes #54
Closes #55
Closes #56
Closes #57
Closes #58
Closes #59
Closes #60
Closes #61
Closes #62
References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]