[Tech Debt] Metadata implementation is fragile and error-prone - needs architectural redesign

[gagantrivedi]

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 367edf4), 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: Creating feature with metadata removes metadata from all existing features #6170
• Bug fix: fix: prevent metadata loss due to bad request #6172 (commit 91dc078)
• Segments metadata addition: commit 367edf4 (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

[emyller]

I like the proposed solution.

[emyller]

@gagantrivedi I've assigned this issue to myself, in hopes I will get the chance to tackle it in some sprint soon (cc @matthewelwell), unless you want to deal with this yourself. Feel free to reassign if appropriate.

[gagantrivedi]

@gagantrivedi I've assigned this issue to myself, in hopes I will get the chance to tackle it in some sprint soon (cc @matthewelwell), unless you want to deal with this yourself. Feel free to reassign if appropriate.

I created a POC implementation for the feature here: #6278. Feel free to build on top of it if you like.

[matthewelwell]

@gagantrivedi @emyller I think we have bigger tech debt items to focus on than this one at this point in time, let's not spend the effort here unless we really need to.

[gagantrivedi]

@gagantrivedi @emyller I think we have bigger tech debt items to focus on than this one at this point in time, let's not spend the effort here unless we really need to.

I’d say it’s a 2-pointer for me now, considering the effort I’ve already invested in this. I’d really like to get this over the line.