Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

fix(cohorts): return 400 for malformed legacy groups payload#70631

Merged
gustavohstrassburger merged 2 commits into
masterPostHog/posthog:masterfrom
posthog-code/cohort-groups-400-validationPostHog/posthog:posthog-code/cohort-groups-400-validationCopy head branch name to clipboard
Jul 24, 2026
Merged

fix(cohorts): return 400 for malformed legacy groups payload#70631
gustavohstrassburger merged 2 commits into
masterPostHog/posthog:masterfrom
posthog-code/cohort-groups-400-validationPostHog/posthog:posthog-code/cohort-groups-400-validationCopy head branch name to clipboard

Conversation

@posthog

@posthog posthog Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Problem

A malformed legacy groups payload in the cohort API threw an uncaught ValueError and returned a 500 instead of a clean 400.

The create/update endpoint exposes the legacy groups field on CohortSerializer with no validation. When a request includes a group that has none of properties, action_id, or event_id, CohortManager.create calls Group(**group), and Group.__init__ raises a plain ValueError. DRF only converts ValidationError into a 400, so this escaped as an HTTP 500 out of perform_create. Low volume, but a real user-triggerable server error rather than a proper client-error response.

Changes

Added a validate_groups method to CohortSerializer that constructs each group through Group(...) and re-raises the resulting ValueError/TypeError as a DRF ValidationError. Bad input now returns a 400 with a helpful message on both create and update.

How did you test this code?

Added one endpoint test (test_creating_cohort_with_malformed_group_returns_400) that posts a group missing all of properties/action_id/event_id and asserts a 400 — locking in the 500→400 fix; no existing test exercised the malformed-group path. Ran ruff check and ruff format --check (both clean). The local sandbox lacks a fully provisioned Django/test runtime, so I was not able to execute the pytest suite here; the test should be run in CI.

🤖 Agent context

Autonomy: Fully autonomous

Authored by PostHog Code (Claude Opus 4.8). Invoked the /improving-drf-endpoints and /writing-tests skills while shaping the change. Chose field-level validate_groups reusing the real Group(...) constructor over a hand-rolled key check, so the validation stays in lockstep with what creation actually accepts and also hardens the update path.

Created with PostHog Code from an inbox report.

@posthog
fix(cohorts): return 400 for malformed legacy groups payload
dcc77f5 
A legacy `groups` entry with none of properties/action_id/event_id raised
an uncaught ValueError from Group.__init__, surfacing as an HTTP 500 out of
the cohort create/update flow. Validate `groups` in CohortSerializer so bad
input is rejected with a clean 400 instead.

Generated-By: PostHog Code
Task-Id: 3abb5c2c-c9e5-4e94-8093-ade2462739d2
@trunk-io

trunk-io Bot commented Jul 14, 2026

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@scheduled-actions-posthog

scheduled-actions-posthog Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

@github-actions

github-actions Bot commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Playwright — all passed

All tests passed.

View test results →

ℹ️ ClickHouse migration SQL — 1 migration(s)

ClickHouse migration SQL per cloud environment

  • unset
    • all 
      CREATE OR REPLACE VIEW custom_metrics_test
    AS SELECT
        'ClickHouseCustomMetric_Test' AS name,
        map('instance', hostname()) AS labels,
        1 AS value,
        'Test to check that the metric endpoint is working' AS help,
        'gauge' AS type
      CREATE OR REPLACE VIEW custom_metrics
    AS SELECT * REPLACE (toFloat64(value) as value)
    FROM custom_metrics_test
  • US, EU, DEV
    • events, shufflehog 
      CREATE OR REPLACE VIEW custom_metrics_test
    AS SELECT
        'ClickHouseCustomMetric_Test' AS name,
        map('instance', hostname()) AS labels,
        1 AS value,
        'Test to check that the metric endpoint is working' AS help,
        'gauge' AS type
      CREATE OR REPLACE VIEW custom_metrics
    AS SELECT * REPLACE (toFloat64(value) as value)
    FROM custom_metrics_test

@gustavohstrassburger gustavohstrassburger self-assigned this Jul 24, 2026
@gustavohstrassburger gustavohstrassburger moved this to In Progress in Feature Flags Jul 24, 2026
@gustavohstrassburger
gustavohstrassburger marked this pull request as ready for review July 24, 2026 13:26
Copilot AI review requested due to automatic review settings July 24, 2026 13:26
@github-project-automation github-project-automation Bot moved this from In Progress to Approved in Feature Flags Jul 24, 2026
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team July 24, 2026 13:26
@posthog-project-board-bot posthog-project-board-bot Bot moved this from Approved to In Review in Feature Flags Jul 24, 2026
Copilot AI reviewed Jul 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds serializer-level validation for legacy cohort groups payloads so malformed input (previously raising a plain ValueError in Group.__init__) returns a client-facing HTTP 400 instead of an uncaught 500, and locks the behavior in with an endpoint test.

Changes:

  • Added CohortSerializer.validate_groups to pre-validate legacy groups entries by constructing Group(**group) and converting ValueError/TypeError into DRF ValidationError.
  • Added an API test ensuring a malformed legacy group returns HTTP 400 with an explanatory message.
  • Imported Group into posthog/api/cohort.py for validation reuse.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
posthog/api/cohort.py Adds serializer validation for legacy groups so malformed payloads return 400 instead of 500.
posthog/api/test/test_cohort.py Adds regression test covering the malformed legacy groups payload case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread posthog/api/cohort.py
@gustavohstrassburger
fix(cohorts): only skip groups validation for None, not any falsy value
3e16c77 
The validate_groups guard returned early on any falsy value, so an empty
string or 0 skipped the type check and was persisted as-is, reintroducing
the 500/corruption path the validation was meant to prevent. Guard on None
only and include the failing group index in error messages.

Generated-By: PostHog Code
Task-Id: 5b1ff18d-e5b5-4978-837f-3978f6fc47f3
@gustavohstrassburger
gustavohstrassburger merged commit 99dc1c3 into master Jul 24, 2026
244 checks passed
@gustavohstrassburger
gustavohstrassburger deleted the posthog-code/cohort-groups-400-validation branch July 24, 2026 17:48
@github-project-automation github-project-automation Bot moved this from In Review to Done in Feature Flags Jul 24, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-07-24 18:25 UTC Run
prod-us ✅ Deployed 2026-07-24 18:46 UTC Run
prod-eu ✅ Deployed 2026-07-24 18:48 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gustavohstrassburger gustavohstrassburger gustavohstrassburger approved these changes
Copilot code review Copilot Copilot left review comments

Assignees

@gustavohstrassburger gustavohstrassburger

Labels

feature/cohorts Feature Tag: Cohorts team/feature-flags

Projects

Feature Flags
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gustavohstrassburger
Morty Proxy This is a proxified and sanitized view of the page, visit original site.