Skip to content

fix: add null validation for environment in FeatureStateSerializerBasic#7099

Open
Pwongchaiya wants to merge 2 commits intoFlagsmith:mainfrom
Pwongchaiya:minor-fix/errorcode500-on-null-env
Open

fix: add null validation for environment in FeatureStateSerializerBasic#7099
Pwongchaiya wants to merge 2 commits intoFlagsmith:mainfrom
Pwongchaiya:minor-fix/errorcode500-on-null-env

Conversation

@Pwongchaiya
Copy link
Copy Markdown

@Pwongchaiya Pwongchaiya commented Apr 1, 2026

Summary

  • Added validation in FeatureStateSerializerBasic.validate_environment to raise a ValidationError when the environment is None, preventing a 500 error response.

Test plan

  • Verify that sending a request with a null environment returns a 400 with a descriptive error message instead of a 500.
  • Verify existing environment validation (cannot change environment of a feature state) still works correctly.

🤖 Generated with Claude Code

@Pwongchaiya Pwongchaiya requested a review from a team as a code owner April 1, 2026 22:21
@Pwongchaiya Pwongchaiya requested review from gagantrivedi and removed request for a team April 1, 2026 22:21
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 1, 2026

@Pwongchaiya is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the api Issue related to the REST API label Apr 1, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.26%. Comparing base (0d68a05) to head (01607d6).
⚠️ Report is 82 commits behind head on main.

Files with missing lines Patch % Lines
api/features/serializers.py 50.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7099      +/-   ##
==========================================
- Coverage   98.31%   98.26%   -0.06%     
==========================================
  Files        1335     1335              
  Lines       49760    49818      +58     
==========================================
+ Hits        48923    48952      +29     
- Misses        837      866      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gagantrivedi

This comment was marked as resolved.

Copy link
Copy Markdown
Member

@gagantrivedi gagantrivedi left a comment

Choose a reason for hiding this comment

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

Thanks for the fix — this is a real bug and worth addressing! A couple of suggestions:

Consider overriding the field instead of adding a validator check

The environment FK on the model has null=True, so DRF auto-generates the serializer field with allow_null=True. Since this serializer always expects a real environment (the validate() method falls back to self.context["environment"]), you could disallow null at the field level instead:

class FeatureStateSerializerBasic(WritableNestedModelSerializer):
    environment = serializers.PrimaryKeyRelatedField(
        queryset=Environment.objects.all(),
        allow_null=False,
    )

This way DRF handles the null rejection automatically with a standard error message, and validate_environment only needs to deal with the "cannot change environment" logic.

Tests needed

The project requires 100% diff coverage for backend PRs, so this will need at least one test — e.g. sending "environment": null and asserting a 400 response.

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

Labels

api Issue related to the REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants