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.

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

Fix Folder.DoesNotExist error in local deployment #3580

Merged

Conversation

cmsetzer
Copy link
Contributor

@cmsetzer cmsetzer commented Jul 27, 2024

#3563 may have introduced a regression affecting a locally deployed vanilla Perma setup (as far as I can tell, this does not affect prod). After re-deploying Perma from scratch with all Docker images, volumes, and caches cleared, the following error occurs when you submit any URL for capture:

  File "/perma/perma_web/api/views.py", line 512, in post
    link.move_to_folder_for_user(folder, request.user)  # also sets link.organization
  File "/perma/perma_web/perma/models.py", line 1794, in move_to_folder_for_user
    Folder.objects.select_for_update().get(pk=folder.tree_root_id)
  File "/usr/local/lib/python3.11/site-packages/django/db/models/query.py", line 637, in get
    raise self.model.DoesNotExist(
perma.models.Folder.DoesNotExist: Folder matching query does not exist.

If we change the get(pk=folder.tree_root_id) to a filter(id=folder.tree_root_id).first(), the query just retrieves 0 rows and then moves on without throwing an exception.

Any concerns with this solution? Or does this suggest a deeper issue?

@cmsetzer cmsetzer requested a review from a team as a code owner July 27, 2024 01:00
@cmsetzer cmsetzer requested review from rebeccacremona and removed request for a team July 27, 2024 01:00
Copy link

codecov bot commented Jul 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.03%. Comparing base (b3b0ad8) to head (98eca7d).
Report is 22 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3580      +/-   ##
===========================================
+ Coverage    68.99%   69.03%   +0.03%     
===========================================
  Files           48       48              
  Lines         6969     6978       +9     
===========================================
+ Hits          4808     4817       +9     
  Misses        2161     2161              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rebeccacremona
Copy link
Contributor

Hmmmmm. Yes, I think that does suggest a deeper issue. Nice catch!!

All folders need to have tree_root_id populated, and if there are any folders that don't, those folders are in a bad state.

Here's what I'm going to bet is going on:

  • since locally I've been using the same dev database for a long while, this data migration took care of it for me
  • during test set up, we use this utility to fix up the installation of our JSON fixtures
  • during local dev setup.... we do neither: we just install the fixtures

These old JSON fixtures are a total pain in the butt, and we've migrated... maybe half?... of the suite from using them. I can't wait until we are done with that project and can lose them.

In the meantime, I think probably the quickest fix is to run _fix_json_fixtures inside invoke dev.init_db?

I think we don't want to tweak the code so this isn't an error; if a production folder finds itself in this state, I think it ought to be noisy about it... though, now that we have migrated the production data to use this new field, we might be able to improve the way in which it is noisy about it (e.g., make the field non-nullable).

@cmsetzer
Copy link
Contributor Author

Thanks @rebeccacremona! I had a hunch it might be something along those lines.

I agree about fixing the root issue in init_db rather than suppressing the exception. I'll try it and push up the change once everything checks out.

@rebeccacremona
Copy link
Contributor

@cmsetzer wonderful, thanks!

if it isn't working for any reason... we can figure out how to update the json files so that they just have all the correct field values. i initially thought that would be too annoying, but could be that was the wrong call 🙂

@cmsetzer
Copy link
Contributor Author

I took a swing at using _fix_json_fixtures in the DB init task but it did not like importing from conftest.py. Rather than deal with that, I just went ahead and corrected the source data in folders.json so no fixer function is needed.

This takes care of the local issue on my end and tests are passing. Want to take a look?

Copy link
Contributor

@rebeccacremona rebeccacremona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ughh, sorry for that hassle!! Thanks for doing this 🙏!

@cmsetzer cmsetzer merged commit 9b97f67 into harvard-lil:develop Jul 31, 2024
2 checks passed
@cmsetzer cmsetzer deleted the fix-folder-does-not-exist-error branch July 31, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants