Repository navigation
[AI] Implement json.enable and json.dump_func in JSONPlugin (fixes #1539) - #1540
hassanzafarr wants to merge 2 commits into
Conversation
…y#1539) Move JSON configuration handling into JSONPlugin.apply(): - Respect route.config['json.enable'] and route.config['json.disable'] to allow disabling dict-to-json serialization globally or per-route. - Support route.config['json.dump_func'] for custom serialization. - Remove unused json.ascii and json.indent options from JSONPlugin.setup(). - Support passing config dictionary to Route via config keyword argument. - Add tests for json.enable, json.dump_func, and autojson deprecation. This contribution is dedicated to the public domain. ai-assisted-by: Antigravity (Gemini 3.8 Flash)
There was a problem hiding this comment.
🟡 Changes recommended
A critical custom-plugin behavior issue and missing json.disable coverage remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds configurable JSON response handling at application and route levels.
Changes:
- Supports
json.enable,json.disable, and customjson.dump_func. - Supports direct route configuration dictionaries.
- Adds regression tests for JSON settings and
Bottle(autojson=False).
File summaries
| File | Summary |
|---|---|
test/test_outputfilter.py |
Tests JSON configuration and custom serializers. Moderate (1 vote): add route-level json.disable coverage. |
test/test_app.py |
Tests the autojson=False constructor option. |
bottle.py |
Implements JSON plugin behavior and route configuration. Critical (2 votes): replacement JSONPlugin instances inherit json.enable=False after Bottle(autojson=False). Moderate (1 vote): missing global and route-level json.disable coverage. Nit (1 vote): update documentation references from autojson to the supported JSON settings. |
Review details
Suppressed comments (4)
bottle.py:1974
- The new
json.disablebranch is not covered by the added tests: they exercise global and route-leveljson.enable=False, but never setjson.disable=True. Sincejson.disableis a separate supported configuration path, a regression in this condition would leave the full new test set green; add at least global and route-level coverage.
if route.config.get('json.disable') or not route.config.get('json.enable', True):
bottle.py:1975
- This early return makes the JSON setting one-way after the route callback is cached. If a route is first accessed while
json.enableis false,Route.callstores the unwrapped callback; changing the app or route config to true later cannot enable serialization, even though the wrapper below rechecks the setting on every request. Keep the wrapper and let its runtime check bypass JSON while disabled.
if route.config.get('json.disable') or not route.config.get('json.enable', True):
return callback
bottle.py:1969
- These are now the supported public controls, but
docs/configuration.rststill tells users to setapp.config['autojson'] = False(including theload_dictexample), which JSONPlugin does not read. Following the documentation still leaves the default JSON plugin enabled, so update the configuration docs withjson.enable,json.dump_func, and route-levelconfig={...}examples as part of this fix.
app.config._define('json.enable', default=True, validate=bool,
help="Enable or disable automatic dict->json filter.")
app.config._define('json.dump_func', default=None,
help="If defined, use this function to transform"
" dict into json.")
test/test_outputfilter.py:117
- The implementation adds a separate
json.disablebranch, but the new tests exercise onlyjson.enable. Add a route usingconfig={'json.disable': True}and assert that its dict response remains unencoded, so the legacy switch named in this fix cannot regress unnoticed.
self.app.route('/raw', config={'json.enable': False})(lambda: {'a': 1})
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if route.config.get('json.disable') or not route.config.get('json.enable', True): | ||
| return callback |
- Remove one-way early return in JSONPlugin.apply() so route callbacks can dynamically toggle JSON serialization at runtime. - Ensure replacement JSONPlugin instances with custom serializers re-enable JSON handling after Bottle(autojson=False). - Add tests for global and route-level json.disable, dynamic toggling, and replacement JSONPlugin. - Update docs/configuration.rst example to use json.enable instead of autojson. This contribution is dedicated to the public domain. ai-assisted-by: Antigravity (Gemini 3.8 Flash)
|
Addressed review feedback in commit 4432727:
Human Here!! |
|
You are not @alisonatwork and they already said they wanted to look into it. Why are you now submitting an AI generated PR? |
|
I'm asking because of this rule in
The Copilot code review is also really bad.
|
Fixes #1539.
As discussed in #1539, this PR implements JSON configuration handling in
JSONPlugin:route.config['json.enable']androute.config['json.disable']to allow disabling the default dict->JSON transformation either globally or per-route dynamically.Bottle(autojson=False)while ensuring replacementJSONPlugininstances re-enable JSON handling with custom dump functions.route.config['json.dump_func']to specify a custom serialization function globally or per-route.json.asciiandjson.indentsettings fromJSONPlugin.setup().@app.route(..., config={...}).docs/configuration.rstexample to usejson.enable.test_outputfilter.pyandtest_app.pycovering global/route toggles,json.disable, dynamic toggling, and replacement plugins.Testing & Verification
python -m pytest test), with all 338 unit tests passing.This contribution is dedicated to the public domain.