edition2024 #85
edition2024 #85
Conversation
Signed-off-by: FITAHIANA Nomeniavo Joe <24nomeniavo@gmail.com>
Signed-off-by: FITAHIANA Nomeniavo Joe <24nomeniavo@gmail.com>
📝 WalkthroughWalkthroughUpdated Rust/Python bindings and runtime plumbing: Cargo edition bumped; serializer.context made non-optional; many PyO3 exceptions switched to a macro; CORS fields expanded; orjson loader made fallible via PyOnceLock; request/response/session plumbing adjusted; small Python test additions. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpServer
participant Router
participant Handler
participant Catcher
participant Cors
Client->>HttpServer: send HTTP request
HttpServer->>Router: resolve route, build ProcessRequest (includes cors, catchers, request_sender)
Router->>Handler: invoke handler (may use request_sender channel)
Handler-->>HttpServer: return Response (direct or via channel)
HttpServer->>Catcher: apply catcher if response status matches
HttpServer->>Cors: apply CORS headers
HttpServer->>Client: send final HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cors.rs (2)
55-77: Flag potential CORS security misconfiguration.The default configuration sets
allow_credentials = truewithorigins = ["*"]. This combination is a security risk and will actually be rejected by browsers. According to the CORS specification, when credentials are allowed, theAccess-Control-Allow-Originheader cannot be a wildcard.Consider either:
- Setting
allow_credentials = falseby default, or- Documenting that users must specify explicit origins when enabling credentials
🔒 Recommended fix
impl Default for Cors { fn default() -> Self { Self { origins: vec!["*".to_string()], methods: vec![ "DELETE".to_string(), "GET".to_string(), "OPTIONS".to_string(), "PATCH".to_string(), "POST".to_string(), "PUT".to_string(), ], headers: vec![ "Accept".to_string(), "Authorization".to_string(), "Content-Type".to_string(), "X-Requested-With".to_string(), ], - allow_credentials: true, + allow_credentials: false, max_age: 86400, } } }
107-115: Incorrect handling of multiple origins in CORS headers.Line 108 uses
origins.join(", ")for theAccess-Control-Allow-Originheader. According to the CORS specification, this header must contain either a single origin or the wildcard"*", not a comma-separated list of multiple origins.The proper pattern for multiple origins is to:
- Check the request's
Originheader- If it matches one of the allowed origins, echo that single origin back
- Otherwise, omit the header or use the first allowed origin
♻️ Suggested approach
The
apply_headersmethod should accept the request'sOriginheader and match it against the allowed origins list:pub fn apply_headers(&self, response: &mut Response, request_origin: Option<&str>) { if self.origins.contains(&"*".to_string()) { response.insert_header("Access-Control-Allow-Origin", "*".to_string()); } else if let Some(origin) = request_origin { if self.origins.contains(&origin.to_string()) { response.insert_header("Access-Control-Allow-Origin", origin.to_string()); } } // ... rest of headers }
🤖 Fix all issues with AI agents
In @oxapy/__init__.py:
- Around line 41-42: The error message raised by exceptions.NotFoundError
contains a non-breaking space (U+00A0) in the f-string "The file at {path} is
not found."; replace that invisible NBSP with a normal ASCII space so the raised
message becomes "The file at {path} is not found." while keeping the same
f-string and use of exceptions.NotFoundError.
In @oxapy/serializer/__init__.pyi:
- Around line 270-272: Document the breaking change that the Serializer.context
property type was changed from Optional[dict] to dict (i.e., it will never be
None) by updating the release notes/changelog and migration guide; mention that
this mirrors the Rust implementation in src/serializer/mod.rs which converts
None to an empty dict (context.unwrap_or_else(|| PyDict::new(py).into())),
include examples of affected checks like "if serializer.context is None" and
recommend replacing them with truthiness checks or explicit emptiness checks
(e.g., if not serializer.context or if serializer.context == {}), and add a
short note in the Python API section referencing the context property and the
rationale for the change.
In @src/json.rs:
- Around line 5-7: The static ORJSON currently uses Lazy::new with
Python::attach and PyModule::import(...).unwrap(), which risks deadlock and
panics on missing orjson; replace the Lazy::new initialization with a
pyo3::sync::GILOnceCell (or PyOnceLock if available) static and perform the
import inside a GIL-safe get_or_init/set call (using Python::with_gil) so
initialization coordinates with the GIL, and replace the .unwrap() with explicit
error handling (propagate a Result, log the error, or return a fallback) when
calling PyModule::import to avoid panics.
In @src/response.rs:
- Around line 194-197: append_header currently unwraps HeaderName::from_str and
value.parse which will panic on invalid user input; change append_header to
handle errors instead of unwrapping by returning a Result<(), E> (or Option) and
propagate parsing errors from HeaderName::from_str(key) and value.parse() back
to the caller, e.g., map or ? the errors and only call self.headers.append(...)
on success so invalid header names/values do not cause a panic.
🧹 Nitpick comments (1)
tests/app.py (1)
1-9: Test code binding to all interfaces - acceptable for development/testing.The static analysis tool flags binding to
0.0.0.0, which exposes the server on all network interfaces. While this is a security concern for production code, it's acceptable for test scripts. Just ensure this test file is not used as a template for production deployments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Cargo.tomloxapy/__init__.pyoxapy/serializer/__init__.pyisrc/cors.rssrc/exceptions.rssrc/into_response.rssrc/json.rssrc/jwt.rssrc/lib.rssrc/request.rssrc/response.rssrc/serializer/mod.rssrc/session.rssrc/templating/minijinja.rstests/app.pytests/test_bench.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-22T14:44:51.700Z
Learnt from: j03-dev
Repo: j03-dev/oxapy PR: 82
File: src/routing.rs:412-417
Timestamp: 2025-11-22T14:44:51.700Z
Learning: In oxapy's Router (src/routing.rs), the `middleware()` method implements a layered architecture: it adds the middleware to the current layer, then creates a new empty layer via `self.layers.push(Layer::default())`. This is intentional - subsequent routes registered after the middleware call go into the new layer without inheriting the middleware from the previous layer. Each layer maintains its own routes and middlewares independently. When a request is processed, the system searches through layers to find a matching route and applies only that layer's middlewares.
Applied to files:
src/request.rs
🧬 Code graph analysis (10)
tests/test_bench.py (2)
src/lib.rs (1)
oxapy(655-685)oxapy/__init__.pyi (1)
Response(844-964)
src/session.rs (2)
oxapy/__init__.pyi (2)
id(1203-1203)session(775-799)src/request.rs (1)
session(215-222)
src/jwt.rs (2)
src/exceptions.rs (1)
exceptions(129-139)oxapy/jwt/__init__.pyi (1)
JwtError(112-116)
src/serializer/mod.rs (2)
src/exceptions.rs (1)
exceptions(129-139)src/serializer/fields.rs (1)
new(79-107)
src/into_response.rs (2)
oxapy/__init__.pyi (5)
status(869-869)status(871-871)cors(495-511)Cors(56-151)Status(1459-1758)src/lib.rs (1)
cors(292-295)
oxapy/__init__.py (4)
src/lib.rs (1)
send_file(649-652)oxapy/__init__.pyi (1)
send_file(2088-2088)src/exceptions.rs (1)
exceptions(129-139)oxapy/exceptions/__init__.pyi (1)
NotFoundError(74-84)
src/exceptions.rs (1)
oxapy/exceptions/__init__.pyi (2)
ClientError(20-31)InternalError(61-71)
src/templating/minijinja.rs (2)
src/request.rs (1)
json(126-132)oxapy/__init__.pyi (1)
json(815-838)
src/response.rs (1)
src/into_response.rs (1)
convert_to_response(175-186)
src/request.rs (1)
src/into_response.rs (3)
from(68-76)from(80-105)from(109-113)
🪛 Ruff (0.14.10)
oxapy/__init__.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
42-42: exceptions may be undefined, or defined from star imports
(F405)
42-42: String contains ambiguous (NO-BREAK SPACE). Did you mean (SPACE)?
(RUF001)
tests/app.py
9-9: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: macos (macos-14, aarch64)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: macos (macos-14, x86_64)
🔇 Additional comments (25)
src/templating/minijinja.rs (1)
8-9: LGTM!Import reordering is a cosmetic change with no functional impact.
src/response.rs (1)
239-250: Nice use of let-chains (Rust 2024 feature).The refactored
apply_catcherusing let-chains is cleaner than nested if-let blocks. This is a good use of the new edition features.src/jwt.rs (1)
17-45: LGTM! Clean macro-based exception hierarchy.The refactoring to use
extend_exception!macro eliminates repetitive boilerplate. The pattern is consistent:
JwtErroras base (single-arg macro)- Child exceptions extending
JwtError(two-arg macro)This aligns with the exception wiring patterns in
src/exceptions.rs.src/exceptions.rs (2)
15-41: Well-designed macro for exception boilerplate.The
extend_exception!macro cleanly handles both base and derived exception patterns:
- Single-arg: For base exceptions that directly wrap
Py<PyAny>- Two-arg: For subclass exceptions using PyO3's tuple return pattern
This significantly reduces code duplication across the exception hierarchy.
15-27: No action needed. Theextend_exceptionmacro visibility is properly configured via#[macro_use]on themod exceptions;declaration insrc/lib.rs(line 4). This is the correct approach for making macros available across modules within a crate and does not require#[macro_export].Likely an incorrect or invalid review comment.
tests/test_bench.py (1)
2-2: LGTM!Removing the unused
serializerimport is appropriate cleanup aligned with the API surface changes in this PR.Cargo.toml (1)
4-4: Edition 2024 upgrade is compatible.The edition bump to 2024 requires Rust 1.85.0 or later. Verify that your CI and development environments are configured with the appropriate toolchain version.
Likely an incorrect or invalid review comment.
src/into_response.rs (2)
99-99: LGTM! More explicit Response construction.The change from
status.into()toResponse::from(status)makes the code more explicit and easier to understand.
108-114:apply_headersmethod exists and implementation is correct.The
Cors::apply_headersmethod is properly defined insrc/cors.rsas a public method that inserts the appropriate CORS headers into the response. The parameter rename fromvaltocorsimproves clarity.src/session.rs (2)
7-9: Minor import reordering for consistency.The imports have been reordered slightly. This is a stylistic change with no functional impact.
398-407: LGTM! Rust 2024 let-chain syntax improves readability.The control flow has been simplified using Rust 2024's let-chain syntax (
if let Some(id) = session_id && let Some(session) = sessions.get(id)). This is more concise than nested if-let statements while maintaining the same functionality.oxapy/__init__.py (2)
3-3: LGTM! Import added to support file existence check.The
osmodule is imported to support theos.path.exists()check insend_file().
24-29: LGTM! Simplified error handling.The try/except block has been removed, deferring error handling to the
send_file()function. This centralizes file existence checking and makes the code cleaner.src/lib.rs (3)
21-32: LGTM! Imports are well-organized.The new imports support the expanded functionality (atomic shutdown signal, PyO3 types, async runtimes, Tokio networking, and channels). All are used appropriately throughout the file.
49-56: LGTM! ProcessRequest struct enhancements are appropriate.The addition of
catchersandcorsfields enables per-request error handling and CORS configuration, and wrappingrequestinArcsupports efficient shared ownership across async tasks.
58-68: LGTM! RequestContext correctly integrates the request sender.Adding
request_senderenables the channel-based request processing architecture. The field is properly initialized increate_request_context(line 458) and used insend_and_wait_response(line 333).src/cors.rs (1)
38-53: LGTM! CORS field additions are well-designed.The new fields (
methods,headers,allow_credentials,max_age) are properly documented and use appropriate types. UsingVec<String>for methods and headers provides better flexibility than combined strings.src/request.rs (3)
18-18: LGTM! Import additions are necessary and correctly used.The newly imported types (
IntoPyException,ProcessRequest,RequestContext) are all used within the file for request processing and error handling.
215-222: LGTM! Session getter refactored to be more idiomatic.The refactor using
ok_or_elsewith an inline closure is cleaner and more idiomatic Rust. The error message remains clear and helpful.
328-339: LGTM! Refactored to use Edition 2024 let-chains feature.The refactor combines sending and receiving into a single chained condition using the
let-chains pattern from Rust 2024. This makes the code more concise while maintaining the same logic.src/serializer/mod.rs (5)
9-19: LGTM! Import reorganization improves clarity.The import changes consolidate related items and are all used appropriately throughout the file.
34-34: LGTM! Context field simplified to non-optional.Changing
contextfromOption<Py<PyDict>>toPy<PyDict>with a default empty dict simplifies the API. This aligns with the PR objective and eliminates the need to handleNonecases throughout the codebase.Also applies to: 98-98
207-213: LGTM! Validate logic refactored with let-chains.The refactor uses Rust 2024's let-chains to make the read-only field filtering more concise. The logic correctly removes read-only fields from validated input data.
410-431: LGTM! to_representation refactored consistently with let-chains.Both the columns and relationships processing use let-chains to filter out
write_onlyfields during serialization. The context propagation to nested serializers (lines 427-428) is correctly implemented.
524-524: LGTM! Exception macro simplified.The change from separate boilerplate and extension macros to a single
extend_exception!macro aligns with the PR objective to improve exception macros. TheValidationExceptionis correctly defined as extendingClientError.
| def context(self) -> dict: ... | ||
| @context.setter | ||
| def context(self, value: typing.Optional[dict]) -> None: ... | ||
| def context(self, value: dict) -> None: ... |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat oxapy/serializer/__init__.pyi | head -n 280 | tail -n 20Repository: j03-dev/oxapy
Length of output: 830
🏁 Script executed:
# Find documentation files mentioning context or breaking changes
find . -type f \( -name "*.md" -o -name "*.rst" -o -name "CHANGELOG*" -o -name "RELEASE*" \) | head -20Repository: j03-dev/oxapy
Length of output: 68
🏁 Script executed:
# Search for migration guides or documentation about this change
rg -l "context" --type=md --type=rst | head -10Repository: j03-dev/oxapy
Length of output: 39
🏁 Script executed:
# Look for examples using the context property
fd -e py | xargs rg -l "\.context" --type=py | head -10Repository: j03-dev/oxapy
Length of output: 39
🏁 Script executed:
cat README.mdRepository: j03-dev/oxapy
Length of output: 5574
🏁 Script executed:
# Find the actual Python source file for the serializer
find . -path "./oxapy/serializer/*.py" -not -path "*/.*"Repository: j03-dev/oxapy
Length of output: 39
🏁 Script executed:
# Check if there's a git history we can examine
git log --oneline oxapy/serializer/__init__.pyi 2>/dev/null | head -10Repository: j03-dev/oxapy
Length of output: 85
🏁 Script executed:
# Look for version information or changelog
rg -l "version|changelog|breaking|release" --type=md --type=txt | head -10Repository: j03-dev/oxapy
Length of output: 66
🏁 Script executed:
# Find the actual Python source file for serializer
find . -name "*.py" -path "*/serializer/*" -type fRepository: j03-dev/oxapy
Length of output: 39
🏁 Script executed:
# Check setup.py or pyproject.toml for changelog/version info
find . -maxdepth 2 \( -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" \) -type fRepository: j03-dev/oxapy
Length of output: 73
🏁 Script executed:
# Search for any docstrings or comments about context in Python files
rg -C5 "def context" --type=pyRepository: j03-dev/oxapy
Length of output: 39
🏁 Script executed:
cat pyproject.tomlRepository: j03-dev/oxapy
Length of output: 637
🏁 Script executed:
# Search for any mention of breaking changes, context, or migration in any file type
rg -i "breaking|context|migration" --max-count=20Repository: j03-dev/oxapy
Length of output: 3772
🏁 Script executed:
# Look for any documentation files in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "docs" -type d \) | head -20Repository: j03-dev/oxapy
Length of output: 68
The type annotation change is intentional and correctly reflects the Rust implementation, but document this breaking change in release notes.
The context property type change from Optional[dict] to dict is backed by the Rust implementation in src/serializer/mod.rs, which explicitly converts None to an empty dict via context.unwrap_or_else(|| PyDict::new(py).into()).
However, this creates a breaking change for code that checks if serializer.context is None, which will now always be False. While code that treats context as a dict continues to work, the absence of migration documentation, release notes, or changelog entry leaves users unaware of this behavioral shift. Add these details to the next release documentation.
🤖 Prompt for AI Agents
In @oxapy/serializer/__init__.pyi around lines 270 - 272, Document the breaking
change that the Serializer.context property type was changed from Optional[dict]
to dict (i.e., it will never be None) by updating the release notes/changelog
and migration guide; mention that this mirrors the Rust implementation in
src/serializer/mod.rs which converts None to an empty dict
(context.unwrap_or_else(|| PyDict::new(py).into())), include examples of
affected checks like "if serializer.context is None" and recommend replacing
them with truthiness checks or explicit emptiness checks (e.g., if not
serializer.context or if serializer.context == {}), and add a short note in the
Python API section referencing the context property and the rationale for the
change.
| pub fn insert_header(&mut self, key: &str, value: String) { | ||
| self.headers.insert( | ||
| HeaderName::from_bytes(key.as_bytes()).unwrap(), | ||
| value.parse().unwrap(), | ||
| ); | ||
| self.headers | ||
| .insert(HeaderName::from_str(key).unwrap(), value.parse().unwrap()); | ||
| } |
There was a problem hiding this comment.
Potential panic on invalid header names.
HeaderName::from_str(key).unwrap() will panic if key contains invalid characters (non-ASCII, control characters) or is empty. Since this method is exposed to Python users, invalid input could crash the application.
Consider returning a PyResult with proper error handling:
🔧 Proposed fix
- pub fn insert_header(&mut self, key: &str, value: String) {
- self.headers
- .insert(HeaderName::from_str(key).unwrap(), value.parse().unwrap());
+ pub fn insert_header(&mut self, key: &str, value: String) -> PyResult<()> {
+ self.headers.insert(
+ HeaderName::from_str(key).into_py_exception()?,
+ value.parse().into_py_exception()?,
+ );
+ Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn insert_header(&mut self, key: &str, value: String) { | |
| self.headers.insert( | |
| HeaderName::from_bytes(key.as_bytes()).unwrap(), | |
| value.parse().unwrap(), | |
| ); | |
| self.headers | |
| .insert(HeaderName::from_str(key).unwrap(), value.parse().unwrap()); | |
| } | |
| pub fn insert_header(&mut self, key: &str, value: String) -> PyResult<()> { | |
| let header_name = HeaderName::from_str(key) | |
| .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; | |
| let header_value = value.parse() | |
| .map_err(|e: http::header::ToStrError| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; | |
| self.headers.insert(header_name, header_value); | |
| Ok(()) | |
| } |
| pub fn append_header(&mut self, key: &str, value: String) { | ||
| self.headers.append( | ||
| HeaderName::from_bytes(key.as_bytes()).unwrap(), | ||
| value.parse().unwrap(), | ||
| ); | ||
| self.headers | ||
| .append(HeaderName::from_str(key).unwrap(), value.parse().unwrap()); | ||
| } |
There was a problem hiding this comment.
Same panic risk in append_header.
The same issue applies here with .unwrap() on potentially invalid header name/value from user input.
🔧 Proposed fix
- pub fn append_header(&mut self, key: &str, value: String) {
- self.headers
- .append(HeaderName::from_str(key).unwrap(), value.parse().unwrap());
+ pub fn append_header(&mut self, key: &str, value: String) -> PyResult<()> {
+ self.headers.append(
+ HeaderName::from_str(key).into_py_exception()?,
+ value.parse().into_py_exception()?,
+ );
+ Ok(())
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/response.rs around lines 194 - 197, append_header currently unwraps
HeaderName::from_str and value.parse which will panic on invalid user input;
change append_header to handle errors instead of unwrapping by returning a
Result<(), E> (or Option) and propagate parsing errors from
HeaderName::from_str(key) and value.parse() back to the caller, e.g., map or ?
the errors and only call self.headers.append(...) on success so invalid header
names/values do not cause a panic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/serializer/mod.rs:
- Around line 409-413: The condition uses field.extract::<Field>()? which will
propagate errors into to_representation; instead, first call slf.getattr(&col)
and then attempt to extract the Field in a non-fallible way (e.g. match or if
let Ok(f) = field.extract::<Field>() ) so non-Field attributes are skipped; only
when extraction succeeds and f.write_only is false call dict.set_item with
instance.getattr(&col)?, updating the block around slf.getattr(&col) /
field.extract::<Field>() to avoid using ? inside the conditional.
- Around line 423-430: The relationships loop currently calls
field.extract::<Field>()? inline which will return an error if the attribute
exists but isn't a Field; change the logic to attempt extraction first and skip
the key on Err (i.e., extract into a temporary like extracted_field and continue
when extraction fails), then check extracted_field.write_only before performing
the same context/instance setattr chain and dict.set_item calls; keep using
slf.getattr(&key), field.setattr("context"), field.setattr("instance"),
instance.getattr(&key), and field.getattr("data")? as in the columns loop for
consistent behavior.
- Line 35: Change the struct field "context" from Option<Py<PyDict>> to
Py<PyDict> and keep the constructor parameter as Option<Py<PyDict>> (the
constructor function that takes context should call unwrap_or_else to supply an
empty dict when None); update the initializer in the impl/new function to use
context.unwrap_or_else(|| PyDict::new(py).into()) (or equivalent with
Py::new/PyDict::new) so the field is always a dict, and remove any downstream
None checks that assumed context could be absent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/json.rssrc/serializer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/json.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: macos (macos-14, x86_64)
- GitHub Check: macos (macos-14, aarch64)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
🔇 Additional comments (4)
src/serializer/mod.rs (4)
11-22: LGTM on the lazy initialization pattern split.The hybrid approach is appropriate:
PyOnceLockforSQL_ALCHEMY_INSPECT(holdsPy<PyAny>requiring GIL-aware initialization) while retainingonce_cell::sync::LazyforCACHES_JSON_SCHEMA_VALUE(pure RustHashMap<String, Value>with no Python objects).
208-213: Clean use of Rust 2024 let-chains.The chained
if letwith&&consolidates the field extraction and read-only check into a single readable expression.
397-401: LGTM on the PyOnceLock initialization.The
get_or_try_initpattern correctly handles fallible lazy initialization for the SQLAlchemy inspect function with proper error propagation.
520-523: LGTM on the simplified exception pattern.Using
extend_exception!alone (removing the separate boilerplate macro) aligns with the PR's macro improvements. The exception properly extendsClientError.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @oxapy/__init__.py:
- Around line 24-27: The handler currently concatenates the URL path into
file_path allowing path traversal; fix by normalizing and validating the
requested path before calling send_file: use a safe resolver on the path (e.g.,
join directory with the requested path using a Path/OS-safe API, call resolve()
or equivalent), reject absolute or parent-traversal attempts by ensuring the
resolved path starts with the resolved directory root, and return an error/404
if validation fails; keep using the existing handler and send_file but only pass
validated, inside-directory paths.
🧹 Nitpick comments (1)
oxapy/__init__.py (1)
41-42: Consider simplifying the error message.The error message could be more concise, and the static analysis tool suggests avoiding f-strings in exception calls for better consistency.
♻️ Optional refactor
if not os.path.exists(path): - raise exceptions.NotFoundError(f"The file at {path} is not found.") + raise exceptions.NotFoundError(f"File not found: {path}")Or, if you prefer to address the TRY003 hint more strictly, define the message template separately:
+FILE_NOT_FOUND_MSG = "File not found: {}" + def send_file(path: str) -> Response: # ... if not os.path.exists(path): - raise exceptions.NotFoundError(f"The file at {path} is not found.") + raise exceptions.NotFoundError(FILE_NOT_FOUND_MSG.format(path))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxapy/__init__.py
🧰 Additional context used
🪛 Ruff (0.14.10)
oxapy/__init__.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
42-42: exceptions may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: macos (macos-14, x86_64)
🔇 Additional comments (1)
oxapy/__init__.py (1)
3-3: LGTM!The
osimport is correctly added to support the file existence check insend_file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/serializer/mod.rs (1)
439-443: Consider cache eviction for dynamic serializer classes.The schema cache grows indefinitely. For typical static serializer class definitions this is fine, but if serializers are dynamically generated at runtime, this could lead to unbounded memory growth. Consider documenting this behavior or adding a cache-clearing mechanism if dynamic class creation is a supported use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlsrc/serializer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/serializer/mod.rs (2)
src/serializer/fields.rs (1)
new(79-107)oxapy/serializer/__init__.pyi (2)
Field(134-212)Serializer(256-487)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: macos (macos-14, x86_64)
- GitHub Check: macos (macos-14, aarch64)
🔇 Additional comments (8)
src/serializer/mod.rs (8)
21-21: Good migration to PyOnceLock for GIL-aware lazy initialization.Using
PyOnceLockinstead ofOnceCellis the correct approach for storing Python objects in statics, as it ensures proper GIL handling.
34-34: Breaking API change:contextis now always a dict.This is a breaking change for consumers who may have relied on
contextbeingNone. The Python constructor still acceptsOptional[dict], but the property will always return a non-None dict. Ensure this is documented in release notes.Also applies to: 98-98
131-134: LGTM!The
pyparameter is correctly threaded through for cache access.
211-216: Good use of Rust 2024 let-chains for cleaner control flow.The chained
if let ... && ...syntax replaces nested pattern matching with a more readable single conditional block.
400-404: LGTM!Proper use of
get_or_try_initfor fallible lazy initialization of the SQLAlchemy inspector.
412-433: LGTM!The
write_onlyfield filtering correctly excludes fields from serialized output. Context propagation to nested serializers is properly handled.
456-463: Benign race condition in cache population.Between the cache read and write, another thread could insert the same key. This is harmless since the computed schema would be identical, resulting only in redundant work. If this becomes a performance concern under high concurrency, consider using an entry API pattern.
Also applies to: 516-519
530-533: LGTM!The
extend_exception!macro consolidation aligns with the PR objective to improve exception macros.
Signed-off-by: FITAHIANA Nomeniavo Joe <24nomeniavo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @oxapy/__init__.py:
- Around line 41-42: The NotFoundError currently exposes the full filesystem
path in oxapy.__init__.py (the check that raises exceptions.NotFoundError(f"The
file at {path} is not found.")), which can leak server directory structure;
change the raised message to a generic client-facing string (e.g., "Requested
file not found") and move the detailed path information to a server-side log
call (use the module logger or existing logging facility to log the actual path
and context) so clients don't receive sensitive paths but developers can still
debug using logs.
- Around line 26-27: The handler currently uses file_path =
os.path.join(directory, path) and returns send_file(file_path), which still
allows path traversal; replace this with a safe resolve-and-validate flow:
construct a Path from directory and path (e.g., Path(directory) / path), call
.resolve() on the resulting path and on Path(directory).resolve(), then verify
the resolved file path is inside the resolved directory using os.path.commonpath
or Path.commonpath (compare resolved_dir and resolved_file) and reject requests
(raise/abort with 400/403) if the check fails or if the resolved path is not a
file; only then call send_file on the validated resolved path. Ensure absolute
inputs, symlink escapes, and traversal sequences are all handled by using
Path.resolve() and the commonpath check.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxapy/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
oxapy/__init__.py (3)
oxapy/__init__.pyi (1)
send_file(2088-2088)src/exceptions.rs (1)
exceptions(129-139)oxapy/exceptions/__init__.pyi (1)
NotFoundError(74-84)
🪛 Ruff (0.14.10)
oxapy/__init__.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
42-42: exceptions may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: macos (macos-14, x86_64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: macos (macos-14, aarch64)
🔇 Additional comments (1)
oxapy/__init__.py (1)
3-3: Good addition to support path operations.The
osimport is necessary for the file path handling improvements below.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @oxapy/__init__.py:
- Around line 11-12: The current access check rejects the base directory itself
because it only allows paths that start with base + os.sep; update the check in
the access validation (the variables target, base and the
exceptions.ForbiddenError raise) to allow target equal to base as well as
descendants: normalize both target and base with os.path.normpath and then
change the condition to only raise if target is neither equal to base nor
startswith base + os.sep, so accessing the base directory (e.g., empty user_path
or ".") is permitted.
- Around line 7-14: secure_join uses os.path.abspath which doesn't resolve
symlinks, allowing traversal via symlinks; update secure_join to resolve
symlinks by computing os.path.realpath (or Path.resolve(strict=False)) for both
base and the joined target, then enforce containment by checking that target is
either equal to base or startswith base + os.sep and raise
exceptions.ForbiddenError otherwise; reference the secure_join function and
replace the current abspath calls and containment check with realpath-based
resolution and the equality/or-startswith containment test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxapy/__init__.py
🧰 Additional context used
🪛 Ruff (0.14.10)
oxapy/__init__.py
12-12: Avoid specifying long messages outside the exception class
(TRY003)
12-12: exceptions may be undefined, or defined from star imports
(F405)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
52-52: exceptions may be undefined, or defined from star imports
(F405)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
55-55: exceptions may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: macos (macos-14, x86_64)
- GitHub Check: macos (macos-14, aarch64)
🔇 Additional comments (3)
oxapy/__init__.py (3)
3-3: LGTM: Required import for path operations.The
osimport is necessary for the newsecure_joinfunction's path validation logic.
36-37: LGTM: Proper delegation to secure path joining.The static file handler now correctly delegates path validation to
secure_join, improving security and code clarity. Ensure the issues insecure_joinare addressed for complete protection.
51-55: LGTM: Appropriate validation checks.The pre-checks properly distinguish between non-existent paths (404 Not Found) and directories (403 Forbidden), improving error handling and user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
oxapy/__init__.py (3)
7-14: Strong path traversal protection implemented.The implementation correctly prevents path traversal attacks:
- Uses
realpathto resolve symlinks and relative paths- Validates that the resolved target is within the base directory
- The condition
target != base and not target.startswith(base + os.sep)properly handles edge cases like prefix attacks📝 Consider adding a docstring
def secure_join(base: str, *paths: str) -> str: + """ + Safely join path components, ensuring the result stays within base directory. + + Args: + base: Base directory path + *paths: Path components to join + + Returns: + Resolved absolute path within base directory + + Raises: + ForbiddenError: If the resolved path escapes the base directory + """ base = os.path.realpath(base)
51-55: Enhanced file validation prevents directory access.The pre-checks correctly handle edge cases:
- Explicit
NotFoundErrorfor missing filesForbiddenErrorfor non-file paths (directories, special files)The check order is appropriate: existence first, then file type.
Note: Ruff flags TRY003 (long messages outside exception class) for lines 52 and 55. These messages are concise enough that this is a low-priority style suggestion.
1-1: Consider explicit imports for better clarity.The wildcard import on line 1 (
from .oxapy import *) triggers Ruff F405 warnings when usingexceptions.ForbiddenErrorandexceptions.NotFoundError(lines 12, 52, 55). While these exceptions are defined in the module, explicit imports improve code clarity and IDE support.♻️ Optional: make imports explicit
-from .oxapy import * +from .oxapy import ( + HttpServer, Router, Status, Response, Request, Cors, + Session, SessionStore, Redirect, FileStreaming, File, + get, post, delete, patch, put, head, options, + render, catcher, convert_to_response, templating, + serializer, exceptions, jwt +)This is a codebase-wide style consideration and can be addressed later if desired.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oxapy/__init__.py
🧰 Additional context used
🪛 Ruff (0.14.10)
oxapy/__init__.py
12-12: Avoid specifying long messages outside the exception class
(TRY003)
12-12: exceptions may be undefined, or defined from star imports
(F405)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
52-52: exceptions may be undefined, or defined from star imports
(F405)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
55-55: exceptions may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: macos (macos-14, aarch64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: macos (macos-14, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: windows (windows-latest, x64)
🔇 Additional comments (2)
oxapy/__init__.py (2)
3-3: LGTM!The
osimport is necessary for the new path validation and file checking functionality.
36-37: Security enhancement: path validation centralized.The refactoring to use
secure_joinensures consistent path traversal protection and simplifies the handler implementation.
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.