feat: major improvement - #89
Conversation
- remove cors - remplace catcher with wrappe function - allows user to inject function in template engine - only tera is available
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe 0.10.0 release removes catcher and Minijinja support, adds global request wrappers, changes middleware chains to borrow slices, consolidates templating on Tera, updates stub generation, broadens JSON conversion input, and synchronizes package metadata. ChangesCore API and runtime refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HttpServer
participant Request
participant Response
participant PythonWrapper
HttpServer->>Request: create context with wrapper
Request->>Response: process route or not-found response
Response->>PythonWrapper: call wrapper(request, response)
PythonWrapper-->>Response: return converted response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/templating/mod.rs (1)
202-207: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the stale
templatstub forTera.
oxapy/templating/__init__.pyistill exportsTerawith__new__/render, but the extension only registersTemplateviatemplating_submodule. Regenerate/remove that generated stub to avoid exposing a Python class that no longer exists in the compiled module.🤖 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/templating/mod.rs` around lines 202 - 207, Remove or regenerate the stale Tera declaration from oxapy/templating/__init__.pyi, including its __new__ and render members, so the stub matches templating_submodule’s registered Template class and render function without exposing a nonexistent Tera class.
🧹 Nitpick comments (4)
src/templating/mod.rs (3)
133-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueException type fully-qualified instead of imported; error message wording is awkward.
pyo3::exceptions::PyRuntimeErroris referenced inline rather than via an import (comparePyValueError, which is imported at the top of the file). Also, "Tera engine is already shared cloned copies" reads oddly and could be clearer, e.g. "Cannot register function: the Template's Tera engine is already shared (register functions before attaching the Template to the server)."As per coding guidelines, "Use
pyo3::exceptions::*for standard Python exceptions in Rust code."♻️ Suggested fix
-use pyo3::{ - Bound, PyResult, - exceptions::PyValueError, - prelude::*, - types::{PyDict, PyModule, PyModuleMethods}, -}; +use pyo3::{ + Bound, PyResult, + exceptions::*, + prelude::*, + types::{PyDict, PyModule, PyModuleMethods}, +};- Err(PyErr::new::<pyo3::exceptions::PyRuntimeError, _>( - "Cannot register function: Tera engine is already shared cloned copies", + Err(PyErr::new::<PyRuntimeError, _>( + "Cannot register function: the Tera engine is already shared; register functions before attaching this Template to the server", ))🤖 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/templating/mod.rs` around lines 133 - 143, Update register_function to use the imported PyRuntimeError type instead of the fully qualified pyo3::exceptions path, and revise the error text to clearly state that the Template’s Tera engine is already shared and functions must be registered before attaching the Template to the server.Source: Coding guidelines
114-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
renderandregister_function(both public pymethods) have no doc comments.Neither method documents
Args/Returns/Example, unlikenewabove them.register_functionin particular is a new public API central to this PR's stated goal of letting users register template functions — worth documenting its behavior, especially the "must be called before theTemplateis shared with the server, or it errors" constraint (seeArc::get_mutcheck below), since that's a non-obvious usage requirement.As per coding guidelines, "Use doc comments
///for public APIs in Rust" and "Include Args, Returns, and Example sections in Rust docstrings for public APIs."🤖 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/templating/mod.rs` around lines 114 - 143, Add Rust doc comments to the public pymethods render and register_function, including Args, Returns, and Example sections. Document render’s template/context inputs and rendered-string result, and document register_function’s callable registration behavior plus its requirement to run before the Template engine is shared, including the resulting error when that requirement is violated.Source: Coding guidelines
90-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring still describes removed engine-selection behavior.
Raises: PyException: If an invalid engine type is specified.and the# Use Jinja with default template directory/templating.Template()example are stale now thatTemplate::newonly takesdirand Jinja/minijinja support has been removed entirely. This will surface in the generated.pyistub docs and mislead users.🤖 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/templating/mod.rs` around lines 90 - 101, Update the documentation for Template::new to remove the obsolete invalid-engine PyException description and Jinja-specific example, including templating.Template() without arguments. Revise the example and surrounding text to reflect the current dir-only constructor and supported engine behavior.README.md (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
alttext to the version badge image.markdownlint flags this
<img>for missing alternate text.As per static analysis hints, "Images should have alternate text (alt text)" (MD045, no-alt-text).♻️ Suggested fix
-<a href='https://github.com/j03-dev/oxapy/#'><img src='https://img.shields.io/badge/version-0.10.0-%23b7410e'/></a> +<a href='https://github.com/j03-dev/oxapy/#'><img src='https://img.shields.io/badge/version-0.10.0-%23b7410e' alt='version'/></a>🤖 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 `@README.md` at line 12, Add descriptive alt text to the version badge img element in README.md, preserving its existing source, link, and displayed badge behavior.Source: Linters/SAST tools
🤖 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/lib.rs`:
- Around line 345-357: Update the documentation for the public wrap method to
remove obsolete catchers terminology, status-code catcher behavior, and any
catchers argument references. Document the current single response wrapper
parameter and its behavior so generated stubs and help reflect wrap’s actual
API.
In `@src/response.rs`:
- Around line 234-244: Resolve the inert CORS configuration exposed alongside
Response::call_wrapper: either propagate the configured CORS policy through
call_wrapper and apply its headers, or remove the CORS field, HttpServer::cors
method, public export, and generated documentation as one consistent API change.
Ensure no publicly configurable CORS path remains without corresponding
behavior.
In `@src/templating/mod.rs`:
- Around line 41-43: Update the is_safe method on the templating wrapper to
return false, matching Tera’s default escaping behavior. Ensure functions
registered through register_function() remain autoescaped unless an explicit
raw-output contract is intentionally introduced.
- Around line 26-39: Update PyTeraFunction::call to convert the Python callable
result through the existing JSON-to-Tera conversion path instead of wrapping
result.to_string() in Value::String. Preserve booleans, numbers, lists, and
dictionaries as their corresponding tera::Value types, propagating conversion
errors through tera::Error.
---
Outside diff comments:
In `@src/templating/mod.rs`:
- Around line 202-207: Remove or regenerate the stale Tera declaration from
oxapy/templating/__init__.pyi, including its __new__ and render members, so the
stub matches templating_submodule’s registered Template class and render
function without exposing a nonexistent Tera class.
---
Nitpick comments:
In `@README.md`:
- Line 12: Add descriptive alt text to the version badge img element in
README.md, preserving its existing source, link, and displayed badge behavior.
In `@src/templating/mod.rs`:
- Around line 133-143: Update register_function to use the imported
PyRuntimeError type instead of the fully qualified pyo3::exceptions path, and
revise the error text to clearly state that the Template’s Tera engine is
already shared and functions must be registered before attaching the Template to
the server.
- Around line 114-143: Add Rust doc comments to the public pymethods render and
register_function, including Args, Returns, and Example sections. Document
render’s template/context inputs and rendered-string result, and document
register_function’s callable registration behavior plus its requirement to run
before the Template engine is shared, including the resulting error when that
requirement is violated.
- Around line 90-101: Update the documentation for Template::new to remove the
obsolete invalid-engine PyException description and Jinja-specific example,
including templating.Template() without arguments. Revise the example and
surrounding text to reflect the current dir-only constructor and supported
engine behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61cca9e3-2573-487d-8641-a650f0a25043
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlREADME.mdbuild.shoxapy/__init__.pysrc/catcher.rssrc/lib.rssrc/middleware.rssrc/request.rssrc/response.rssrc/templating/minijinja.rssrc/templating/mod.rssrc/templating/tera.rs
💤 Files with no reviewable changes (3)
- src/templating/minijinja.rs
- src/catcher.rs
- src/templating/tera.rs
| /// def global_middleware(request, response): | ||
| /// if response.status.code == 200: | ||
| /// return Response("<h1>Page Not Found</h1>", content_type="text/html") | ||
| /// return response | ||
| /// | ||
| /// server.catchers([not_found]) | ||
| /// server.wrap(global_middleware) | ||
| /// ``` | ||
| fn catchers<'py>( | ||
| fn wrap<'py>( | ||
| mut slf: PyRefMut<'py, Self>, | ||
| catchers: Vec<PyRef<Catcher>>, | ||
| wrapper: Py<PyAny>, | ||
| py: Python<'py>, | ||
| ) -> PyRefMut<'py, Self> { | ||
| let map = catchers | ||
| .into_iter() | ||
| .map(|c| (c.status, c.handler.clone_ref(py))) | ||
| .collect(); | ||
| slf.catchers = Some(Arc::new(map)); | ||
| slf.wrapper = Some(Arc::new(wrapper)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the obsolete catchers documentation.
This public method still documents status-code catchers and a catchers argument, while it now accepts one response wrapper. Generated stubs/help will expose the wrong API.
🤖 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/lib.rs` around lines 345 - 357, Update the documentation for the public
wrap method to remove obsolete catchers terminology, status-code catcher
behavior, and any catchers argument references. Document the current single
response wrapper parameter and its behavior so generated stubs and help reflect
wrap’s actual API.
| pub(crate) fn call_wrapper(mut self, req: &ProcessRequest) -> Self { | ||
| if let Some(wrapper) = &req.wrapper { | ||
| let request = req.request.as_ref().clone(); | ||
| self = Python::attach(|py| { | ||
| let result = handler.call(py, (request, self), None)?; | ||
| let result = wrapper.call(py, (request, self), None)?; | ||
| convert_to_response(result, py) | ||
| }) | ||
| .unwrap_or_else(Response::from); | ||
| } | ||
| self | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove or restore the now-inert CORS API.
call_wrapper no longer applies CORS, while HttpServer::cors(...) remains publicly configurable. Existing users can configure CORS successfully but receive no CORS headers. Either thread CORS through this flow or remove the field, method, export, and generated documentation together.
🤖 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/response.rs` around lines 234 - 244, Resolve the inert CORS configuration
exposed alongside Response::call_wrapper: either propagate the configured CORS
policy through call_wrapper and apply its headers, or remove the CORS field,
HttpServer::cors method, public export, and generated documentation as one
consistent API change. Ensure no publicly configurable CORS path remains without
corresponding behavior.
| fn is_safe(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate templating mod and relevant files =="
git ls-files | rg '(^|/)src/templating/mod\.rs$|Cargo\.toml|pyproject\.toml|oxapy/templating/__init__\.pyi$' || true
echo
echo "== src/templating/mod.rs outline =="
ast-grep outline src/templating/mod.rs || true
echo
echo "== relevant lines in src/templating/mod.rs =="
cat -n src/templating/mod.rs | sed -n '1,160p'
echo
echo "== dependency version declarations for tera =="
rg -n 'tera|minijinja|pyo3' Cargo.toml pyproject.toml -S || trueRepository: j03-dev/oxapy
Length of output: 7108
🌐 Web query:
Tera Rust Function trait is_safe default return value documentation
💡 Result:
In the Tera template engine for Rust, the is_safe method provided by the Function trait (and similarly by the Filter trait) has a default return value of false [1][2]. This method is used to determine whether the output of a custom function or filter should be treated as safe content [1][2]. When is_safe returns false (the default), Tera will automatically escape the output to prevent injection attacks, consistent with its default behavior of escaping content [1][2][3]. If a developer intends for their function or filter to return raw, safe content that should not be escaped, they must explicitly override is_safe to return true [1][4][2].
Citations:
- 1: https://docs.rs/tera/latest/tera/trait.Function.html
- 2: https://docs.rs/tera/latest/tera/trait.Filter.html
- 3: https://docs.rs/tera/latest/tera/
- 4: Enabled filters & functions to be safe Keats/tera#545
Default is_safe() to Tera’s normal escaping behavior.
tera::Function::is_safe() defaults to false, but this wrapper always returns true, so every Python function registered through register_function() can bypass autoescaping in HTML/XML templates. Return false unless a specific raw-output contract is documented and intentional.
🤖 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/templating/mod.rs` around lines 41 - 43, Update the is_safe method on the
templating wrapper to return false, matching Tera’s default escaping behavior.
Ensure functions registered through register_function() remain autoescaped
unless an explicit raw-output contract is intentionally introduced.
1 participant
Summary by CodeRabbit
New Features
wrap) to customize request/response handling end-to-end.Changes
Release