Check response status codes - #465
Conversation
|
Warning Review limit reached
Next review available in: 6 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 (26)
📝 WalkthroughWalkthroughAdds a global forward-compatibility mode and client activation method, then validates expected HTTP status codes across REST mutation APIs. Unexpected responses return bodies when compatibility mode is disabled and throw ChangesForward-compatible status validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiMethod
participant RedmineServer
participant Future
participant Exception
Client->>Future: enableFutureMode()
ApiMethod->>RedmineServer: send mutation request
RedmineServer-->>ApiMethod: response body and status code
ApiMethod->>Future: read compatibility state
Future-->>ApiMethod: enabled or disabled
ApiMethod->>Exception: throw UnexpectedResponseException for unexpected status when enabled
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #465 +/- ##
============================================
- Coverage 98.72% 98.47% -0.25%
- Complexity 783 891 +108
============================================
Files 29 30 +1
Lines 2275 2497 +222
============================================
+ Hits 2246 2459 +213
- Misses 29 38 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a31b81a to
a7be46f
Compare
a7be46f to
ad776dc
Compare
ad776dc to
dd287a2
Compare
f51aed2 to
e02ef15
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Redmine/Api/Issue.php (1)
227-269: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
create()still throws on unexpected status even with forward compatibility disabled, unlike siblingcreate()methods.The added guard is
if (!Future::isForwardCompatibilityEnabled() && $body === '') { return $body; }— otherwise it throws. This means a non-empty body (e.g. a 422 validation-error response, which is the most common failure case for issue creation) will throwUnexpectedResponseExceptioneven whenFutureis disabled, breaking the very BC guarantee this flag exists for. Compare withGroup::create()andProject::create()(same PR), which use the plain form:if ($statusCode !== 201) { if (!Future::isForwardCompatibilityEnabled()) { return $body; } throw UnexpectedResponseException::create($this->lastResponse); }Every other mutation method touched in this PR (
update(),addWatcher(),removeWatcher(),attachMany(),remove()) follows this same plain pattern —create()is the outlier.🐛 Proposed fix to align with sibling implementations
$body = $this->lastResponse->getContent(); if ($this->lastResponse->getStatusCode() !== 201) { - if (!Future::isForwardCompatibilityEnabled() && $body === '') { + if (!Future::isForwardCompatibilityEnabled()) { return $body; } throw UnexpectedResponseException::create($this->lastResponse); } return new SimpleXMLElement($body);Note:
IssueCategory::create()(referenced in graph context) appears to have the identical pattern/issue — worth checking as well.🤖 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/Redmine/Api/Issue.php` around lines 227 - 269, Update Issue::create() so its non-201 response handling returns the response body whenever Future::isForwardCompatibilityEnabled() is disabled, regardless of whether the body is empty. Preserve throwing UnexpectedResponseException only when forward compatibility is enabled, and inspect IssueCategory::create() for the same conditional pattern and align it if present.
🧹 Nitpick comments (3)
src/Redmine/Api/Issue.php (1)
314-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the duplicated status-validation block into a shared helper.
The same "read body/status, return body if
Futureis disabled, otherwise throwUnexpectedResponseException" block is copy-pasted across at least 7 methods in these two files (and, per graph context, inGroup,Project,IssueCategorytoo). A single protected helper on a shared base class (e.g.AbstractApi::returnBodyOrThrow(array $expectedStatusCodes): string) would remove this duplication and centralize the compatibility logic. Notably,Issue::create()'s divergent, buggy variant of this exact block (flagged separately) is a direct symptom of this duplication — a shared helper would make such drift structurally impossible.
src/Redmine/Api/Issue.php#L314-L325: replace the inline block inupdate()with a call to the shared helper.src/Redmine/Api/Issue.php#L343-L351: replace the inline block inaddWatcher().src/Redmine/Api/Issue.php#L373-L384: replace the inline block inremoveWatcher().src/Redmine/Api/Issue.php#L537-L548: replace the inline block inattachMany().src/Redmine/Api/Issue.php#L565-L576: replace the inline block inremove().src/Redmine/Api/Attachment.php#L160-L170: replace the inline block inupload().src/Redmine/Api/Attachment.php#L189-L200: replace the inline block inremove().♻️ Example shared helper
protected function returnBodyOrThrow(array $expectedStatusCodes): string { $body = $this->lastResponse->getContent(); $statusCode = $this->lastResponse->getStatusCode(); if (!in_array($statusCode, $expectedStatusCodes, true)) { if (!Future::isForwardCompatibilityEnabled()) { return $body; } throw UnexpectedResponseException::create($this->lastResponse); } return $body; }🤖 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/Redmine/Api/Issue.php` around lines 314 - 325, Extract the duplicated response-status compatibility logic into a protected shared-base helper such as returnBodyOrThrow, accepting the expected status codes and preserving the existing body-return and UnexpectedResponseException behavior. Replace the inline blocks in src/Redmine/Api/Issue.php lines 314-325, 343-351, 373-384, 537-548, and 565-576, and src/Redmine/Api/Attachment.php lines 160-170 and 189-200 with calls to that helper, using each method’s existing expected status codes.src/Redmine/Api/Group.php (1)
199-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRepeated status-validation boilerplate — candidate for a shared helper.
The same 8-line pattern (
getContent(),getStatusCode(), compare, return-or-throw) is repeated 5 times in this file, and the same shape recurs across ~10 other API classes in this PR (Attachment, Issue, IssueCategory, IssueRelation, Membership, Project, TimeEntry, User, Version, Wiki). This duplication is exactly what allowed the divergent bug flagged above (create()using a subtly different condition in sibling classes) to slip in unnoticed. Extracting a shared helper (e.g. onAbstractApi) would remove the copy/paste risk going forward.Sketch of a possible shared helper
// in AbstractApi protected function returnBodyOrThrow(string $body, bool $isExpectedStatus): string { if (!$isExpectedStatus && Future::isForwardCompatibilityEnabled()) { throw UnexpectedResponseException::create($this->lastResponse); } return $body; }Not blocking this PR, but worth considering as a follow-up cleanup.
Also applies to: 242-253, 304-315, 337-345, 371-382
🤖 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/Redmine/Api/Group.php` around lines 199 - 207, The repeated status-validation blocks in Group API methods should be centralized to prevent divergent behavior. Add a shared helper on AbstractApi, such as returnBodyOrThrow, that accepts the response body and expected-status result, throws UnexpectedResponseException when the status is unexpected and forward compatibility is enabled, otherwise returns the body; replace the repeated validation logic in the affected Group methods with this helper.src/Redmine/Api/IssueCategory.php (1)
310-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the repeated status-validation logic into a shared helper. The same "read status/body, compare against expected code(s), return body vs. throw
UnexpectedResponseExceptionbased onFuture::isForwardCompatibilityEnabled()" block is copy-pasted across every mutation endpoint touched by this PR. Beyond maintainability, this duplication is the direct root cause of the divergent behavior found inIssueCategory::create()(andIssue::create()) versus their siblings — a single shared method (e.g.AbstractApi::validateResponseStatus(array $expectedCodes)) would make such silent divergence impossible going forward.
src/Redmine/Api/IssueCategory.php#L310-L321: extract this block (and the identical one below) into a shared helper called fromupdate().src/Redmine/Api/IssueCategory.php#L343-L354: call the same shared helper fromremove().src/Redmine/Api/TimeEntry.php#L177-L185: call the shared helper fromcreate()(expected code[201]).src/Redmine/Api/TimeEntry.php#L223-L234: call the shared helper fromupdate()(expected codes[200, 204]).src/Redmine/Api/TimeEntry.php#L253-L264: call the shared helper fromremove()(expected codes[200, 204]).src/Redmine/Api/Version.php#L278-L286: call the shared helper fromcreate()(expected code[201]).src/Redmine/Api/Version.php#L324-L335: call the shared helper fromupdate()(expected codes[200, 204]).src/Redmine/Api/Version.php#L392-L403: call the shared helper fromremove()(expected codes[200, 204]).🤖 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/Redmine/Api/IssueCategory.php` around lines 310 - 321, Extract the duplicated response status/body validation into a shared AbstractApi::validateResponseStatus(array $expectedCodes) helper, preserving the existing Future::isForwardCompatibilityEnabled() return-or-throw behavior. Update src/Redmine/Api/IssueCategory.php lines 310-321 and 343-354 to use it from update() and remove(); src/Redmine/Api/TimeEntry.php lines 177-185, 223-234, and 253-264 to use expected codes [201], [200, 204], and [200, 204] in create(), update(), and remove(); and src/Redmine/Api/Version.php lines 278-286, 324-335, and 392-403 to use expected codes [201], [200, 204], and [200, 204] in create(), update(), and remove().
🤖 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/Redmine/Api/Group.php`:
- Around line 199-207: Update the non-201 response handling in Issue::create(),
IssueCategory::create(), and Membership::create() to return the raw body
whenever Future::isForwardCompatibilityEnabled() is false, regardless of whether
the body is empty; otherwise throw UnexpectedResponseException::create(). Add
`@throws` UnexpectedResponseException to each affected method’s docblock.
In `@src/Redmine/Api/IssueCategory.php`:
- Around line 271-278: Update the non-201 response handling in
IssueCategory::create() and Issue::create() so that, when
Future::isForwardCompatibilityEnabled() is false, they return the raw $body
unconditionally; remove the $body === '' requirement while preserving the
existing exception path for forward-compatibility mode.
In `@src/Redmine/Api/Membership.php`:
- Around line 153-160: Update the non-201 handling in the membership response
method to preserve the legacy path when future compatibility is disabled: return
control for non-empty bodies so the existing SimpleXMLElement conversion remains
reachable, while retaining the empty-body return and throwing
UnexpectedResponseException for all non-201 responses only when future mode is
enabled.
In `@tests/Unit/Api/IssueRelation/RemoveTest.php`:
- Around line 59-77: Update the non-success response tests to use a non-empty
500 response body and assert that each API method returns that body unchanged.
Apply this to IssueRelation::remove in
tests/Unit/Api/IssueRelation/RemoveTest.php:59-77, Membership::remove in
tests/Unit/Api/Membership/RemoveTest.php:59-77, TimeEntry::remove in
tests/Unit/Api/TimeEntry/RemoveTest.php:59-77, Version::remove in
tests/Unit/Api/Version/RemoveTest.php:67-85, and Wiki::create in
tests/Unit/Api/Wiki/CreateTest.php:103-121.
In `@tests/Unit/Client/NativeCurlClient/EnableFutureModeTest.php`:
- Around line 15-28: Ensure both test methods always restore the global Future
flag by wrapping the enable-and-assert sequence in try/finally: update
tests/Unit/Client/NativeCurlClient/EnableFutureModeTest.php lines 15-28 and
tests/Unit/Client/Psr18Client/EnableFutureModeTest.php lines 18-34, placing
Future::disableForwardCompatibility() in the finally block.
In `@tests/Unit/FutureTest.php`:
- Around line 19-24: Reset Future's process-wide forward-compatibility state
around every test in FutureTest: add setUp() and/or tearDown() hooks that
disable the flag before and after each test, including
testEnableForwardCompatabilityLetsIsForwardCompatabilityEnabledReturnTrue, so
tests remain isolated and start from the disabled default.
---
Outside diff comments:
In `@src/Redmine/Api/Issue.php`:
- Around line 227-269: Update Issue::create() so its non-201 response handling
returns the response body whenever Future::isForwardCompatibilityEnabled() is
disabled, regardless of whether the body is empty. Preserve throwing
UnexpectedResponseException only when forward compatibility is enabled, and
inspect IssueCategory::create() for the same conditional pattern and align it if
present.
---
Nitpick comments:
In `@src/Redmine/Api/Group.php`:
- Around line 199-207: The repeated status-validation blocks in Group API
methods should be centralized to prevent divergent behavior. Add a shared helper
on AbstractApi, such as returnBodyOrThrow, that accepts the response body and
expected-status result, throws UnexpectedResponseException when the status is
unexpected and forward compatibility is enabled, otherwise returns the body;
replace the repeated validation logic in the affected Group methods with this
helper.
In `@src/Redmine/Api/Issue.php`:
- Around line 314-325: Extract the duplicated response-status compatibility
logic into a protected shared-base helper such as returnBodyOrThrow, accepting
the expected status codes and preserving the existing body-return and
UnexpectedResponseException behavior. Replace the inline blocks in
src/Redmine/Api/Issue.php lines 314-325, 343-351, 373-384, 537-548, and 565-576,
and src/Redmine/Api/Attachment.php lines 160-170 and 189-200 with calls to that
helper, using each method’s existing expected status codes.
In `@src/Redmine/Api/IssueCategory.php`:
- Around line 310-321: Extract the duplicated response status/body validation
into a shared AbstractApi::validateResponseStatus(array $expectedCodes) helper,
preserving the existing Future::isForwardCompatibilityEnabled() return-or-throw
behavior. Update src/Redmine/Api/IssueCategory.php lines 310-321 and 343-354 to
use it from update() and remove(); src/Redmine/Api/TimeEntry.php lines 177-185,
223-234, and 253-264 to use expected codes [201], [200, 204], and [200, 204] in
create(), update(), and remove(); and src/Redmine/Api/Version.php lines 278-286,
324-335, and 392-403 to use expected codes [201], [200, 204], and [200, 204] in
create(), update(), and remove().
🪄 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: 3bf1c2a9-4a62-49fb-9ce0-09f661e7fa84
📒 Files selected for processing (52)
src/Redmine/Api/Attachment.phpsrc/Redmine/Api/Group.phpsrc/Redmine/Api/Issue.phpsrc/Redmine/Api/IssueCategory.phpsrc/Redmine/Api/IssueRelation.phpsrc/Redmine/Api/Membership.phpsrc/Redmine/Api/Project.phpsrc/Redmine/Api/TimeEntry.phpsrc/Redmine/Api/User.phpsrc/Redmine/Api/Version.phpsrc/Redmine/Api/Wiki.phpsrc/Redmine/Client/ClientApiTrait.phpsrc/Redmine/Future.phptests/Unit/Api/Attachment/RemoveTest.phptests/Unit/Api/Attachment/UploadTest.phptests/Unit/Api/Group/AddUserTest.phptests/Unit/Api/Group/CreateTest.phptests/Unit/Api/Group/RemoveTest.phptests/Unit/Api/Group/RemoveUserTest.phptests/Unit/Api/Group/UpdateTest.phptests/Unit/Api/Issue/AddWatcherTest.phptests/Unit/Api/Issue/AttachManyTest.phptests/Unit/Api/Issue/CreateTest.phptests/Unit/Api/Issue/RemoveTest.phptests/Unit/Api/Issue/RemoveWatcherTest.phptests/Unit/Api/Issue/UpdateTest.phptests/Unit/Api/IssueCategory/CreateTest.phptests/Unit/Api/IssueCategory/RemoveTest.phptests/Unit/Api/IssueCategory/UpdateTest.phptests/Unit/Api/IssueRelation/CreateTest.phptests/Unit/Api/IssueRelation/RemoveTest.phptests/Unit/Api/IssueTest.phptests/Unit/Api/Membership/CreateTest.phptests/Unit/Api/Membership/RemoveTest.phptests/Unit/Api/Membership/UpdateTest.phptests/Unit/Api/Project/CreateTest.phptests/Unit/Api/Project/RemoveTest.phptests/Unit/Api/Project/UpdateTest.phptests/Unit/Api/TimeEntry/CreateTest.phptests/Unit/Api/TimeEntry/RemoveTest.phptests/Unit/Api/TimeEntry/UpdateTest.phptests/Unit/Api/User/CreateTest.phptests/Unit/Api/User/RemoveTest.phptests/Unit/Api/User/UpdateTest.phptests/Unit/Api/Version/CreateTest.phptests/Unit/Api/Version/RemoveTest.phptests/Unit/Api/Version/UpdateTest.phptests/Unit/Api/Wiki/CreateTest.phptests/Unit/Api/Wiki/RemoveTest.phptests/Unit/Client/NativeCurlClient/EnableFutureModeTest.phptests/Unit/Client/Psr18Client/EnableFutureModeTest.phptests/Unit/FutureTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/Unit/Api/Membership/CreateTest.php`:
- Around line 101-122: Update testCreateWithIncorrectStatusCodeReturnsBody to
capture the current Future compatibility state, disable forward compatibility
before calling Membership::create, and restore the original state in a finally
block. Match the setup and cleanup pattern used by the neighboring exception
test while preserving the existing body assertion.
🪄 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: 180f3bd8-a612-4828-b73d-93582420db20
📒 Files selected for processing (6)
src/Redmine/Api/Issue.phpsrc/Redmine/Api/IssueCategory.phpsrc/Redmine/Api/Membership.phptests/Unit/Api/Issue/CreateTest.phptests/Unit/Api/IssueCategory/CreateTest.phptests/Unit/Api/Membership/CreateTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Unit/Api/IssueCategory/CreateTest.php
- src/Redmine/Api/IssueCategory.php
- tests/Unit/Api/Issue/CreateTest.php
- src/Redmine/Api/Issue.php
…mbership::create()
0d4a81a to
5f68c45
Compare
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 (1)
src/Redmine/Api/Issue.php (1)
260-272: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the empty-body success path in
Issue::create().A successful
201response with an empty body now reachesnew SimpleXMLElement($body)on Line 272. This can throw instead of preserving the existing return behavior;IssueCategory::create()already returns the empty body before parsing. Add the same guard here and cover201+ empty body with a regression test.Proposed fix
if ($this->lastResponse->getStatusCode() !== 201) { if (!Future::isForwardCompatibilityEnabled()) { if ($body === '') { return $body; } return new SimpleXMLElement($body); } throw UnexpectedResponseException::create($this->lastResponse); } + if ($body === '') { + return $body; + } + return new SimpleXMLElement($body);🤖 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/Redmine/Api/Issue.php` around lines 260 - 272, Update Issue::create() so a successful 201 response with an empty $body returns the empty body before constructing SimpleXMLElement, while preserving XML parsing for non-empty responses and existing non-201 handling. Add a regression test covering the 201-plus-empty-body case, following the behavior of IssueCategory::create().
🤖 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 `@src/Redmine/Api/Issue.php`:
- Around line 260-272: Update Issue::create() so a successful 201 response with
an empty $body returns the empty body before constructing SimpleXMLElement,
while preserving XML parsing for non-empty responses and existing non-201
handling. Add a regression test covering the 201-plus-empty-body case, following
the behavior of IssueCategory::create().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6daa9945-9d43-4794-b322-418f6938fec6
📒 Files selected for processing (6)
src/Redmine/Api/Issue.phpsrc/Redmine/Api/IssueCategory.phpsrc/Redmine/Api/Membership.phptests/Unit/Api/Issue/CreateTest.phptests/Unit/Api/IssueCategory/CreateTest.phptests/Unit/Api/Membership/CreateTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Unit/Api/Membership/CreateTest.php
- tests/Unit/Api/IssueCategory/CreateTest.php
- tests/Unit/Api/Issue/CreateTest.php
- src/Redmine/Api/Membership.php
All POST/PUT methods now return new SimpleXMLElement() instead of raw on the backwards-compatibility fallback path, matching the pre-PR behavior. Affects Group::create, Group::addUser, Issue::addWatcher, Project::create, TimeEntry::create, User::create, Version::create, Wiki::create.
1 participant
Fixes #367
Summary by CodeRabbit
UnexpectedResponseExceptionwhen on.