Add consolidation of duplicate LocalSiteProfile objects.

Review Request #12471 — Created July 14, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

We had two problems that were potentially causing the existence of
duplicate LocalSiteProfile models. The first of these is that this
wasn't using ConcurrencyManager, which handles integrity issues across
separate threads (or servers). The second, more difficult one, is that
some databases (particularly MySQL) were ignoring our unique_together
constraint when one of the relations was NULL. This meant it was
possible to end up with multiple LocalSiteProfile objects with
local_site=None.

This change moves us over to ConcurrencyManager, and implements a
check for the duplicate case when attempting to use
User.get_site_profile. If we do find duplicates, they'll be
consolidated into a single one before returning.

Ran unit tests.

Summary ID
Add consolidation of duplicate LocalSiteProfile objects.
We had two problems that were potentially causing the existence of duplicate `LocalSiteProfile` models. The first of these is that this wasn't using `ConcurrencyManager`, which handles integrity issues across separate threads (or servers). The second, more difficult one, is that some databases (particularly MySQL) were ignoring our unique_together constraint when one of the relations was NULL. This meant it was possible to end up with multiple `LocalSiteProfile` objects with `local_site=None`. This change moves us over to `ConcurrencyManager`, and implements a check for the duplicate case when attempting to use `User.get_site_profile`. If we do find duplicates, they'll be consolidated into a single one before returning. Testing Done: Ran unit tests.
a86e6b05cdefb853369e80ef04d5582c4db09faf
Description From Last Updated

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

'reviewboard.accounts.models.Profile' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'reviewboard.site.models.LocalSite' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Can we give create_if_missing a default?

chipx86chipx86

Most of our code capitalized as "Local Site". Can we have these docs use the same?

chipx86chipx86

Let's make this a numeric list. Makes it much easier to read what's in which slot.

chipx86chipx86

Can we do one per line? We should treat these like a dictionary when we have multiple keyword args. It's …

chipx86chipx86

Same as above, can we do one per line? Minimizes changes if we need to update the query. We may …

chipx86chipx86

We should save all this and permissions using update_fields=, I think?

chipx86chipx86

Can the paren be moved to the next line, and a trailing comma added? We'll probably need to update this …

chipx86chipx86

Swap these.

chipx86chipx86

We should add a step that reloads and verifies resultls.

chipx86chipx86

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Show all issues

    Can we give create_if_missing a default?

  3. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Show all issues

    Most of our code capitalized as "Local Site". Can we have these docs use the same?

  4. reviewboard/accounts/managers.py (Diff revision 2)
     
     
     
     
    Show all issues

    Let's make this a numeric list. Makes it much easier to read what's in which slot.

  5. reviewboard/accounts/managers.py (Diff revision 2)
     
     
     
    Show all issues

    Can we do one per line? We should treat these like a dictionary when we have multiple keyword args. It's much easier to read and to update.

  6. reviewboard/accounts/managers.py (Diff revision 2)
     
     
     
    Show all issues

    Same as above, can we do one per line? Minimizes changes if we need to update the query.

    We may also want to use .only(...) with pk, permissions, and the counters. Small optimization, and this is unlikely to be called much, so I'll leave that up to you.

  7. reviewboard/accounts/managers.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    We should save all this and permissions using update_fields=, I think?

  8. Show all issues

    Swap these.

  9. reviewboard/accounts/tests/test_local_site_profile.py (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    We should add a step that reloads and verifies resultls.

  10. 
      
david
Review request changed
Commits:
Summary ID
Add consolidation of duplicate LocalSiteProfile objects.
We had two problems that were potentially causing the existence of duplicate `LocalSiteProfile` models. The first of these is that this wasn't using `ConcurrencyManager`, which handles integrity issues across separate threads (or servers). The second, more difficult one, is that some databases (particularly MySQL) were ignoring our unique_together constraint when one of the relations was NULL. This meant it was possible to end up with multiple `LocalSiteProfile` objects with `local_site=None`. This change moves us over to `ConcurrencyManager`, and implements a check for the duplicate case when attempting to use `User.get_site_profile`. If we do find duplicates, they'll be consolidated into a single one before returning. Testing Done: Ran unit tests.
f75d2fa605a47d7485491004c54a5ae8a0f12905
Add consolidation of duplicate LocalSiteProfile objects.
We had two problems that were potentially causing the existence of duplicate `LocalSiteProfile` models. The first of these is that this wasn't using `ConcurrencyManager`, which handles integrity issues across separate threads (or servers). The second, more difficult one, is that some databases (particularly MySQL) were ignoring our unique_together constraint when one of the relations was NULL. This meant it was possible to end up with multiple `LocalSiteProfile` objects with `local_site=None`. This change moves us over to `ConcurrencyManager`, and implements a check for the duplicate case when attempting to use `User.get_site_profile`. If we do find duplicates, they'll be consolidated into a single one before returning. Testing Done: Ran unit tests.
9c6f01e643cc3bfd0d23976c2d481edd2e97c419

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revisions 2 - 4)
     
     
    Show all issues

    Can the paren be moved to the next line, and a trailing comma added? We'll probably need to update this list at some point.

    It'd be nice to alphabetize it, too.

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (906cbbc)