Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

source-stripe-native: Support Stripe Connected Accounts #2467

Merged
merged 2 commits into from
Mar 7, 2025

Conversation

JustinASmith
Copy link
Contributor

@JustinASmith JustinASmith commented Feb 28, 2025

Description:

  • Enhanced open_binding function to support dictionary-based fetch functions for fetch_changes, fetch_page, and fetch_snapshot. This allows each function to run as a separate subtask with its own independent state.
  • Updated source-stripe-native to use the CDK feature to capture data for connected accounts when enabled in the EndpointConfig.

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@JustinASmith JustinASmith force-pushed the jsmith/stripe-native-connected-accounts branch 7 times, most recently from 588a43d to eb9ac3b Compare March 4, 2025 15:46
@JustinASmith JustinASmith marked this pull request as ready for review March 4, 2025 16:44
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good; made a few comments for consideration, nothing major.

@JustinASmith JustinASmith force-pushed the jsmith/stripe-native-connected-accounts branch from eb9ac3b to 8213252 Compare March 5, 2025 17:41
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Enhance the capture common module to support running multiple subtasks for incremental and backfill captures with independent states. This allows more flexible capture configurations where different subtasks can track their own progress and state within a single binding.
@JustinASmith JustinASmith force-pushed the jsmith/stripe-native-connected-accounts branch from 8213252 to c943ff8 Compare March 7, 2025 14:22
Comment on lines +51 to +79
if (
not validate.lastCapture
or (
validate.lastCapture.config.config.get("capture_connected_accounts")
== validate.config.capture_connected_accounts
)
or not validate.lastCapture.bindings
):
resources = await all_resources(log, self, validate.config)
resolved = common.resolve_bindings(validate.bindings, resources)
return common.validated(resolved)

prev_bindings_by_name = {
binding.resourceConfig.name: binding.backfill
for binding in validate.lastCapture.bindings
}

for current_binding in validate.bindings:
resource_name = current_binding.resourceConfig.name
if (
resource_name in prev_bindings_by_name
and current_binding.backfill <= prev_bindings_by_name[resource_name]
):
raise ValidationError(
[
"Cannot change the `capture_connected_accounts` property without backfilling all collections again."
]
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation logic to require a backfill of all collections when the capture_connected_accounts endpoint config property changes

Comment on lines +40 to +52
class ValidateLastCaptureEndpointConfig(BaseModel):
config: dict[str, Any]


class ValidateLastCapture(ValidateBase, Generic[EndpointConfig, ResourceConfig]):
config: ValidateLastCaptureEndpointConfig


class Validate(ValidateBase, Generic[EndpointConfig, ResourceConfig]):
config: EndpointConfig
lastCapture: ValidateLastCapture[EndpointConfig, ResourceConfig] | None = None


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated Validate models to include lastCapture spec. Currently uses a work around due to issues serializing lastCapture into the EndpointConfig model.

Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates LGTM! I think this is fine as-is, with the workaround for the lastCapture issue. That's probably worth looking into more, but for now this seems good.

Enhance the Stripe source connector to support capture across multiple connected accounts. Key changes include:

- Add account_id field to Stripe object models
- Fetch connected account and platform account IDs during resource discovery
- Modify incremental and backfill fetch methods to support account-specific replication using `Stripe-Account` HTTP header
- Update resource generation to create subtasks for each connected account each with their own state
- Added validation to require backfilling all collections when `capture_connected_accounts` property changes
@JustinASmith JustinASmith force-pushed the jsmith/stripe-native-connected-accounts branch from c943ff8 to ceb9c3b Compare March 7, 2025 15:22
@JustinASmith JustinASmith merged commit ebc98fe into main Mar 7, 2025
87 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants