fix: no longer require token expiration from the OAuth resource server #462
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
93-93:⚠️ Potential issueType mismatch between method signature and usage.
As mentioned earlier, there's a pipeline error here: "Argument 1 to 'set_token_expiry_date' of 'AbstractOauth2Authenticator' has incompatible type 'str | int | None'; expected 'str | int'"
We need to add a conditional check before calling
set_token_expiry_dateto handle the case whenexpires_inisNone. This is similar to the change made in the oauth.py implementation.🧰 Tools
🪛 GitHub Actions: Linters
[error] 93-93: Argument 1 to 'set_token_expiry_date' of 'AbstractOauth2Authenticator' has incompatible type 'str | int | None'; expected 'str | int' [arg-type]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(3 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py
[error] 367-367: Incompatible return value type (got 'tuple[Any, str | None, str | None]', expected 'tuple[str, str | None, str]') [return-value]
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
[error] 93-93: Argument 1 to 'set_token_expiry_date' of 'AbstractOauth2Authenticator' has incompatible type 'str | int | None'; expected 'str | int' [arg-type]
[error] 215-215: Returning Any from function declared to return 'Mapping[str, Any]' [no-any-return]
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
349-353: Good enhancement to handle optional expiration dates!This conditional check intelligently prevents calculating the expiration date when
access_token_expires_inisNone, which aligns perfectly with making expiration dates optional. This defensive programming approach avoids potential runtime errors.airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (2)
297-298: Consistent string/None handling - nice!I like how you've updated all three extraction methods to consistently handle potential None values and ensure string conversion. This creates a reliable pattern across the codebase and improves type safety.
Also applies to: 310-311, 323-324
332-332: Return type clarification is good.Explicitly including
Nonein the return type makes it clear to consumers of this method that they need to handle the potential absence of values.
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Outdated
Show resolved
Hide resolved
dbgold17
force-pushed
the
dbgold17/optional-oauth-token-expiration-date
branch
from
739ebec to
ccac289
Compare
dbgold17
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
93-93:⚠️ Potential issueType mismatch in get_access_token method
There's a critical type error in the
get_access_tokenmethod. Theexpires_invalue fromrefresh_access_token()can now beNone, butset_token_expiry_dateexpects aUnion[str, int].To fix this issue, add a null check before setting the expiry date:
def get_access_token(self) -> str: if self.token_has_expired(): token, expires_in = self.refresh_access_token() self.access_token = token - self.set_token_expiry_date(expires_in) + if expires_in is not None: + self.set_token_expiry_date(expires_in) + # Optional: else consider setting a default expiry date here return self.access_tokenThis ensures we only call
set_token_expiry_datewith a valid non-None value, wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[error] 93-93: Argument 1 to 'set_token_expiry_date' of 'AbstractOauth2Authenticator' has incompatible type 'str | None'; expected 'str | int' [arg-type]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/auth/oauth.py(1 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(3 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(1 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
refresh_access_token(133-145)
airbyte_cdk/sources/declarative/auth/oauth.py (3)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (2)
set_token_expiry_date(123-124)set_token_expiry_date(304-313)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
set_token_expiry_date(459-460)airbyte_cdk/utils/datetime_helpers.py (1)
AirbyteDateTime(92-342)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (3)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (3)
refresh_access_token(358-370)token_expiry_is_time_of_expiration(127-128)get_expires_in_name(105-106)airbyte_cdk/utils/datetime_helpers.py (1)
ab_datetime_now(345-358)airbyte_cdk/sources/declarative/auth/oauth.py (1)
get_expires_in_name(219-220)
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (2)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
refresh_access_token(358-370)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
refresh_access_token(133-145)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py
[error] 350-350: Argument 1 to 'get_new_token_expiry_date' of 'SingleUseRefreshTokenOauth2Authenticator' has incompatible type 'str | None'; expected 'str' [arg-type]
airbyte_cdk/sources/declarative/auth/oauth.py
[error] 242-242: Argument 1 of 'set_token_expiry_date' is incompatible with supertype 'AbstractOauth2Authenticator'; supertype defines the argument type as 'str | int' [override]
[error] 243-243: Argument 1 to '_parse_token_expiration_date' of 'AbstractOauth2Authenticator' has incompatible type 'AirbyteDateTime'; expected 'str | int' [arg-type]
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
[error] 93-93: Argument 1 to 'set_token_expiry_date' of 'AbstractOauth2Authenticator' has incompatible type 'str | None'; expected 'str | int' [arg-type]
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
[error] Ruff formatting check failed. 2 files would be reformatted.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (8)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
358-358: Type signature updated for better type safety – looks good!The change from
Tuple[str, str, str]toTuple[str, Optional[str], str]correctly reflects that token expiration might not always be available from the OAuth provider. This makes the API more flexible and aligns with the PR objective of not requiring token expiration from the resource server.unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (4)
239-239: Good addition of time freezing for deterministic testsAdding the
@freezegun.freeze_timedecorator makes these tests more deterministic by fixing the time to a specific value, which is great for testing time-dependent logic.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
285-292: Good test case for missing expires_in propertyThis new test case verifies that when the
expires_infield is missing from the response, the system defaults to "3600" seconds (1 hour), which aligns perfectly with the PR's objective of creating a default expiration time.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
406-407: Good test parameter additionsAdding test cases for
Nonevalues inexpires_in_responsewith and without format strings ensures comprehensive test coverage for the new default behavior.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
409-409: Clear test case identifiersThe updated test identifiers clearly communicate what each test case is checking, making the tests more maintainable.
🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (3)
133-133: Good type signature improvementUpdating the return type to
Tuple[str, Optional[str]]improves type safety by explicitly indicating that token expiration might be absent.
151-158: Well-implemented default token expiry date logicThe new
_default_token_expiry_datemethod provides a sensible default (1 hour) for token expiration when not specified by the OAuth provider. This is a clean implementation that respects the format expectations based ontoken_expiry_is_time_of_expiration.
330-336: Good implementation of fallback mechanismThe updated
_extract_token_expiry_datemethod now properly handles the case when expiration information is missing by calling_default_token_expiry_date(), which aligns with the PR objective.
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThe changes update the OAuth token refresh methods to handle missing token expiry values gracefully. In the abstract and concrete OAuth authenticator classes, the return type for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Authenticator
Client->>Authenticator: Call refresh_access_token()
Authenticator->>Authenticator: Extract "expires_in" from response
alt "expires_in" is present and valid
Authenticator-->>Client: Return (access_token, expires_in as AirbyteDateTime, refresh_token)
else "expires_in" missing or expired
Authenticator->>Authenticator: Call _default_token_expiry_date()
Authenticator-->>Client: Return (access_token, default_expiry, refresh_token)
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Does this updated summary and structure look good to you? Would you like me to help with anything else? Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
331-331: Handle missing or null access_token_expires_in?Here we're setting the token expiry date using
access_token_expires_in. If the auth server returns no expiry, could this call fail? Would you like to add a fallback check for a missing or null value, or is it guaranteed by design? Wdyt?unit_tests/sources/declarative/auth/test_oauth.py (6)
229-232: Comprehensive test for token expiry logic?You're verifying both the returned
access_tokenand the computedtoken_expiry_date. Do you think we need a separate test scenario for when the server omitsexpires_inentirely, or is that covered elsewhere? Wdyt?
262-265: Validate missing expires_in response?These lines confirm
access_tokenandtoken_expiry_dateare correct whenexpires_inis provided. Should we also assert behavior if the server omitsexpires_inin this test suite? Wdyt?
343-343: Testing new access token response.Here you're returning
expires_in=1000. Do you want to add a negative or zero edge case scenario, or is 1000 seconds enough coverage? Wdyt?
348-349: Accessing private attribute in tests.You're asserting
oauth._token_expiry_datedirectly. Would calling the public methodget_token_expiry_date()be safer to maintain encapsulation? Wdyt?
350-392: Great parameterized test for absent expiry.Testing multiple scenarios for missing expiry date is thorough! Do you see any benefit in adding a case for partially malformed responses, or is this enough coverage right now? Wdyt?
529-529: Consider negative expires_in in assertion test?We confirm
(access_token, ab_datetime_now() + 1000s). Would you like to add an assertion or test path for invalid or negative durations if an auth server sends unexpected data? Wdyt?unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (3)
108-110: Mocking logic is clear.Mocking
refresh_access_tokenwith a specific expiry is a helpful practice for precise test coverage. Maybe add a brief comment on why 1000 seconds was chosen for clarity? Wdyt?
256-269: Comprehensive expired-token instantiation.Creating
oauth_with_expired_tokenis a clean approach to test fallback scenarios. Are there any additional edge cases (e.g., negative refresh tokens) you want to cover? Wdyt?
1-1: Formatting changes flagged by Ruff.The pipeline indicates that Ruff would reformat this file. Would you like to run a formatter (e.g.,
ruff --fix .) to satisfy the linters? Wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. File would be reformatted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/auth/oauth.py(1 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(6 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(2 hunks)unit_tests/sources/declarative/auth/test_oauth.py(7 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
airbyte_cdk/sources/declarative/auth/oauth.py (3)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
set_token_expiry_date(461-462)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (2)
set_token_expiry_date(123-124)set_token_expiry_date(304-313)airbyte_cdk/utils/datetime_helpers.py (1)
AirbyteDateTime(92-342)
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (3)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (4)
set_token_expiry_date(461-462)access_token(490-491)access_token(495-496)refresh_access_token(133-145)airbyte_cdk/sources/declarative/auth/oauth.py (3)
set_token_expiry_date(242-243)access_token(267-270)access_token(273-274)airbyte_cdk/utils/datetime_helpers.py (1)
AirbyteDateTime(92-342)
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/auth/test_oauth.py
[error] 1-1: Ruff formatting check failed. File would be reformatted.
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
[error] 1-1: Ruff formatting check failed. File would be reformatted.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (28)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
242-243: Confirm type consistency with parent authenticator?It looks like we're now expecting an
AirbyteDateTimeinstead ofUnion[str, int]. Do we want to double-check if there's a matching signature in the parent class to avoid future type conflicts, or is this fully aligned now? Wdyt?airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (2)
123-124: Keep signature consistent with parent class?We're passing an
AirbyteDateTimedirectly. The parent abstract authenticator also usesAirbyteDateTime, so this looks good. Do you see any potential edge cases where we might need to parse other data types, or is this the final approach? Wdyt?
335-335: Review method signature mismatch with parent?The parent’s
refresh_access_tokenappears to return a 2-element tuple(str, AirbyteDateTime), but we return a 3-element tuple here. Is the# type: ignore[override]intentional to skip a type check? Would it be simpler to unify the signatures, or is this approach best for your use case? Wdyt?unit_tests/sources/declarative/auth/test_oauth.py (5)
1-1: Fix Ruff formatting?The pipeline indicates that Ruff formatting failed. Would you like to apply the standard suggestions from your linter (e.g.,
ruff --fix) to address the formatting issues? Wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. File would be reformatted.
206-206: Use a frozen time for deterministic tests!Freezing time to "2022-01-01" helps ensure consistent, reproducible tests. This looks good. Anything else you'd like to test with different freeze dates? Wdyt?
237-237: Consistent freeze_time usage.Using the freeze_time decorator again keeps things consistent and predictable. Looks great to me. Any alternate test dates you'd consider? Wdyt?
321-321: Freeze_time keeps tests deterministic.Another freeze_time usage. Looks clean. Would you like to unify this with a global fixture, or is per-test usage your preference? Wdyt?
494-494: More deterministic testing via freeze_time.Again, this decorator ensures stable results in
test_profile_assertion. Everything seems well-structured. Good to go?unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (20)
7-7: Import is now used, so it’s no longer an issue.This import was previously flagged as unused, but it's referenced multiple times. Looks good now. Wdyt?
86-86: Time-freezing looks great.Freezing time at "2022-01-01" keeps tests deterministic. Wdyt?
126-130: Good expired-token scenario.Defining
already_expiredhelps verify that new tokens get requested when needed. Wdyt?
134-134: Another clear mock for partial expiry.Using a 100-second window is a neat boundary test. Looks good. Wdyt?
278-280: Accurate assertion for token expiration.Ensuring both the token and
expires_inare validated is a solid check. Wdyt?
288-290: String-basedexpires_inhandling well tested.This ensures that numerical or string forms are both supported. Wdyt?
298-301: Great coverage for unrecognized expiry format.Raising a
ValueErrorwhenexpires_inis a datetime string but the flag is false is a key edge case. Wdyt?
302-307: Good “no expires_in” handling test.This verifies existing valid tokens remain in effect if no
expires_infield is supplied. Wdyt?
309-314: Confirmation of unchanged expiry date.Nice to see a test ensuring we don’t overwrite a non-expired token’s expiry. Wdyt?
316-321: Fallback to 1-hour default.This precisely checks the fallback logic for already expired tokens. Wdyt?
331-333: Deeply nested token fields validated.The test’s thoroughness for nested
expires_inandaccess_tokenis commendable. Wdyt?
438-447: Excellent parameterized edge cases.Covering
Noneor invalid strings ensures robust fallback logic. Wdyt?
482-484: Good check of final parsed values.Verifying both
expires_datetimeandtokenis consistent with your approach. Wdyt?
503-506: Retry mechanism validated.Testing multiple failing responses before a success proves retry logic is working. Wdyt?
517-519: Clear approach to keep test coverage consistent.Mocking
refresh_access_tokenfor a new expiry time is helpful to confirm the pipeline. Wdyt?
583-583: Time freezing for second test class.Freezing to "2022-12-31" ensures deterministic handling for single-use refresh tokens. Wdyt?
603-603: Explicit token refresh endpoint.Defining
"https://refresh_endpoint.com"is consistent with earlier usage—nice clarity. Wdyt?
637-638: Dynamic interpretation of expiry date.Using
token_expiry_date_formatandtoken_expiry_is_time_of_expirationis a smart distinction. Wdyt?
640-645: Mocked refresh endpoint response.This block ensures comprehensive coverage of newly returned expiry values. Wdyt?
677-682: Additional coverage with new refresh endpoint mocks.The multiple lines of mocking illustrate how the token refresh flow is validated. Wdyt?
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
Show resolved
Hide resolved
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
Show resolved
Hide resolved
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Outdated
Show resolved
Hide resolved
fixes airbytehq/airbyte#54198
The proposal is to create a default expiration (currently 1 hour) that we use if we do not receive a token expiration back from the resource server when requesting / refreshing access tokens. This number is somewhat arbitrary but set to be a middle ground to avoid overly aggressive refreshes and a case where an access token expires before our fabricated expiration.
a future improvement would be to check several places for the token expiration (potentially another API endpoint or within the access token JWT)
Also, in writing this, I opted to try to make as small a change as possible to minimally interfere with the existing code paths. As a result I updated the extraction method as opposed to adding code to an existing method that does expiration date parsing
_parse_token_expiration_datewhich is not used by all code paths.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests