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

chore: Upgrade to Diesel 2.2 #1647

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

tommie
Copy link

@tommie tommie commented Jan 9, 2025

Description

Upgrades Diesel to 2.2.

This uncovered some bugs in the MySQL backend, e.g. there can be at most one batch ID per user every 10 ms. It seems Diesel 2 is faster (and libmysqlclient is faster than libmariadb), causing the E2E batch tests to generate batches faster than before. Nothing ever updated the session timestamp in the backend, which suggests there could only really be one batch per user.

Split off from #1644.

Testing

Existing integration tests.

Issue(s)

Closes SYNC-4568 (?)

tommie added 7 commits January 9, 2025 19:24
See https://diesel.rs/guides/migration_guide.html.

- This pulls in mysqlclient-sys >=0.4.0, which allows using libmariadb instead of libmysqlclient.
  That allows using Debian's bundled client library.
  MySQL's APT repository only has packages for AMD64.
- Connections are now passed as mut refs, which requires RefCell/RwLock.
- Embedded migrations had a slight API change.
- put_bso_sync was calling update_collection without its transaction, which now causes a deadlock.
  Updated to write inside its transaction.

Signed-off-by: Tommie Gannert <[email protected]>
Signed-off-by: Tommie Gannert <[email protected]>
The global functions were replaced with Server in 0.32.0 and the
Cargo.toml has 1.4 listed (probably due to a mistake when pinning
Diesel at 1.4.)

Signed-off-by: Tommie Gannert <[email protected]>
Cargo audit:

Crate:     idna
Version:   0.5.0
Title:     `idna` accepts Punycode labels that do not produce any non-ASCII when decoded
Date:      2024-12-09
ID:        RUSTSEC-2024-0421
URL:       https://rustsec.org/advisories/RUSTSEC-2024-0421
Solution:  Upgrade to >=1.0.0
Signed-off-by: Tommie Gannert <[email protected]>
There is no other way for the timestamp to increase in production.

Signed-off-by: Tommie Gannert <[email protected]>
Since the batch_uploads primary key already contains user_id, it does
nothing to increase time resolution. The counter may still see
conflicts with other processes, but the probably is much lower
now. (Spanner uses UUIDv4 for batch IDs.)

The user_id hack was added in e9b455f "to match the Python
implementation". E2E tests are failing because we can now
generate more than one batch within 10 ms.

Signed-off-by: Tommie Gannert <[email protected]>
@jrconlin
Copy link
Member

jrconlin commented Jan 9, 2025

Ugh, Looks like these commits aren't signed, so this PR can't be merged.

You can tell because the commits don't have a verified tag. (see the commits on #1646 as an example)

You can sign using the same SSH key that you set up in github, which can be easier (and more secure) than dealing with GPG signatures.
FWIW, a really stupid way to do this would be to:

  • set up git signing
  • git diff origin/master > diesel.patch
  • git checkout -b diesel3
  • patch -p1 < diesel.patch
  • git commit -a -- If you're not running an ssh-agent, you should be prompted for your passphrase.
  • git push ...

That's basically what I'd do to duplicate your PR as signed.

@tommie
Copy link
Author

tommie commented Jan 9, 2025

Oh, you actually did mean a digital signature. Now I'm curious what this gains you? The code merged is what's reviewed in the PR, so it doesn't seem to protect against anything?

@jrconlin
Copy link
Member

jrconlin commented Jan 9, 2025

It's required by our Security Operations (SecOps) folk for all production run code. Basically, it's to ensure that this and future code comes from "you", and can be trusted. This article presents a fairly good reason for why we require them, even though we're open source and public.

jrconlin added a commit that referenced this pull request Jan 9, 2025
This is a signed version of the work done by @tommie in
#1647

This also resolves some clippy issues for 1.84.0

Closes SYNC-4568
@jrconlin
Copy link
Member

beh, So, in these cases I usually just pull master, drop Cargo.lock, and rebuild.

I'll approve after that, but I'll want to give at least one other team member a chance before I land anything.

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.

2 participants