Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Agent merge failing unexpectedly #4685

Open
grantfitzsimmons opened this issue Mar 25, 2024 · 5 comments · May be fixed by #4699
Open

Agent merge failing unexpectedly #4685

grantfitzsimmons opened this issue Mar 25, 2024 · 5 comments · May be fixed by #4699
Assignees
@CarolineDenis
Labels
1 - Bug Incorrect behavior of the product 2 - Merging
Milestone
7.9.6

Comments

@grantfitzsimmons
Copy link
Contributor

grantfitzsimmons commented Mar 25, 2024 •
edited

Describe the bug
Merging these two agents is failing unexpectedly.

I’ve attached two error messages for merges I’ve attempted (Michael Verlander and Michael Todd). It looks like the foreign key constraint is causing the merge to fail. I believe I was successfully merging agents who were likely to have been considered catalogers before Christmas with no issue. Has there been a change to the system which may now be leading to this?

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://herb32124-edge.test.specifysystems.org/specify/overlay/merge/Agent/?records=299,11910
  2. Merge the two agents
  3. See error

Error:

{
   "message": "Traceback (most recent call last):
   File \"/opt/specify7/specifyweb/specify/record_merging.py\", line 47, in resolve_record_merge_response
   response = start_function()
   File \"/opt/specify7/specifyweb/specify/record_merging.py\", line 389, in <lambda>
   lambda: record_merge_fx(model_name, old_model_ids, int(new_model_id), progress, new_record_info))
   File \"/usr/lib/python3.8/contextlib.py\", line 75, in inner
   return func(*args, **kwds)
   File \"/opt/specify7/specifyweb/specify/record_merging.py\", line 315, in record_merge_fx
   target_model.objects.get(id=old_model_id).delete()
   File \"/opt/specify7/ve/lib/python3.8/site-packages/django/db/models/base.py\", line 966, in delete
   collector.collect([self], keep_parents=keep_parents)
   File \"/opt/specify7/ve/lib/python3.8/site-packages/django/db/models/deletion.py\", line 302, in collect
   raise ProtectedError(
   django.db.models.deletion.ProtectedError: (\"Cannot delete some instances of model 'Agent' because they are referenced through protected foreign keys: 'Collectionobject.cataloger'.\", {<Collectionobject: Collectionobject object (200994)>, <Collectionobject: Collectionobject object (200995)>, <Collectionobject: Collectionobject object (201000)>, <Collectionobject: Collectionobject object (201001)>, <Collectionobject: Collectionobject object (201002)>, <Collectionobject: Collectionobject object (201003)>, <Collectionobject: Collectionobject object (201004)>, <Collectionobject: Collectionobject object (201005)>, <Collectionobject: Collectionobject object (201006)>, <Collectionobject: Collectionobject object (201007)>, <Collectionobject: Collectionobject object (201008)>, <Collectionobject: Collectionobject object (201009)>, <Collectionobject: Collectionobject object (201010)>, <Collectionobject: Collectionobject object (201011)>, <Collectionobject: Collectionobject object (201012)>, <Collectionobject: Collectionobject object (201013)>, <Collectionobject: Collectionobject object (201014)>, <Collectionobject: Collectionobject object (201015)>, <Collectionobject: Collectionobject object (201016)>, <Collectionobject: Collectionobject object (201017)>, <Collectionobject: Collectionobject object (201018)>, <Collectionobject: Collectionobject object (201019)>, <Collectionobject: Collectionobject object (201020)>})"
}

Screenshots

https://herb32124-edge.test.specifysystems.org/specify/overlay/merge/Agent/?records=299,11910

failing_merge_michael_todd.mp4

Crash Report
Michael Todd – Merging 6282219c-e2d1-4943-9b44-aac1173469ea Crash Report - 2024-03-25T19_03_12.668Z.txt

Reported by: Royal Botanic Gardens Edinburgh
@grantfitzsimmons grantfitzsimmons added 1 - Bug Incorrect behavior of the product 2 - Merging labels Mar 25, 2024
@realVinayak
Copy link
Collaborator

Ha, this is a funny issue. @CarolineDenis (or @acwhite211) spot the bug. I zoomed it in for you ;)

I looked over the changes you made, don't know how I missed this.

specify7/specifyweb/specify/record_merging.py

Lines 200 to 206 in 0751046

for field_name in MERGING_OPTIMIZATION_FIELDS[model_name.lower()][table_name.lower()]:
query = Q(**{field_name: old_model_ids[0]})
for old_model_id in old_model_ids[1:]:
query.add(Q(**{field_name: old_model_id}), Q.OR)
foreign_model.objects.filter(query).update(**{field_name: new_model_id})
progress(1, 0) if progress is not None else None
continue

@realVinayak
Copy link
Collaborator

realVinayak commented Mar 26, 2024 •
edited

Unrelated, but spot the bug here too. This is a less priority than the above one bc the below code wouldn't run to completeness to the best of my knowledge. If it does though, it is a bug which will damage database! And user wouldn't have a clue

specify7/specifyweb/specify/record_merging.py

Lines 309 to 311 in 0751046

response: http.HttpResponse = update_record(obj)
if response is not None and response.status_code != 204:
return response

@CarolineDenis CarolineDenis added this to the 7.9.6 milestone Mar 28, 2024
@mpitblado
Copy link

Encountered this bug today (or at least the same result/error message), @realVinayak given the bug that you outline above leading to possible database damage (even if the code isn't expected to run to completeness), should we hold off on agent merging until this issue is marked as resolved?

@CarolineDenis CarolineDenis self-assigned this Mar 28, 2024
CarolineDenis added a commit that referenced this issue Mar 28, 2024
@CarolineDenis
Fixes agent merging failing unexpectedly
09c586c 
Fixes #4685
@CarolineDenis CarolineDenis linked a pull request Mar 28, 2024 that will close this issue
Open
3 tasks
@realVinayak
Copy link
Collaborator

realVinayak commented Mar 28, 2024 •
edited

@mpitblado no, you can go ahead with agent merging. The latter bug mentioned is not a "real" bug, but rather just highlights a minor code smell issue. Based on the current logic, it would never be manifested. I didn't particularly write those specific lines, but @acwhite211 likely didn't include it because of this reason.

However, that's what makes it overly complicated: We needed to know internal workings to make sure this bug doesn't occur (especially since it can completely commit an undergoing transaction).

I'm not sure if this issue will be resolved soon (it is marked for 7.9.6). The current workaround will be to just delete references manually for the original issue (the one that was reported)

@grantfitzsimmons
Copy link
Contributor Author

Reported by VIMS Fish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@CarolineDenis CarolineDenis

Labels
1 - Bug Incorrect behavior of the product 2 - Merging
Projects
None yet
Milestone
7.9.6
Development

Successfully merging a pull request may close this issue.

Fixes agent merging failing unexpectedly specify/specify7
4 participants
@grantfitzsimmons @realVinayak @CarolineDenis @mpitblado