[EPMRPP-109228] OAuth password grant #262
Conversation
WalkthroughAdds sync/async authentication (API-key and OAuth2 Password Grant), new Auth interfaces and implementations, synchronous and asynchronous ClientSession wrappers that inject and refresh Authorization headers on 401/403, wires auth into sync/async clients and factory, and expands tests and configs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as RPClient
participant Session as ClientSession
participant Auth as Auth (OAuth/APIKey)
participant Server as HTTP Server
User->>Client: invoke request (get/post/put)
Client->>Session: call method(url, ...)
Session->>Auth: get()
Auth-->>Session: "Bearer <token>"
Session->>Server: request + Authorization header
alt Server responds 401/403
Server-->>Session: 401/403
Session->>Auth: refresh()
Auth-->>Session: "Bearer <new_token>" or None
Session->>Server: retry request with new header
end
Server-->>Session: response
Session-->>Client: response
Client-->>User: return result
sequenceDiagram
participant ApiKey as ApiKeyAuth
participant OAuth as OAuthPasswordGrant
participant Session as ClientSession
rect rgb(232,245,233)
Note over ApiKey: API Key Flow
Session->>ApiKey: get()
ApiKey-->>Session: "Bearer <api_key>"
end
rect rgb(255,243,224)
Note over OAuth: OAuth Password Grant Flow
Session->>OAuth: get()
alt no token or expired
OAuth->>OAuth: POST oauth_uri (username,password,client_id...)
OAuth-->>Session: stores access_token/refresh_token
end
OAuth-->>Session: "Bearer <access_token>"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)tests/_internal/services/test_auth.py (2)
reportportal_client/_internal/http.py (2)
🪛 Ruff (0.14.3)tests/_internal/services/test_auth.py34-34: Possible hardcoded password assigned to: "PASSWORD" (S105) 36-36: Possible hardcoded password assigned to: "CLIENT_SECRET" (S105) 38-38: Possible hardcoded password assigned to: "ACCESS_TOKEN" (S105) 39-39: Possible hardcoded password assigned to: "REFRESH_TOKEN" (S105) 101-101: Possible hardcoded password assigned to: "new_access_token" (S105) 190-190: Possible hardcoded password assigned to: "new_token" (S105) 224-224: Possible hardcoded password assigned to: "new_token" (S105) 250-250: Possible hardcoded password assigned to: "new_token" (S105) 333-333: Possible hardcoded password assigned to: "new_access_token" (S105) 440-440: Possible hardcoded password assigned to: "new_token" (S105) 482-482: Possible hardcoded password assigned to: "new_token" (S105) 514-514: Possible hardcoded password assigned to: "new_token" (S105) 544-544: Possible hardcoded password assigned to: "api_token" (S105) 552-552: Possible hardcoded password assigned to: "api_token" (S105) 560-560: Possible hardcoded password assigned to: "api_token" (S105) 576-576: Possible hardcoded password assigned to: "api_token" (S105) 585-585: Possible hardcoded password assigned to: "api_token" (S105) 594-594: Possible hardcoded password assigned to: "api_token" (S105) ⏰ 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). (11)
🔇 Additional comments (10)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
reportportal_client/helpers/common_helpers.py (1)
96-107: Fix return type annotation to match actual behavior.The method signature declares a return type of
_T, but the implementation returnsNonewhen the queue is empty (lines 102 and 107). The return type should beOptional[_T]to accurately reflect this behavior.Apply this diff to fix the return type:
- def last(self) -> _T: + def last(self) -> Optional[_T]: """Return the last element from the queue, but does not remove it. :return: The last element in the queue.reportportal_client/client.py (1)
1034-1037: Close the auth backend to avoid leaking its session
OAuthPasswordGrantSynckeeps its ownrequests.Session, soRPClient.close()must delegate to the auth object; otherwise OAuth users will leak sockets. Please invoke the authclose()hook when it exists.def close(self) -> None: """Close current client connections.""" self._log(self._log_batcher.flush()) self.session.close() + auth_close = getattr(self.auth, "close", None) + if callable(auth_close): + auth_close()reportportal_client/aio/client.py (1)
291-296: Await the auth backend’s close hookThe OAuth async backend allocates its own
aiohttp.ClientSession; without awaiting itsclose()we leak connections and trigger “Unclosed client session” warnings. Please call the auth close hook (awaiting it when it’s a coroutine).async def close(self) -> None: """Gracefully close internal aiohttp.ClientSession class instance and reset it.""" if self._session: await self._session.close() self._session = None + auth_close = getattr(self.auth, "close", None) + if callable(auth_close): + maybe_close = auth_close() + if asyncio.iscoroutine(maybe_close): + await maybe_close
🧹 Nitpick comments (5)
tests/conftest.py (1)
36-37: Optional: Consider makingok()status-code-aware.Now that
status_codeis available, you could optionally enhanceok()to check whetherstatus_codeis in the 200-299 range, matching standard HTTP response behavior.Example refactor:
def ok(self): - return True + return 200 <= self.status_code < 300Note: Only apply this if existing tests don't rely on the current unconditional
Truebehavior.reportportal_client/_internal/services/statistics.py (1)
76-78: Good change to add Optional, but consider explicit None return.The return type change accurately reflects that the function can return
Nonewhen an exception occurs. However, the exception handler (lines 96-97) implicitly returnsNone. For consistency with the updatedasync_send_eventand better code clarity, consider explicitly returningNone.Apply this diff to make the None return explicit:
except requests.exceptions.RequestException as err: logger.debug("Failed to send data to Statistics service: %s", str(err)) + return Nonereportportal_client/_internal/logs/batcher.py (1)
53-53: Use generic type parameterT_cofor type consistency.The method signature uses explicit
RPRequestLogtypes instead of the generic type parameterT_co. This breaks consistency with the class's generic design and could cause type checker issues whenappend_async(line 85) passesAsyncRPRequestLog.Apply this diff to use the generic type parameter:
- def _append(self, size: int, log_req: RPRequestLog) -> Optional[List[RPRequestLog]]: + def _append(self, size: int, log_req: T_co) -> Optional[List[T_co]]:reportportal_client/_internal/http.py (1)
64-70: Close the first response before retrying.
When we receive a 401/403 we immediately retry after refreshing the token, but we never close the original response. In requests, leaving that object open keeps the connection checked out of the pool until GC kicks in, which can exhaust sockets under heavy auth failures. Please close the first response before issuing the retry.- if result.status_code in AUTH_PROBLEM_STATUSES and self.__auth: - refreshed_header = self.__auth.refresh() - if refreshed_header: - # Retry with new auth header - request_kwargs["headers"] = request_kwargs.get("headers", {}).copy() - request_kwargs["headers"]["Authorization"] = refreshed_header - result = method(url, **request_kwargs) + if result.status_code in AUTH_PROBLEM_STATUSES and self.__auth: + refreshed_header = self.__auth.refresh() + if refreshed_header: + result.close() + # Retry with new auth header + request_kwargs["headers"] = request_kwargs.get("headers", {}).copy() + request_kwargs["headers"]["Authorization"] = refreshed_header + result = method(url, **request_kwargs)tests/_internal/services/test_auth.py (1)
329-336: Useasyncio.sleepinside async tests.
These async tests calltime.sleep, which blocks the event loop for a full second every run. Switching toawait asyncio.sleep(...)keeps the loop responsive and will shorten the suite once more cases pile up.- time.sleep(1) + await asyncio.sleep(1)(Apply the same change to the other async tests in this module and add
import asyncioat the top.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitignore(1 hunks)pyproject.toml(1 hunks)reportportal_client/__init__.py(2 hunks)reportportal_client/_internal/aio/http.py(5 hunks)reportportal_client/_internal/aio/tasks.py(2 hunks)reportportal_client/_internal/http.py(1 hunks)reportportal_client/_internal/local/__init__.py(1 hunks)reportportal_client/_internal/logs/batcher.py(3 hunks)reportportal_client/_internal/services/auth.py(1 hunks)reportportal_client/_internal/services/client_id.py(1 hunks)reportportal_client/_internal/services/statistics.py(2 hunks)reportportal_client/aio/client.py(16 hunks)reportportal_client/client.py(9 hunks)reportportal_client/helpers/common_helpers.py(3 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)tests/_internal/aio/test_aio_http.py(4 hunks)tests/_internal/services/test_auth.py(1 hunks)tests/_internal/test_http.py(1 hunks)tests/aio/test_aio_client.py(6 hunks)tests/conftest.py(1 hunks)tests/test_client.py(2 hunks)tests/test_client_factory.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
tests/test_client_factory.py (1)
reportportal_client/__init__.py (1)
create_client(80-131)
reportportal_client/_internal/http.py (1)
reportportal_client/_internal/services/auth.py (1)
Auth(31-53)
reportportal_client/_internal/aio/http.py (3)
reportportal_client/_internal/services/auth.py (15)
AuthAsync(57-79)get(40-45)get(66-71)get(98-103)get(131-136)get(361-382)get(487-508)refresh(48-53)refresh(74-79)refresh(105-112)refresh(138-145)refresh(384-390)refresh(510-516)close(392-395)close(518-521)reportportal_client/aio/client.py (3)
close(291-295)close(1089-1092)close(1518-1522)reportportal_client/client.py (2)
close(352-354)close(1034-1037)
tests/_internal/services/test_auth.py (1)
reportportal_client/_internal/services/auth.py (16)
ApiKeyAuthAsync(115-145)ApiKeyAuthSync(82-112)OAuthPasswordGrantAsync(398-521)OAuthPasswordGrantSync(272-395)get(40-45)get(66-71)get(98-103)get(131-136)get(361-382)get(487-508)refresh(48-53)refresh(74-79)refresh(105-112)refresh(138-145)refresh(384-390)refresh(510-516)
reportportal_client/client.py (5)
reportportal_client/_internal/services/auth.py (3)
ApiKeyAuthSync(82-112)Auth(31-53)OAuthPasswordGrantSync(272-395)reportportal_client/aio/client.py (1)
session(246-289)reportportal_client/_internal/http.py (1)
mount(86-92)reportportal_client/core/rp_responses.py (4)
message(112-117)message(176-181)messages(120-127)messages(184-192)reportportal_client/core/rp_requests.py (5)
ErrorPrintingHttpRequest(147-181)make(124-144)make(158-181)make(206-222)make(236-255)
tests/_internal/test_http.py (2)
reportportal_client/_internal/http.py (6)
ClientSession(27-109)get(74-76)post(78-80)put(82-84)mount(86-92)close(94-96)reportportal_client/_internal/services/auth.py (1)
ApiKeyAuthSync(82-112)
reportportal_client/_internal/aio/tasks.py (1)
reportportal_client/_internal/logs/batcher.py (1)
flush(87-100)
reportportal_client/__init__.py (2)
reportportal_client/aio/client.py (7)
endpoint(728-733)endpoint(1130-1135)project(736-741)project(1138-1143)launch_uuid(720-725)launch_uuid(1122-1127)AsyncRPClient(694-1092)reportportal_client/client.py (9)
endpoint(114-119)endpoint(426-431)project(123-128)project(434-439)launch_uuid(91-96)launch_uuid(418-423)OutputType(65-76)RP(80-373)RPClient(376-1057)
tests/aio/test_aio_client.py (2)
reportportal_client/_internal/aio/http.py (4)
ClientSession(168-244)RetryingClientSession(51-165)get(138-140)get(217-219)reportportal_client/aio/client.py (11)
client(712-717)client(1114-1119)Client(92-691)session(246-289)endpoint(728-733)endpoint(1130-1135)project(736-741)project(1138-1143)get_project_settings(623-630)get_project_settings(1036-1041)get_project_settings(1479-1486)
reportportal_client/aio/client.py (3)
reportportal_client/_internal/aio/http.py (4)
ClientSession(168-244)RetryingClientSession(51-165)post(142-144)post(221-223)reportportal_client/_internal/services/auth.py (3)
ApiKeyAuthAsync(115-145)AuthAsync(57-79)OAuthPasswordGrantAsync(398-521)reportportal_client/core/rp_requests.py (15)
payload(268-273)payload(293-312)payload(328-341)payload(393-400)payload(414-422)payload(472-477)payload(491-498)payload(530-535)payload(563-574)payload(627-647)payload(665-678)make(124-144)make(158-181)make(206-222)make(236-255)
tests/test_client.py (3)
tests/aio/test_aio_client.py (8)
test_empty_api_key_argument(160-166)test_oauth_authentication_parameters(169-189)test_oauth_authentication_without_optional_parameters(192-210)test_no_authentication_parameters(213-220)test_partial_oauth_parameters(223-236)test_clone_with_oauth(239-280)test_api_key_authorization_header(923-955)test_oauth_authorization_header(959-1001)reportportal_client/client.py (14)
RPClient(376-1057)endpoint(114-119)endpoint(426-431)project(123-128)project(434-439)_add_current_item(981-983)clone(343-349)clone(1002-1032)launch_uuid(91-96)launch_uuid(418-423)current_item(335-340)current_item(995-1000)get_project_settings(304-309)get_project_settings(966-979)tests/conftest.py (1)
DummyResponse(25-37)
reportportal_client/_internal/logs/batcher.py (1)
reportportal_client/core/rp_requests.py (1)
RPRequestLog(502-549)
reportportal_client/_internal/services/auth.py (3)
reportportal_client/_internal/static/abstract.py (1)
AbstractBaseClass(22-54)reportportal_client/_internal/aio/http.py (7)
get(138-140)get(217-219)post(142-144)post(221-223)close(150-152)close(229-231)ClientSession(168-244)reportportal_client/aio/client.py (1)
session(246-289)
tests/_internal/aio/test_aio_http.py (3)
reportportal_client/_internal/aio/http.py (1)
RetryingClientSession(51-165)reportportal_client/_internal/services/auth.py (1)
ApiKeyAuthAsync(115-145)reportportal_client/aio/client.py (1)
session(246-289)
🪛 Gitleaks (8.28.0)
tests/aio/test_aio_client.py
[high] 925-925: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/test_client.py
[high] 441-441: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.3)
tests/_internal/services/test_auth.py
33-33: Possible hardcoded password assigned to: "PASSWORD"
(S105)
35-35: Possible hardcoded password assigned to: "CLIENT_SECRET"
(S105)
37-37: Possible hardcoded password assigned to: "ACCESS_TOKEN"
(S105)
38-38: Possible hardcoded password assigned to: "REFRESH_TOKEN"
(S105)
100-100: Possible hardcoded password assigned to: "new_access_token"
(S105)
189-189: Possible hardcoded password assigned to: "new_token"
(S105)
223-223: Possible hardcoded password assigned to: "new_token"
(S105)
249-249: Possible hardcoded password assigned to: "new_token"
(S105)
332-332: Possible hardcoded password assigned to: "new_access_token"
(S105)
439-439: Possible hardcoded password assigned to: "new_token"
(S105)
481-481: Possible hardcoded password assigned to: "new_token"
(S105)
513-513: Possible hardcoded password assigned to: "new_token"
(S105)
543-543: Possible hardcoded password assigned to: "api_token"
(S105)
551-551: Possible hardcoded password assigned to: "api_token"
(S105)
559-559: Possible hardcoded password assigned to: "api_token"
(S105)
575-575: Possible hardcoded password assigned to: "api_token"
(S105)
584-584: Possible hardcoded password assigned to: "api_token"
(S105)
593-593: Possible hardcoded password assigned to: "api_token"
(S105)
reportportal_client/client.py
586-598: Avoid specifying long messages outside the exception class
(TRY003)
reportportal_client/__init__.py
131-131: Avoid specifying long messages outside the exception class
(TRY003)
tests/aio/test_aio_client.py
78-78: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
176-176: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
178-178: Possible hardcoded password assigned to argument: "oauth_client_secret"
(S106)
185-185: Possible hardcoded password assigned to: "oauth_password"
(S105)
187-187: Possible hardcoded password assigned to: "oauth_client_secret"
(S105)
199-199: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
206-206: Possible hardcoded password assigned to: "oauth_password"
(S105)
231-231: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
966-966: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
988-988: Possible hardcoded password assigned to: "test_token"
(S105)
reportportal_client/aio/client.py
232-244: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_client.py
328-328: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
330-330: Possible hardcoded password assigned to argument: "oauth_client_secret"
(S106)
337-337: Possible hardcoded password assigned to: "oauth_password"
(S105)
339-339: Possible hardcoded password assigned to: "oauth_client_secret"
(S105)
351-351: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
358-358: Possible hardcoded password assigned to: "oauth_password"
(S105)
383-383: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
474-474: Possible hardcoded password assigned to argument: "oauth_password"
(S106)
489-489: Possible hardcoded password assigned to: "test_token"
(S105)
reportportal_client/_internal/services/auth.py
248-248: Consider moving this statement to an else block
(TRY300)
⏰ 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). (6)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.8)
- GitHub Check: build (3.9)
🔇 Additional comments (24)
reportportal_client/_internal/local/__init__.py (1)
21-25: Explicit return for clarity is a good improvement.The change from implicit to explicit
Nonereturn follows the Pythonic principle of "Explicit is better than implicit" and improves code clarity. No functional impact..gitignore (1)
117-118: Verify the purpose and necessity of ignoring AGENTS.md and PROMPTS.md.The
.gitignoreentries excludeAGENTS.mdandPROMPTS.mdfrom version control. However, the PR context indicates these are new documentation files being introduced. This creates a contradiction: if they're part of the PR changes, they should typically be committed to the repository, not ignored.Before merging, clarify:
- Should these files be committed as part of the repository, or are they generated/temporary artifacts?
- If they're documentation, consider whether excluding them from Git tracking aligns with the project's documentation strategy.
tests/conftest.py (1)
26-28: LGTM! Good addition to make the mock more complete.Adding the
status_codeattribute makesDummyResponsea more realistic HTTP response mock, which aligns well with the PR's OAuth authentication features that check response status codes for token refresh on 401/403.requirements.txt (1)
2-2: No issues found with typing-extensions>=4.13.2 dependency.typing-extensions 4.13.2 requires Python >= 3.8, and the project's setup.py confirms support for Python 3.8 through 3.13. The dependency is fully compatible with the project's minimum Python version, and the
>=constraint is appropriate for allowing updates without breaking compatibility.reportportal_client/_internal/logs/batcher.py (2)
63-69: LGTM! Batching and flush logic improvements.The batch size check (lines 63-69) correctly returns the batch when it reaches
entry_numcapacity. The flush method's double-checked locking pattern (lines 92-100) is a valid optimization that avoids acquiring the lock when the batch is already empty, while maintaining thread safety with the inner check.Also applies to: 92-100
55-60: No changes needed—payload limit is a soft limit by design.The behavior described in the review is intentional. When a single log's size equals or exceeds the payload limit, the batcher allows it in a batch to ensure progress and prevent empty batches. This "soft limit" design is validated by tests that explicitly verify batches can exceed the limit by up to 10% (see
test_log_batch_triggers_previous_request_to_send()intests/_internal/logs/test_log_batcher.py, lines 139).requirements-dev.txt (1)
4-5: LGTM!Adding
blackandisortas development dependencies is a good practice for maintaining consistent code formatting across the codebase.pyproject.toml (1)
19-23: LGTM!The pytest configuration is well-structured and appropriate for the new test suite, including async test support.
reportportal_client/__init__.py (4)
16-23: LGTM!Proper handling of
Unpackimport across Python versions ensures compatibility with both Python 3.11+ (where it's intyping) and older versions (wheretyping_extensionsis needed).
44-77: LGTM!The
_ClientOptionsTypedDict provides excellent type safety for client configuration, covering API key authentication, OAuth 2.0 parameters, and all client-specific options.
80-122: LGTM!The updated signature using
**kwargs: Unpack[_ClientOptions]provides strong type safety while maintaining flexibility. The docstring comprehensively documents both API key and OAuth 2.0 authentication options.
123-131: Excellent improvement in error handling!Replacing the implicit
Nonereturn with an explicitValueErrormakes the failure mode clear and actionable for callers. The kwargs delegation to client constructors is also cleaner.reportportal_client/_internal/services/client_id.py (1)
56-60: LGTM!The explicit
return Noneimproves code clarity by making the control flow obvious when the client ID property is not found.reportportal_client/helpers/common_helpers.py (2)
71-71: LGTM!Correctly changed from initialization to type annotation. The actual initialization properly occurs in
__init__at line 76.
156-156: LGTM!The explicit type annotation for the
attributevariable improves code clarity and type checking.tests/test_client_factory.py (1)
29-30: LGTM!Adding the
api_keyparameter aligns with the new authentication requirements introduced in this PR.reportportal_client/_internal/aio/tasks.py (3)
173-185: LGTM!The updated logic with early
return Nonemakes the control flow more explicit and easier to follow.
187-197: LGTM!The early
return Nonepattern aligns with the changes inappend()and improves code consistency.
225-236: LGTM!The updated
flush()method follows the same clear early-return pattern as the other batchers, improving consistency across the codebase.tests/_internal/test_http.py (2)
23-91: LGTM!The test infrastructure is well-designed:
ReuseAddrTCPServerprevents port conflicts- Context managers ensure proper cleanup
- Tests properly validate both no-auth and authenticated scenarios
93-250: LGTM!Excellent test coverage of authentication flows:
- Proper verification of refresh behavior on 401/403
- Confirmation that refresh only happens once per request
- POST/PUT request bodies properly consumed to avoid connection resets
- Mock assertions verify exact call counts
tests/_internal/aio/test_aio_http.py (1)
185-206: Great coverage for auth header injection.
The newtest_auth_header_added_to_requestneatly exercises the async wrapper and confirms we actually send the Authorization header once auth is configured—nice addition to prevent regressions around header propagation.tests/aio/test_aio_client.py (1)
922-1000: Nice end-to-end assertions for auth headers.
The async client header checks give us confidence that both API key and OAuth credentials are threaded through the new wrapper correctly—solid addition.tests/test_client.py (1)
439-502: Sync coverage mirrors the async path well.
Appreciate the mirrored assertions for the synchronous client—this keeps the two code paths aligned and should catch divergence early.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
reportportal_client/_internal/aio/http.py (1)
213-218: Useawait result.release()instead ofresult.close()Similar to line 104, when retrying with a refreshed token, the previous response's connection should be returned to the pool using the async
release()method. This issue was identified in the past review comments and should be addressed.Apply this diff:
if refreshed_header: - # Close previous result if it's retried to release resources - result.close() + # Release previous result to return connection to pool + await result.release() # Retry with new auth header
🧹 Nitpick comments (1)
tests/_internal/services/test_auth.py (1)
429-430: Replace blocking sleep in async tests
time.sleep(1)inside these@pytest.mark.asynciotests blocks the event loop thread, defeating the async scheduler and risking flakiness/timeouts when other coroutines are scheduled. Swap to the non-blocking variant.Apply this diff:
- time.sleep(1) + await asyncio.sleep(1)Repeat this change for each async test that currently calls
time.sleep.Also applies to: 471-472, 523-524
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
reportportal_client/_internal/aio/http.py(5 hunks)reportportal_client/_internal/services/statistics.py(3 hunks)tests/_internal/services/test_auth.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
reportportal_client/_internal/aio/http.py (2)
reportportal_client/_internal/services/auth.py (15)
AuthAsync(57-79)close(392-395)close(518-521)get(40-45)get(66-71)get(98-103)get(131-136)get(361-382)get(487-508)refresh(48-53)refresh(74-79)refresh(105-112)refresh(138-145)refresh(384-390)refresh(510-516)reportportal_client/aio/client.py (3)
close(291-295)close(1089-1092)close(1518-1522)
tests/_internal/services/test_auth.py (2)
reportportal_client/_internal/services/auth.py (16)
ApiKeyAuthAsync(115-145)ApiKeyAuthSync(82-112)OAuthPasswordGrantAsync(398-521)OAuthPasswordGrantSync(272-395)get(40-45)get(66-71)get(98-103)get(131-136)get(361-382)get(487-508)refresh(48-53)refresh(74-79)refresh(105-112)refresh(138-145)refresh(384-390)refresh(510-516)tests/conftest.py (1)
ok(36-37)
🪛 Ruff (0.14.3)
tests/_internal/services/test_auth.py
34-34: Possible hardcoded password assigned to: "PASSWORD"
(S105)
36-36: Possible hardcoded password assigned to: "CLIENT_SECRET"
(S105)
38-38: Possible hardcoded password assigned to: "ACCESS_TOKEN"
(S105)
39-39: Possible hardcoded password assigned to: "REFRESH_TOKEN"
(S105)
101-101: Possible hardcoded password assigned to: "new_access_token"
(S105)
190-190: Possible hardcoded password assigned to: "new_token"
(S105)
224-224: Possible hardcoded password assigned to: "new_token"
(S105)
250-250: Possible hardcoded password assigned to: "new_token"
(S105)
333-333: Possible hardcoded password assigned to: "new_access_token"
(S105)
440-440: Possible hardcoded password assigned to: "new_token"
(S105)
482-482: Possible hardcoded password assigned to: "new_token"
(S105)
514-514: Possible hardcoded password assigned to: "new_token"
(S105)
544-544: Possible hardcoded password assigned to: "api_token"
(S105)
552-552: Possible hardcoded password assigned to: "api_token"
(S105)
560-560: Possible hardcoded password assigned to: "api_token"
(S105)
576-576: Possible hardcoded password assigned to: "api_token"
(S105)
585-585: Possible hardcoded password assigned to: "api_token"
(S105)
594-594: Possible hardcoded password assigned to: "api_token"
(S105)
⏰ 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). (6)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.8)
- GitHub Check: build (3.10)
- GitHub Check: build (3.8)
🔇 Additional comments (9)
reportportal_client/_internal/services/statistics.py (3)
76-78: LGTM: Return type properly reflects error handling.The updated return type
Optional[requests.Response]accurately reflects that the function returnsNoneon exception, improving type safety and caller expectations.
98-98: Good: Explicit return improves clarity.The explicit
return Noneis clearer than a barereturnand aligns with the updated type annotation.
125-126: Excellent: Logging fix addresses previous feedback.The logging now properly formats the exception with the
%splaceholder, addressing the issue flagged in the previous review. The pattern now matches the sync version at line 97, and the explicitreturn Nonemaintains consistency with the updated return type.reportportal_client/_internal/aio/http.py (6)
27-34: LGTM: Import changes support the new authentication wrapperThe type alias
AioHttpClientSessionclearly distinguishes aiohttp's native session from the new wrapper class, and all imports are properly utilized.
40-40: LGTM: Well-defined constant for authentication error detection
54-78: LGTM: Consistent use of type aliasThe
AioHttpClientSessionalias is used consistently in both the type annotation and instantiation.
171-189: LGTM: Clean wrapper design with proper type hintsThe constructor clearly documents its parameters and the wrapper pattern allows flexible composition with or without authentication.
196-212: LGTM: Authentication injection and refresh logic is correctThe method properly:
- Copies kwargs and headers to avoid mutation
- Adds Authorization header from auth provider
- Detects authentication failures (401/403)
- Attempts token refresh before retrying
222-249: LGTM: HTTP methods and context manager properly implementedThe HTTP method delegates correctly forward requests through
__request, and the async context manager ensures proper cleanup on exit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
reportportal_client/_internal/http.py (2)
44-74: Refine URL type consistency.The
__requestmethod signature acceptsUnion[str, bytes]for theurlparameter (line 44), but the public methodsget,post, andput(lines 76, 80, 84) only acceptstr. This inconsistency could be confusing.Consider aligning the types:
-def __request(self, method: Callable, url: Union[str, bytes], **kwargs: Any) -> requests.Response: +def __request(self, method: Callable, url: str, **kwargs: Any) -> requests.Response:Alternatively, if
bytesURLs should be supported, update the public method signatures to match.
76-86: Consider adding support for additional HTTP methods.Currently, only GET, POST, and PUT are implemented. If the client will need other HTTP methods (DELETE, PATCH, HEAD, OPTIONS), consider adding them now for completeness.
If needed, I can help generate the implementation for additional HTTP methods following the same pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
reportportal_client/_internal/http.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
reportportal_client/_internal/http.py (1)
reportportal_client/_internal/services/auth.py (1)
Auth(31-53)
🔇 Additional comments (3)
reportportal_client/_internal/http.py (3)
27-42: LGTM!The class initialization is clean and properly encapsulates the requests.Session and Auth instances. The use of private
__authattribute provides good encapsulation.
88-111: LGTM!The utility methods and context manager support are well-implemented:
mount()properly delegates to the underlying session for custom adaptersclose()ensures proper resource cleanup- Context manager follows the standard pattern and guarantees cleanup
24-24: ****The inclusion of 403 in
AUTH_PROBLEM_STATUSESis intentional and correct. TheAuth.refresh()method implements a full OAuth re-authentication flow (clearing the cached token and callingget()), not just a simple token refresh. Tests explicitly document that 403 responses trigger a fallback to password grant authentication, which properly resolves the authorization issue. This is the appropriate behavior for handling revoked refresh tokens.
1 participant
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Tests