SnowflakeHook: extract OAuth token lifecycle management into dedicated helper

[SameerMesiah97]

Description

This change refactors OAuth token handling in SnowflakeHook by extracting token lifecycle responsibilities into a dedicated _SnowflakeOAuthManager helper.

OAuth-specific functionality previously implemented directly in SnowflakeHook, including grant type validation, token retrieval, token caching, and token expiration tracking, has been moved into the new helper class. SnowflakeHook continues to expose the existing get_oauth_token() interface, but now delegates OAuth token management to the helper.

Rationale

SnowflakeHook previously mixed connection management responsibilities with OAuth-specific concerns. Extracting OAuth token lifecycle management into _SnowflakeOAuthManager establishes a clearer separation of concerns and provides a dedicated owner for token acquisition, refresh, validation, and cache management.

This reduces the amount of authentication-specific state maintained by SnowflakeHook, simplifies the hook implementation, and makes OAuth functionality easier to reason about and maintain independently.

Tests

• Updated OAuth-related tests to reflect the new ownership boundaries where appropriate.

Notes

• Updated the _get_static_conn_params() docstring to reflect the new ownership boundaries introduced by _SnowflakeOAuthManager.

Backwards Compatibility

This change is a refactor only and does not modify public APIs or runtime behavior. SnowflakeHook continues to expose the existing get_oauth_token() interface, and existing authentication flows remain unchanged.

[phanikumv]

still see some reference to _get_valid_oauth_token in the test_snowflake.py, could you please correct it

[jason810496]

Thanks for the PR. Sorry for pushing back, but I'm not sure the extraction earns its keep.

_SnowflakeOAuthManager has a single consumer. SnowflakeHook creates it once, it's private, and it isn't injected or swappable, so we pay for the indirection without the usual payoffs (reuse, isolation, substitutability).

The boundary is also leaky. Every method still takes conn_config and indexes into it (account, client_id, refresh_token...), and validate_grant_type still runs in _get_conn_params then again inside get_valid_oauth_token. That duplicate validation is relocated, not fixed.

It's behavior-preserving, so nothing breaks, but it's mostly lateral churn on recently landed code (#60027), and the tests now reach into hook._oauth.validate_grant_type, coupling to internals the PR is trying to hide.

If there's a concrete second consumer or isolation benefit in mind, I'd be glad to hear it. Otherwise I'd lean toward holding off.

[SameerMesiah97]

Thanks for the PR. Sorry for pushing back, but I'm not sure the extraction earns its keep.

_SnowflakeOAuthManager has a single consumer. SnowflakeHook creates it once, it's private, and it isn't injected or swappable, so we pay for the indirection without the usual payoffs (reuse, isolation, substitutability).

The boundary is also leaky. Every method still takes conn_config and indexes into it (account, client_id, refresh_token...), and validate_grant_type still runs in _get_conn_params then again inside get_valid_oauth_token. That duplicate validation is relocated, not fixed.

It's behavior-preserving, so nothing breaks, but it's mostly lateral churn on recently landed code (#60027), and the tests now reach into hook._oauth.validate_grant_type, coupling to internals the PR is trying to hide.

If there's a concrete second consumer or isolation benefit in mind, I'd be glad to hear it. Otherwise I'd lean toward holding off.

After considering your argument, I have to agree now that adding a new internal helper class is difficult to justify considering that it is only used by one other class. The motivation for this PR was to group all Oauth related functionality in one part of the hook to improve maintainability and this naturally led me to consider creating _SnowflakeOAuthManager. What are your thoughts on simply grouping all Oauth related methods in one part of the hook? I would not open a PR just for something like that but since this has already been opened, are you fine with me changing this PR to group all the Oauth methods within SnowflakeHook?

[SameerMesiah97]

@jason810496

Whilst your argument is valid considering the current state of the provider, you may have to keep in mind that I am currently in process of adding a new hook for Snowflake Cortex Agents (PR #68942). Would it be better to wait until that is reviewed as SnowflakeCortexAgentHook is currently reaching into SnowflakeHook internals? Perhaps, the utility of this new internal helper would be more readily apparent when there is another hook that needs it.

[jason810496]

you may have to keep in mind that I am currently in process of adding a new hook for Snowflake Cortex Agents (PR #68942).

Then it makes sense now, I will merge this one first so your new Snowflake Cortex Agents hook can rebase on top of this and reuse the _SnowflakeOAuthManager util. Thanks.