test: expand backend API contract edge-case tests#14
Conversation
- Add backend/api_contract.py with Python validation helpers mirroring the Rust protocol validation logic - Add tests/backend_api/ with 125 edge-case tests covering: - Malformed/missing required field error contracts - Numeric boundary conditions (quantity, price, amount) - String/pattern validation edge cases - Enum validation with case sensitivity - Message envelope version/payload/priority/flags validation - Frame version and payload size validation - RPC error code contracts - Async helper execution without optional pytest plugins - Multi-field compound error aggregation - Type coercion edge cases - Include diagnostic/build-00000000.logd artifact from build.py Closes 9904099#1
|
Warning Review limit reached
More reviews will be available in 55 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR adds backend API contract validation types and helpers for payload, envelope, frame, and RPC error handling, plus isolated pytest coverage for edge cases. It also updates a diagnostic build JSON with refreshed metadata. ChangesBackend API contract and edge-case tests
Diagnostic metadata refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 (2)
tests/backend_api/test_api_contract.py (2)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sys.path.insert(0, ".")is fragile and likely redundant.This depends on pytest being invoked from the repo root, and
conftest.pyalready importsbackend.api_contractat collection time (before this module body runs), so if the package weren't importable, collection would fail first. Prefer makingbackendimportable via project config (e.g.pyproject.toml/pytest.inirootdir/pythonpath) rather than a cwd-dependent path hack.🤖 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 `@tests/backend_api/test_api_contract.py` at line 25, Remove the cwd-dependent sys.path.insert(0, ".") from test_api_contract and make backend importable through project/test configuration instead, since conftest.py already imports backend.api_contract during collection and this path hack is redundant; adjust the test setup via pytest configuration or package/import settings so backend resolves without relying on the current working directory.
593-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test_concurrent_validationsis not actually concurrent.The
concurrent()coroutine builds a list ofValidationResultsynchronously (the validators aren't coroutines, and there's noasyncio.gather). The name/intent suggests concurrency coverage that isn't exercised. Consider renaming, or wrapping each validation in a task viaasyncio.gatherif concurrent execution is the goal.🤖 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 `@tests/backend_api/test_api_contract.py` around lines 593 - 602, The test named test_concurrent_validations is misleading because concurrent() only builds a list synchronously and never runs validate_order_payload concurrently. Update the test to either rename it to reflect sequential validation or, if concurrency is intended, make the validations actual async tasks using asyncio.gather around validate_order_payload so the test exercises concurrent execution.
🤖 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 `@backend/api_contract.py`:
- Around line 252-267: `validate_price` in `backend/api_contract.py` does not
match the Rust contract: it should keep the positive check but also enforce the
same upper bound of less than 1_000_000_000.0 and the same fractional-precision
validation. Update `validate_price` to mirror the Rust helper’s behavior, while
leaving `validate_quantity` unchanged since it already matches and the
1_000_000.0 cap belongs in `validate_order_payload`.
- Around line 186-192: The strict validators are currently vulnerable to
trailing-newline matches because they rely on regex patterns with only ^ and $;
update validate_email, validate_uuid, validate_symbol, and
validate_instrument_id to use an exact-end anchor like \Z (or an equivalent
strict end match) while leaving validate_pattern() as a search helper. Do not
change validate_pattern() in backend/api_contract.py to fullmatch(), since it is
intentionally used for substring-style checks elsewhere, including the
validate_pattern usage covered in the API contract tests.
---
Nitpick comments:
In `@tests/backend_api/test_api_contract.py`:
- Line 25: Remove the cwd-dependent sys.path.insert(0, ".") from
test_api_contract and make backend importable through project/test configuration
instead, since conftest.py already imports backend.api_contract during
collection and this path hack is redundant; adjust the test setup via pytest
configuration or package/import settings so backend resolves without relying on
the current working directory.
- Around line 593-602: The test named test_concurrent_validations is misleading
because concurrent() only builds a list synchronously and never runs
validate_order_payload concurrently. Update the test to either rename it to
reflect sequential validation or, if concurrency is intended, make the
validations actual async tasks using asyncio.gather around
validate_order_payload so the test exercises concurrent execution.
🪄 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
Run ID: bec29fb2-d88a-4566-80e7-a8ca343859d9
📒 Files selected for processing (8)
backend/__init__.pybackend/api_contract.pydiagnostic/build-00000000.jsondiagnostic/build-00000000.logdtests/__init__.pytests/backend_api/__init__.pytests/backend_api/conftest.pytests/backend_api/test_api_contract.py
| def validate_price(price: float, field_name: str = "price") -> ValidationResult: | ||
| result = ValidationResult.ok() | ||
| if price <= 0.0: | ||
| result.add_error(field_name, "invalid_price", "Price must be positive") | ||
| elif price >= 1_000_000_000.0: | ||
| result.add_error(field_name, "max_exceeded", "Price exceeds maximum") | ||
| return result | ||
|
|
||
|
|
||
| def validate_quantity(qty: float, field_name: str = "quantity") -> ValidationResult: | ||
| result = ValidationResult.ok() | ||
| if qty <= 0.0: | ||
| result.add_error(field_name, "invalid_quantity", "Quantity must be positive") | ||
| elif qty >= 100_000_000.0: | ||
| result.add_error(field_name, "max_exceeded", "Quantity exceeds maximum") | ||
| return result |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'fn validate_(price|quantity)\b' backend/src 2>/dev/null
rg -nP '1000000\.0|100000000\.0|1_000_000|100_000_000' backend/src 2>/dev/nullRepository: jackjin1997/zeroeye
Length of output: 790
🏁 Script executed:
#!/bin/bash
set -e
sed -n '230,280p' backend/api_contract.py
printf '\n--- Rust validate.rs excerpt ---\n'
sed -n '310,440p' backend/src/protocol/validate.rsRepository: jackjin1997/zeroeye
Length of output: 6508
🏁 Script executed:
sed -n '280,360p' backend/api_contract.pyRepository: jackjin1997/zeroeye
Length of output: 3388
🏁 Script executed:
sed -n '280,360p' backend/api_contract.pyRepository: jackjin1997/zeroeye
Length of output: 3388
🏁 Script executed:
sed -n '280,360p' backend/api_contract.pyRepository: jackjin1997/zeroeye
Length of output: 3388
🏁 Script executed:
sed -n '280,360p' backend/api_contract.pyRepository: jackjin1997/zeroeye
Length of output: 3388
🏁 Script executed:
rg -n "def validate_order_payload|validate_price|validate_quantity|max_exceeded|invalid_price|invalid_quantity" backend/api_contract.pyRepository: jackjin1997/zeroeye
Length of output: 1199
Align validate_price with the Rust contract in backend/api_contract.py.
validate_quantityalready matches the Rust helper; the1_000_000.0cap belongs tovalidate_order_payload.validate_pricestill differs from Rust: it should enforce< 1_000_000_000.0and the same fractional-precision check.
🤖 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 `@backend/api_contract.py` around lines 252 - 267, `validate_price` in
`backend/api_contract.py` does not match the Rust contract: it should keep the
positive check but also enforce the same upper bound of less than
1_000_000_000.0 and the same fractional-precision validation. Update
`validate_price` to mirror the Rust helper’s behavior, while leaving
`validate_quantity` unchanged since it already matches and the 1_000_000.0 cap
belongs in `validate_order_payload`.
|
Addressed CodeRabbit review: changed |
|
Addressed CodeRabbit docstring coverage warning: added docstrings to all 36 public functions and classes (coverage now 94.7%, up from 1.6%). This should resolve the pre-merge check failure. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/api_contract.py (2)
412-421: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the lower bounds for
priorityandflags.Negative values currently pass validation even though the contract and error text say these fields must be in
0-255and0-0xFFFF.Suggested fix
- if self.priority > 255: + if not 0 <= self.priority <= 255: result.add_error( "priority", "invalid_priority", f"Priority must be 0-255, got {self.priority}", ) - if self.flags > 0xFFFF: + if not 0 <= self.flags <= 0xFFFF: result.add_error( "flags", "invalid_flags", f"Flags must be 0-0xFFFF, got {self.flags}", )🤖 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 `@backend/api_contract.py` around lines 412 - 421, In api_contract.py, the validation in the contract object only checks the upper bounds for priority and flags, so negative values are incorrectly accepted; update the validation logic in the same block to reject values below 0 as well as above 255/0xFFFF, keeping the existing error reporting pattern via result.add_error and the relevant field checks for priority and flags.
277-294: 🎯 Functional Correctness | 🟠 MajorReject non-finite
priceandquantityvalues.Python's ordered comparisons treat
NaNas incomparable, causingvalidate_priceandvalidate_quantityto incorrectly acceptfloat("nan")as valid input. While infinities are currently rejected by the upper/lower bounds,NaNfalls through all checks.Add a
math.isfinite()guard at the start of both validators to ensure only finite numbers are processed.Suggested fix
def validate_price(price: float, field_name: str = "price") -> ValidationResult: """Validate price is positive and below maximum.""" result = ValidationResult.ok() + if not math.isfinite(price): + result.add_error(field_name, "invalid_price", "Price must be finite") if price <= 0.0: result.add_error(field_name, "invalid_price", "Price must be positive") elif price >= 1_000_000_000.0: result.add_error(field_name, "max_exceeded", "Price exceeds maximum") return result @@ def validate_quantity(qty: float, field_name: str = "quantity") -> ValidationResult: """Validate quantity is positive and below maximum.""" result = ValidationResult.ok() + if not math.isfinite(qty): + result.add_error(field_name, "invalid_quantity", "Quantity must be finite") if qty <= 0.0: result.add_error(field_name, "invalid_quantity", "Quantity must be positive") elif qty >= 100_000_000.0: result.add_error(field_name, "max_exceeded", "Quantity exceeds maximum") return result🤖 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 `@backend/api_contract.py` around lines 277 - 294, Add a finiteness check at the start of validate_price and validate_quantity in api_contract.py so non-finite inputs are rejected before the existing bounds checks; specifically, use math.isfinite() in both validators and add an appropriate validation error when the value is NaN or infinite, keeping the current positive/maximum rules unchanged for valid finite numbers.
🤖 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.
Outside diff comments:
In `@backend/api_contract.py`:
- Around line 412-421: In api_contract.py, the validation in the contract object
only checks the upper bounds for priority and flags, so negative values are
incorrectly accepted; update the validation logic in the same block to reject
values below 0 as well as above 255/0xFFFF, keeping the existing error reporting
pattern via result.add_error and the relevant field checks for priority and
flags.
- Around line 277-294: Add a finiteness check at the start of validate_price and
validate_quantity in api_contract.py so non-finite inputs are rejected before
the existing bounds checks; specifically, use math.isfinite() in both validators
and add an appropriate validation error when the value is NaN or infinite,
keeping the current positive/maximum rules unchanged for valid finite numbers.
…ice/validate_quantity
|
Addressed latest CodeRabbit review:
All 3 fixes applied and pushed. |
1 participant
Summary
Expand the backend API contract test suite with deterministic negative cases, async wrapper coverage, and boundary-condition tests. Closes #1.
Changes
backend/api_contract.py— New Python module mirroring the Rust validation logic inbackend/src/protocol/validate.rsandbackend/src/protocol/messages.rs. Provides reusable validators for orders, accounts, envelopes, frames, and RPC error codes.tests/backend_api/conftest.py— Shared pytest fixtures (valid order/account payloads, message envelopes).tests/backend_api/test_api_contract.py— 125 edge-case tests across 14 test classes.Testing
Test coverage (125 tests, all passing)
Commands run locally
Key design decisions
asyncio.run()directly, avoiding thepytest-asyncioplugin dependency.Checklist
python3 -m pytest -q tests/backend_api)diagnostic/build-00000000.logd+.json)Summary by CodeRabbit