Flagsmith / Flagsmith/flagsmith
Metadata implementation is fragile and error-prone - needs architectural redesign
- Dominant language
- Python
- Stars
- 6.6k
- Forks
- 567
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 121
Description
## Summary
The metadata implementation suffers from shallow abstractions that led to a critical bug (#6170, fixed in #6172) where metadata was lost when creating features/environments. While the tactical fix prevents data loss, the underlying architecture remains
fragile and error-prone.
**Problem**: The current `MetadataSerializerMixin` requires developers to manually coordinate 6 separate pieces with no enforcement mechanisms, making it easy to introduce silent bugs.
Current Design Problems
1. Shallow Abstraction - High Cognitive Load
To use MetadataSerializerMixin correctly, developers must remember:
1. Add metadata = MetadataSerializer(required=False, many=True) field
2. Override validate() to call _validate_required_metadata()
3. Override create() to pop metadata from validated_data
4. Call _update_metadata() in create()
5. Override update() to pop metadata from validated_data
6. Call _update_metadata() in update()
No enforcement: Forgetting to override create() causes silent data corruption, not an error.
Evidence: Segments serializer got it right from the start (commit 367edf4877), but Features/Environments didn't until the bug was discovered months later. This shows the design relies on tribal knowledge.
2. Boilerplate Repetition
Same ~20 lines repeated in 3 serializers:
- features/serializers.py:347-378
- environments/serializers.py:77-106
- segments/serializers.py:81-137
# Repeated in EVERY serializer with metadata:
```
def validate(self, attrs):
attrs = super().validate(attrs)
organisation = # ... get organisation logic ...
self._validate_required_metadata(organisation, attrs.get("metadata", []))
return attrs
def create(self, validated_data):
metadata_data = validated_data.pop("metadata", [])
instance = super().create(validated_data)
self._update_metadata(instance, metadata_data)
return instance
def update(self, instance, validated_data):
metadata = validated_data.pop("metadata", [])
instance = super().update(instance, validated_data)
self._update_metadata(instance, metadata)
return instance
```
3. Fighting the Framework
The validated_data.pop("metadata", []) pattern disables DRF's nested serializer handling because it doesn't work correctly with GenericForeignKey. This is an impedance mismatch between the abstraction and the framework.
4. Inefficient Delete-Before-Create Pattern
_update_metadata() deletes ALL metadata then recreates it on every update:
- ID instability: Metadata IDs change on every update
- Inefficient: Deletes and recreates even when only one field changed
5. No Defense Against Misuse
- No runtime check: "Does this metadata ID belong to this parent object?"
- No validation: "Is this metadata already attached to another object?"
- No warning: "You're using MetadataSerializerMixin but didn't override create()"
- GenericForeignKey allows IDs to be confused between model types
Proposed Solution
Decorator Pattern
Eliminate boilerplate and MRO fragility:
```
def with_metadata(get_organisation_fn):
"""
Decorator that adds metadata handling to a serializer.
Usage:
@with_metadata(lambda self, attrs: self.context['project'].organisation)
class FeatureSerializer(CreateFeatureSerializer):
metadata = MetadataSerializer(required=False, many=True)
"""
def decorator(cls):
original_create = cls.create
original_update = cls.update
original_validate = cls.validate
def create(self, validated_data):
metadata_data = validated_data.pop("metadata", [])
instance = original_create(self, validated_data)
MetadataManager.sync_metadata(instance, metadata_data)
return instance
def update(self, instance, validated_data):
metadata_data = validated_data.pop("metadata", [])
instance = original_update(self, instance, validated_data)
MetadataManager.sync_metadata(instance, metadata_data)
return instance
def validate(self, attrs):
attrs = original_validate(self, attrs)
organisation = get_organisation_fn(self, attrs)
_validate_required_metadata(self, organisation, attrs.get("metadata", []))
return attrs
cls.create = create
cls.update = update
cls.validate = validate
return cls
return decorator
```
# Usage (clean and explicit):
@with_metadata(lambda self, attrs: self.context["project"].organisation)
class FeatureSerializer(CreateFeatureSerializer):
metadata = MetadataSerializer(required=False, many=True)
# That's it! No methods to override.
Benefits:
- ✅ No MRO fragility (works with any inheritance)
- ✅ Explicit (you can SEE metadata handling is added)
- ✅ One line to add metadata support
- ✅ No boilerplate to maintain
- ✅ Impossible to forget - decorator does everything
References
- Bug report: #6170
- Bug fix: #6172 (commit 91dc078f9)
- Segments metadata addition: commit 367edf4877 (got it right from start)
- Affected files:
- metadata/models.py:103-127
- metadata/serializers.py:91-152
- features/serializers.py:347-378
- environments/serializers.py:77-106
- segments/serializers.py:81-137
Contributor guide
Assessment
This issue has not been assessed yet.