Skip to content

feat: set users ID to be autoincrementing - #240

Open
6ixfalls wants to merge 3 commits into
allenporter:mainfrom
6ixfalls:feat/user-increment
Open

feat: set users ID to be autoincrementing#240
6ixfalls wants to merge 3 commits into
allenporter:mainfrom
6ixfalls:feat/user-increment

Conversation

@6ixfalls

@6ixfalls 6ixfalls commented Sep 4, 2026

Copy link
Copy Markdown

User IDs are currently recycled as the column is an ROWID. When a user is deleted from the database, a new user can obtain the same User ID, and the old user's files are assigned to the new user who now has full access to them. This changes the column to be autoincremented and also ensures this for old deployments.

@allenporter

Copy link
Copy Markdown
Owner

I noticed there are some test failures. Does your change pass the tests locally?

@6ixfalls

6ixfalls commented Sep 7, 2026

Copy link
Copy Markdown
Author

I noticed there are some test failures. Does your change pass the tests locally?

Should be fixed, but I have not deployed this to test yet.

@allenporter

Copy link
Copy Markdown
Owner

OK Looks great. How did you notice this? I was wondering if we could have a test that reproduced the original issue in a similar way that you noticed it through the API e.g.:

  • create a user
  • add a file
  • query the files for the user and see the file
  • delete a user
  • create a user
  • query the files for the user and see nothing
    etc

@codecov-commenter

codecov-commenter commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.89%. Comparing base (bb77bd2) to head (603b45c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...bic/versions/68964804740d_prevent_user_id_reuse.py 79.31% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   90.96%   90.89%   -0.08%     
==========================================
  Files         113      114       +1     
  Lines       11749    11779      +30     
==========================================
+ Hits        10687    10706      +19     
- Misses       1062     1073      +11     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@6ixfalls

6ixfalls commented Sep 8, 2026

Copy link
Copy Markdown
Author

OK Looks great. How did you notice this? I was wondering if we could have a test that reproduced the original issue in a similar way that you noticed it through the API e.g.:

  • create a user
  • add a file
  • query the files for the user and see the file
  • delete a user
  • create a user
  • query the files for the user and see nothing
    etc

It might be quite long to add a test like that. I found this issue through an AI passthrough (and a few others, but those fixes are larger).

@allenporter

Copy link
Copy Markdown
Owner

It might be quite long to add a test like that. I found this issue through an AI passthrough (and a few others, but those fixes are larger).

Please sync and rebase. I added the test in #241 setup for you to update with your fix.

@allenporter allenporter left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Marking request changes; update the test that should reproduce the issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants