Conversions prevent Food Merge

[Svagtlys]

Tandoor Version

1.5.11

Setup

Docker / Docker-Compose

Reverse Proxy

Caddy

Other

Technically Caddy Docker Proxy by Lucas Lorentz

Bug description

When attempting to merge two foods that had been assigned conversions, I received either no error (when drag and drop merge was attempted) or the error "There was an error merging a resource!
An error occurred attempting to merge oregano with dried oregano" (when using the 3 dot menu on the food to merge). This occurs both when the conversions are different and when they are exactly the same.

Relevant logs

Traceback (most recent call last):
  File "/opt/recipes/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "f_unique_conversion_per_space"
DETAIL:  Key (space_id, base_unit_id, converted_unit_id, food_id)=(1, 2, 12, 221) already exists.


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/opt/recipes/cookbook/views/api.py", line 272, in merge
    r.save()
  File "/opt/recipes/venv/lib/python3.10/site-packages/django/db/models/base.py", line 814, in save
    self.save_base(
  File "/opt/recipes/venv/lib/python3.10/site-packages/django/

[vabene1111]

@smilerz you implemented merging in a very nice and generic way, I suspect its going to be tricky to get this to work right or do you think it could be added?

If not we will just override the merge method for the food model to delete the property entries of the merged (looser) food.

[smilerz]

I've been thinking about how to handle for a little bit and not sure how to address this case. Are there any other relationships that could cause duplicates like this?

[vabene1111]

Interestingly there are a few many to many fields on food, including all your inheritance fields. Property is the only one that has a specific trough relation model defined but that model has on_delete=models.CASCADE so deleting a food should not cause any issues with that relation.

Edit: but this wasn't about relation but uniqueness. I think the problem is related to the script trying to copy the values from that relation. and thus creating duplicates. I would say that we might already have this problem with all many to many relations (e.g. FoodInheritField) but they just don't enforce uniqueness. But there is no way to detect if copying a M2M relation is correct or if it would be correct to override it.

We could enforce custom implementations as soon as a M2M relation is there or we could only merge full sets of M2M. E.g. lets say we merge B onto A, we only merge B.some_set completely onto A.some_set if A.some_set is an empty set. If A.some_set is not empty B.some_set will just get deleted completely.

[vabene1111]

I think we actually have this piece of code in place to solve the properties issue but it might not be working

if isinstance(source, Food):
    source.properties.remove()

still we should probably look at the set logic

[vabene1111]

ok sorry i missread this the whole time, its about conversions not properties, though properties was also broken doubling relations and circumventing uniqueness.

I think I solved both for now using

  if isinstance(source, Food):
      source.properties.all().delete()
      source.properties.clear()
      UnitConversion.objects.filter(food=source).delete()

but merging stil probably needs some more attention or a dedicated method for each model to make sure strange things like this don't happen (although they could probably sneak in over time)