refactor(data-import): refactor source credential handlers by EDsCODE · Pull Request #30260 · PostHog/posthog

EDsCODE · 2025-03-21T00:18:53Z

Problem

source handlers growing out of control
no diff check api

Changes

refactor to handler pattern
add diff check api and tests

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

greptile-apps

PR Summary

This PR introduces a significant refactoring of data source handlers in PostHog, implementing a standardized pattern for managing external data source integrations.

Key changes:

Introduces base SourceHandler class in /pipelines/source/handlers.py to standardize credential validation and schema retrieval
Adds specialized handlers for multiple data sources (Stripe, Hubspot, BigQuery, etc.) with consistent validation patterns
Implements SSH tunnel support and improved error handling in SQL database handlers
Adds schema diff checking capabilities to detect changes between source and database
Consolidates common validation patterns and removes redundant code across handlers

The changes appear well-structured and improve maintainability, though there are some potential concerns around error handling consistency and edge cases in schema validation.

_{12 file(s) reviewed, 18 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-03-21T00:19:25Z

posthog/temporal/data_imports/pipelines/chargebee/handlers.py

+        api_key = self.request_data.get("api_key", "")
+        site_name = self.request_data.get("site_name", "")


style: consider trimming whitespace from api_key and site_name inputs, as seen in test_trimming_payload

Suggested change

api_key = self.request_data.get("api_key", "")

site_name = self.request_data.get("site_name", "")

api_key = self.request_data.get("api_key", "").strip()

site_name = self.request_data.get("site_name", "").strip()

greptile-apps · 2025-03-21T00:19:28Z

posthog/temporal/data_imports/pipelines/salesforce/handlers.py

+        if not salesforce_integration_id:
+            return False, "Missing required parameters: Salesforce integration ID"


style: Error message inconsistent with other handlers - should be 'Invalid credentials: Salesforce integration ID is missing' to match pattern

greptile-apps · 2025-03-21T00:19:29Z

posthog/temporal/data_imports/pipelines/bigquery/handlers.py

+                    for column_name, column_type in columns
+                ],
+                "incremental_available": True,
+                "incremental_field": columns[0][0] if len(columns) > 0 and len(columns[0]) > 0 else None,


style: Assumes first column is always suitable for incremental field. Should validate column type is appropriate for incremental updates.

greptile-apps · 2025-03-21T00:19:54Z

posthog/temporal/data_imports/pipelines/snowflake/handlers.py

+        auth_type_obj = self.request_data.get("auth_type", {})
+        auth_type = auth_type_obj.get("selection", None)
+        auth_type_username = auth_type_obj.get("username", None)


logic: auth_type is allowed to be None here, but there's no validation to ensure a valid auth_type is provided. This could lead to authentication failures.

Suggested change

auth_type_obj = self.request_data.get("auth_type", {})

auth_type = auth_type_obj.get("selection", None)

auth_type_username = auth_type_obj.get("username", None)

auth_type_obj = self.request_data.get("auth_type", {})

auth_type = auth_type_obj.get("selection", None)

if auth_type not in ["password", "keypair"]:

return False, "Invalid auth_type: must be 'password' or 'keypair'"

auth_type_username = auth_type_obj.get("username", None)

greptile-apps · 2025-03-21T00:19:54Z

posthog/temporal/data_imports/pipelines/stripe/handlers.py

+
+class StripeSourceHandler(SourceHandler):
+    def validate_credentials(self) -> tuple[bool, str | None]:
+        key = self.request_data.get("stripe_secret_key", "")


logic: Missing key existence check before validation. Empty string will be used if key doesn't exist.

greptile-apps · 2025-03-21T00:20:23Z

posthog/temporal/data_imports/pipelines/vitally/handlers.py

+        subdomain_regex = re.compile("^[a-zA-Z-]+$")
+        if region == "US" and not subdomain_regex.match(subdomain):


logic: Subdomain regex will reject valid subdomains containing numbers. Consider using ^[a-zA-Z0-9-]+$ instead

Suggested change

subdomain_regex = re.compile("^[a-zA-Z-]+$")

if region == "US" and not subdomain_regex.match(subdomain):

subdomain_regex = re.compile("^[a-zA-Z0-9-]+$")

if region == "US" and not subdomain_regex.match(subdomain):

greptile-apps · 2025-03-21T00:20:24Z

posthog/temporal/data_imports/pipelines/vitally/handlers.py

+        if region == "US" and not subdomain_regex.match(subdomain):
+            return False, "Invalid credentials: Vitally subdomain is incorrect"


logic: Subdomain validation only happens for US region. Should validate for all regions or document why US-only

