feat(backend): add meta field to question generation endpoints#493
Open
toyes3429 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
feat(backend): add meta field to question generation endpoints#493toyes3429 wants to merge 1 commit intoAOSSIE-Org:mainfrom
toyes3429 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
- Add requested, generated, and status fields to /get_mcq, /get_boolq, /get_shortq, and /get_problems responses - Status values: success | partial | insufficient - Add require_json_field helper with centralized input validation - Add global error handlers returning structured JSON instead of HTML - Non-breaking change: existing output field unchanged Closes AOSSIE-Org#463
📝 WalkthroughWalkthroughThe pull request adds a GPL v3 license file and introduces centralized error handling, input validation utilities, and metadata tracking to question generation API endpoints to expose requested vs. generated question counts with status indicators. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (3)
backend/server.py (2)
193-201: Consider extractingbuild_metato module level for reuse.This helper duplicates the status logic from
/get_mcq,/get_boolq, and/get_shortq. Extracting it would reduce duplication and ensure consistency.Proposed refactor
# At module level (e.g., after require_json_field) def build_meta(questions, requested): """Build metadata dict with requested/generated counts and status.""" generated = len(questions) if generated == 0: status = "insufficient" elif generated < requested: status = "partial" else: status = "success" return {"requested": requested, "generated": generated, "status": status}Then reuse in all four endpoints:
# In get_mcq, get_boolq, get_shortq: meta = build_meta(questions, max_questions) return jsonify({"output": questions, "meta": meta})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/server.py` around lines 193 - 201, Extract the nested build_meta function to module scope (e.g., alongside require_json_field) so it can be reused; move the existing logic into a top-level def build_meta(questions, requested) and update each endpoint handler get_mcq, get_boolq, and get_shortq to call this module-level build_meta(questions, max_questions) when constructing the response meta before jsonify. Ensure the symbol name build_meta remains unchanged so calls throughout the file continue to work and remove the duplicated in-function definitions.
2-4: Remove unused imports.
NotFoundandsecure_filenameare imported but never used in this file.Proposed fix
from collections.abc import Mapping -from werkzeug.exceptions import HTTPException, BadRequest, NotFound -from werkzeug.utils import secure_filename +from werkzeug.exceptions import HTTPException, BadRequest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/server.py` around lines 2 - 4, Remove the unused imports NotFound and secure_filename from the top-level import statements: keep Mapping from collections.abc and HTTPException and BadRequest from werkzeug.exceptions, but delete NotFound and remove the import of secure_filename from werkzeug.utils; update the import lines where these symbols appear (the lines containing "from werkzeug.exceptions import HTTPException, BadRequest, NotFound" and "from werkzeug.utils import secure_filename") so only the actually used names remain.LICENSE.md (1)
1-595: License file appears unrelated to PR objectives.This PR's stated purpose is to add metadata to question generation API responses (closing issue
#463). Adding a LICENSE.md file seems outside the scope of this change. Consider splitting this into a separate PR for clearer change tracking and review, unless the license addition is intentionally bundled with this feature work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LICENSE.md` around lines 1 - 595, The LICENSE.md addition is unrelated to the stated change (adding metadata to question generation API responses for issue `#463`); remove LICENSE.md from this PR and instead create a separate PR that adds the license, or move the LICENSE.md commit onto a dedicated branch/PR, ensuring this PR contains only changes tied to the question generation metadata (e.g., the files/methods implementing the question generation API and its response schema).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/server.py`:
- Around line 73-82: The require_json_field function currently accepts
whitespace-only strings; update it to trim string inputs and reject strings that
are empty after stripping: when value is a str, call strip() and if the result
is empty raise BadRequest(f"{field_name} cannot be empty"); return the stripped
string (instead of the original) so callers get the normalized value; leave
non-string checks (None, empty list/dict) as-is.
- Around line 64-70: The global Flask error handler handle_unexpected_exception
should log the caught exception before returning the generic JSON response: use
the app logger (e.g., app.logger.exception or logging.exception) inside
handle_unexpected_exception to record the exception and traceback for
server-side debugging, then return the same sanitized response (error/type and
500) without exposing exception details to the client.
---
Nitpick comments:
In `@backend/server.py`:
- Around line 193-201: Extract the nested build_meta function to module scope
(e.g., alongside require_json_field) so it can be reused; move the existing
logic into a top-level def build_meta(questions, requested) and update each
endpoint handler get_mcq, get_boolq, and get_shortq to call this module-level
build_meta(questions, max_questions) when constructing the response meta before
jsonify. Ensure the symbol name build_meta remains unchanged so calls throughout
the file continue to work and remove the duplicated in-function definitions.
- Around line 2-4: Remove the unused imports NotFound and secure_filename from
the top-level import statements: keep Mapping from collections.abc and
HTTPException and BadRequest from werkzeug.exceptions, but delete NotFound and
remove the import of secure_filename from werkzeug.utils; update the import
lines where these symbols appear (the lines containing "from werkzeug.exceptions
import HTTPException, BadRequest, NotFound" and "from werkzeug.utils import
secure_filename") so only the actually used names remain.
In `@LICENSE.md`:
- Around line 1-595: The LICENSE.md addition is unrelated to the stated change
(adding metadata to question generation API responses for issue `#463`); remove
LICENSE.md from this PR and instead create a separate PR that adds the license,
or move the LICENSE.md commit onto a dedicated branch/PR, ensuring this PR
contains only changes tied to the question generation metadata (e.g., the
files/methods implementing the question generation API and its response schema).
Comment on lines
+64
to
+70
| @app.errorhandler(Exception) | ||
| def handle_unexpected_exception(e): | ||
| """Handle unexpected exceptions with a generic JSON response.""" | ||
| return jsonify({ | ||
| "error": "Internal server error", | ||
| "type": "InternalServerError", | ||
| }), 500 |
There was a problem hiding this comment.
Log unexpected exceptions before returning generic error.
The exception e is caught but not logged, making production debugging difficult. While hiding error details from clients is correct for security, the exception should be logged server-side.
Proposed fix
+import logging
+
+logger = logging.getLogger(__name__)
+
`@app.errorhandler`(Exception)
def handle_unexpected_exception(e):
"""Handle unexpected exceptions with a generic JSON response."""
+ logger.exception("Unexpected error occurred")
return jsonify({
"error": "Internal server error",
"type": "InternalServerError",
}), 500🧰 Tools
🪛 Ruff (0.15.2)
[warning] 65-65: Unused function argument: e
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/server.py` around lines 64 - 70, The global Flask error handler
handle_unexpected_exception should log the caught exception before returning the
generic JSON response: use the app logger (e.g., app.logger.exception or
logging.exception) inside handle_unexpected_exception to record the exception
and traceback for server-side debugging, then return the same sanitized response
(error/type and 500) without exposing exception details to the client.
Comment on lines
+73
to
+82
| def require_json_field(data, field_name): | ||
| """Validate JSON payload and return a required field.""" | ||
| if not isinstance(data, Mapping): | ||
| raise BadRequest("JSON body must be an object") | ||
| if field_name not in data: | ||
| raise BadRequest(f"{field_name} is required") | ||
| value = data[field_name] | ||
| if value in (None, "", [], {}): | ||
| raise BadRequest(f"{field_name} cannot be empty") | ||
| return value |
There was a problem hiding this comment.
Consider stripping whitespace from string inputs.
Whitespace-only strings like " " pass the current validation but would produce no questions. Consider stripping and validating string inputs more strictly.
Proposed fix
def require_json_field(data, field_name):
"""Validate JSON payload and return a required field."""
if not isinstance(data, Mapping):
raise BadRequest("JSON body must be an object")
if field_name not in data:
raise BadRequest(f"{field_name} is required")
value = data[field_name]
+ if isinstance(value, str):
+ value = value.strip()
if value in (None, "", [], {}):
raise BadRequest(f"{field_name} cannot be empty")
return value🧰 Tools
🪛 Ruff (0.15.2)
[warning] 76-76: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 78-78: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 81-81: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/server.py` around lines 73 - 82, The require_json_field function
currently accepts whitespace-only strings; update it to trim string inputs and
reject strings that are empty after stripping: when value is a str, call strip()
and if the result is empty raise BadRequest(f"{field_name} cannot be empty");
return the stripped string (instead of the original) so callers get the
normalized value; leave non-string checks (None, empty list/dict) as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Closes #463
Screenshots/Recordings:
Additional Notes:
What this PR does
Adds a
metafield to responses from/get_mcq,/get_boolq,/get_shortq, and/get_problemsendpoints.Why
When fewer questions are generated than requested, the frontend
receives a shorter array with no context. This makes it impossible
to distinguish between partial success, insufficient input, and failure.
Response change (non-breaking)
Existing clients reading only
outputare unaffected.Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: I have used the following AI models and tools: Claude (Anthropic) —
for code structure guidance and review. All changes were tested locally
using Thunder Client and verified across all three status values
(success, partial, insufficient). I understand every line of code in
this PR and take full responsibility for it
Summary by CodeRabbit
Bug Fixes
New Features
Chores