fix: prevent 500 error on partial update of multivariate options - #8309
fix: prevent 500 error on partial update of multivariate options#8309srijantrpth wants to merge 7 commits into
Conversation
|
@srijantrpth is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe serializer now supports partial updates for multivariate feature options. It uses existing instance values when PATCH data omits the feature or allocation. Key uniqueness validation also uses the existing feature. A view test covers a partial key update and confirms that the percentage allocation remains unchanged. Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change prevents partial multivariate-option updates from returning a 500 error while preserving existing values; no actionable merge-blocking risk remains after normal checks and review. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1dd16f9-b08c-4af5-97d0-0f21c05b367f
📒 Files selected for processing (2)
api/features/multivariate/serializers.pyapi/tests/unit/features/multivariate/test_unit_multivariate_views.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
♻️ Duplicate comments (2)
api/features/multivariate/serializers.py (1)
148-149:⚠️ Potential issue | 🟠 MajorResolve the existing key before applying this feature fallback.
When a PATCH changes
featurebut omits a non-nullkey,_validate_key_is_uniquereturns at Lines 145-147 before Lines 148-149 run. The existing key is not checked against the destination feature. This can violateunique_together = ("feature", "key")and produce a 500 instead of a serializer validation error. Resolvekeyfromattrsorself.instancebefore theNonecheck, and add a feature-only conflicting-key regression test.This is based on the uniqueness contract in
api/features/multivariate/models.pyand the writablefeaturePATCH path inapi/features/multivariate/views.py.api/tests/unit/features/multivariate/test_unit_multivariate_views.py (1)
137-137:⚠️ Potential issue | 🟡 MinorAdd the final newline.
Ruff 0.16.1 reports W292 at Line 137. Add a newline at the end of
api/tests/unit/features/multivariate/test_unit_multivariate_views.py.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 360a512e-20b7-456e-b757-e96f8211284e
📒 Files selected for processing (2)
api/features/multivariate/serializers.pyapi/tests/unit/features/multivariate/test_unit_multivariate_views.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8309 +/- ##
==========================================
- Coverage 98.78% 98.64% -0.14%
==========================================
Files 1604 1604
Lines 64335 64344 +9
==========================================
- Hits 63551 63470 -81
- Misses 784 874 +90 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hello @srijantrpth, thanks for your contribution. Can you fix the remaining linting error? |
|
Hi @khvn26 , Thanks for the review! I just pushed a commit to fix that linting error by renaming the test to match the required test_{subject}underscore underscore{condition}underscore underscore {expected} convention. Let me know if everything looks good now! |
| url = reverse( | ||
| "api-v1:projects:feature-mv-options-detail", | ||
| args=[project.id, multivariate_feature.id, mv_option.id], | ||
| ) |
There was a problem hiding this comment.
Sorry. See the url variable in test_list_mv_options__feature_in_other_project__returns_404 as an example.
There was a problem hiding this comment.
Hey @khvn26, I think the link you added for 'here' might be broken it just loops me back to this exact comment! Could you share the link to the example you meant to show me so I can get this updated?
There was a problem hiding this comment.
Ah, got it! Thanks for pointing me to that example. I've updated it to use the f-string URL pattern.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8306
Resolved an issue where partially updating a multivariate option via a
PATCHrequest returned a 500 Internal Server Error when omitting fields likedefault_percentage_allocationorfeature.Updated
MultivariateFeatureOptionSerializer.validate()and_validate_key_is_unique()inapi/features/multivariate/serializers.pyto safely extract these fields using.get()and fallback toself.instancevalues, preventing the unhandledKeyError.How did you test this code?
Added a unit test (
test_partially_updating_multivariate_option_success) inapi/tests/unit/features/multivariate/test_unit_multivariate_views.pyto verify that sending aPATCHrequest with only thekeyfield correctly returns a 200 OK and preserves the existingdefault_percentage_allocationwithout throwing an error. Tested successfully via pytest.