greptile-apps · 2025-03-21T00:20:26Z

posthog/temporal/data_imports/pipelines/zendesk/handlers.py

+        api_key = self.request_data.get("api_key", "")
+        email_address = self.request_data.get("email_address", "")
+
+        subdomain_regex = re.compile("^[a-zA-Z-]+$")


logic: regex pattern could be too restrictive - some Zendesk subdomains may contain numbers

Suggested change

subdomain_regex = re.compile("^[a-zA-Z-]+$")

subdomain_regex = re.compile("^[a-zA-Z0-9-]+$")

greptile-apps · 2025-03-21T00:20:27Z

posthog/temporal/data_imports/pipelines/zendesk/handlers.py

+        subdomain = self.request_data.get("subdomain", "")
+        api_key = self.request_data.get("api_key", "")
+        email_address = self.request_data.get("email_address", "")


style: consider validating that none of these required fields are empty before proceeding with validation

greptile-apps · 2025-03-21T00:20:45Z

posthog/warehouse/api/external_data_source.py

+            current_schemas = handler.get_schema_options()
+            current_schemas = [schema["table"] for schema in current_schemas]


logic: schema["table"] access could raise KeyError if schema options format changes

Suggested change

current_schemas = handler.get_schema_options()

current_schemas = [schema["table"] for schema in current_schemas]

current_schemas = handler.get_schema_options()

current_schemas = [schema.get("table") for schema in current_schemas if isinstance(schema, dict) and schema.get("table")]

posthog/warehouse/api/external_data_source.py

            return Response(
                status=status.HTTP_400_BAD_REQUEST,
-                data={"message": "Invalid parameter: source_type"},
+                data={"message": str(e)},


To fix the problem, we need to ensure that detailed exception messages are not exposed to the end user. Instead, we should log the detailed error message on the server and return a generic error message to the user. This can be achieved by modifying the exception handling block to log the exception and return a generic error message.

Specifically, we will:

Log the detailed exception message using logger.exception.

Return a generic error message in the HTTP response.

posthog/warehouse/api/external_data_source.py

+            logger.exception(f"Error checking schema changes for source type {source_type}", exc_info=e)
+            return Response(
+                status=status.HTTP_400_BAD_REQUEST,
+                data={"message": f"Error checking schema changes: {str(e)}"},


To fix the problem, we need to replace the detailed error message returned to the user with a more generic message. This can be done by modifying the response data in the exception handling block. The detailed error message should be logged using logger.exception, which already captures the stack trace.

Steps to fix:

Modify the response data in the exception handling block to return a generic error message.

Ensure that the detailed error message is logged for debugging purposes.

posthog-bot · 2025-03-31T07:32:22Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

posthog-bot · 2025-04-07T07:32:53Z

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

EDsCODE added 3 commits March 20, 2025 19:44

refactor to handler pattern

cd39409

rename

984b739

add check schema diff

9d39a2c

greptile-apps bot reviewed Mar 21, 2025

View reviewed changes

github-advanced-security bot found potential problems Mar 21, 2025

View reviewed changes

EDsCODE marked this pull request as draft March 21, 2025 13:16

posthog-bot added the stale label Mar 31, 2025

posthog-bot closed this Apr 7, 2025

@@ -978,5 +978,6 @@
                     except ValidationError as e:
+                        logger.exception(f"Validation error for source type {source_type}", exc_info=e)
                         return Response(
                             status=status.HTTP_400_BAD_REQUEST,
-                            data={"message": str(e)},
+                            data={"message": "Validation error occurred"},
                         )
@@ -987,3 +988,3 @@
                             status=status.HTTP_400_BAD_REQUEST,
-                            data={"message": f"Error handling source type {source_type}"},
+                            data={"message": "An internal error has occurred"},
                         )

		api_key = self.request_data.get("api_key", "")
		site_name = self.request_data.get("site_name", "")

		if not salesforce_integration_id:
		return False, "Missing required parameters: Salesforce integration ID"

		subdomain_regex = re.compile("^[a-zA-Z-]+$")
		if region == "US" and not subdomain_regex.match(subdomain):

		if region == "US" and not subdomain_regex.match(subdomain):
		return False, "Invalid credentials: Vitally subdomain is incorrect"

	subdomain_regex = re.compile("^[a-zA-Z-]+$")
	subdomain_regex = re.compile("^[a-zA-Z0-9-]+$")

		current_schemas = handler.get_schema_options()
		current_schemas = [schema["table"] for schema in current_schemas]

Conversation

EDsCODE commented Mar 21, 2025

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

posthog-bot commented Mar 31, 2025

Uh oh!

posthog-bot commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants