Flagsmith / Flagsmith/flagsmith

Metadata implementation is fragile and error-prone - needs architectural redesign

Open
#6,277 5 comments 1 reaction 1 assignee Claimed by @emyller View on GitHub
tech-debt
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.