Skip to content

Create tag relations one by one, and use CTEs to minimize queries doing so#3043

Merged
rtibbles merged 1 commit intolearningequality:hotfixesfrom
bjester:duplicate-tags
Mar 26, 2021
Merged

Create tag relations one by one, and use CTEs to minimize queries doing so#3043
rtibbles merged 1 commit intolearningequality:hotfixesfrom
bjester:duplicate-tags

Conversation

@bjester
Copy link
Copy Markdown
Member

@bjester bjester commented Mar 25, 2021

Summary

We're receiving quite a bit of Sentry errors for attempts to create duplicate tags and tag-node relationships. This PR moves to creating them one by one, as @rtibbles suggested, but also uses a more complex, single, query to obtain all of the necessary information to create the tags and relations. It may still encounter attempts to duplicate tags or relations due to more than one user attempting to create the same tag at the same time, but since it uses get_or_create it shouldn't error any longer.

Description of the change(s) you made

  • PostgreSQL allows you to create pseudo temp tables from a list of values, like used when bulk inserting. I added some new classes to allow for using a VALUES list in a CTE. I'm planning on submitting a PR to django_cte to support this
  • I customized new CTE With classes to pass along other join types than just INNER or LOUTER, which in combination with a CTE values list can be a powerful tool by using RIGHT or FULL joins
  • Tag creation now uses the new VALUES CTE classes to send tag-node relation data to Postgres from which we can obtain all the information in one query result to create both new tags and relations, as well as delete relationships if necessary
  • The core functionality of adding / removing tags should be unchanged

Manual verification steps performed

  1. Open editable channel
  2. Edit a node's details
  3. Add a tag and save
  4. Verify new tags are created in the DB
  5. Select multiple nodes
  6. Click to bulk edit the nodes
  7. Add tag(s)
  8. Verify tags are applied

Reviewer guidance

How can a reviewer test these changes?

  • Test adding or removing tags from content nodes

References

Addresses: #2826


Contributor's Checklist

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2021

Codecov Report

Merging #3043 (356f609) into hotfixes (01adabf) will increase coverage by 0.05%.
The diff coverage is 95.40%.

Impacted file tree graph

@@             Coverage Diff              @@
##           hotfixes    #3043      +/-   ##
============================================
+ Coverage     85.71%   85.76%   +0.05%     
============================================
  Files           298      298              
  Lines         15879    15943      +64     
============================================
+ Hits          13611    13674      +63     
- Misses         2268     2269       +1     
Impacted Files Coverage Δ
contentcuration/contentcuration/db/models/query.py 94.23% <93.75%> (-5.77%) ⬇️
...ntcuration/contentcuration/viewsets/contentnode.py 86.89% <96.87%> (+0.87%) ⬆️
...tcuration/contentcuration/db/models/expressions.py 95.23% <100.00%> (+1.90%) ⬆️
contentcuration/contentcuration/models.py 84.43% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01adabf...356f609. Read the comment docs.

Copy link
Copy Markdown
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Tag creation has good test coverage, and the code seems logical to me!

@rtibbles rtibbles merged commit 26d8873 into learningequality:hotfixes Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding new tags during node update can seem to result in duplicate tag-node relation creation

2 participants