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

[offline] Change prepare to one-file-per-query #2110

Closed
wants to merge 3 commits into from

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Sep 21, 2022

Description

Continuation of/supersedes #1770 (comment) and #1183.

Overhauls offline query metadata from being saved in a single sqlx-data.json file, to every query being saved separately in a .sqlx directory (either in the package or workspace root depending on the --workspace flag).

E.g.

.sqlx
├── query-2b192bf413d46a28226de2ad754a382043c483cf55c04d19bdfa916dcbf0e7ae.json
├── query-95d796755ea0ade4578c91d102407ae003487e40922284cdd6f5c8cbc8d0b5e2.json
└── query-e46f12d720e0911c9280a1b43d7aab30beef94861713fa686db77cfe30702792.json

Changed

Many of the changes are from #1770 and #1183. I updated the previous work on this with the latest changes from the main branch, implemented outstanding things like cargo sqlx prepare --check, and fixed several bugs.

  • Change sqlx prepare to save queries individually in a .sqlx directory.
  • Rename --merged flag to --workspace for sqlx prepare.
  • Add offline query support for MSSQL.
  • Test offline query macros in GitHub Actions.

The file changes look bigger than they are because sqlx-macros/src/query/data.rs was split into its own submodule: sqlx-macros/src/query/{mod.rs, offline.rs}. Edit: reverted this change after rebasing on 0.7-dev.

TODO:

  • update documentation, README.md when the new behaviour is agreed upon. Edit: first pass done.
  • squash commits

Related issues

Closes #570
Closes #1005

Supersedes #2322

Type of change

Breaking change. Developers will need to install the latest sqlx-cli and re-run cargo sqlx prepare in their projects.

How does it work?

Saving offline queries

  1. When running cargo sqlx prepare, the command will execute cargo check with the SQLX_OFFLINE_DIR environment variable set to <package or workspace>/.sqlx, depending on the --workspace flag.
  2. Then, expand_with_data in sqlx-macros saves the query metadata to a temporary file, before moving it to the SQLX_OFFLINE_DIR path (the move/rename operation should be atomic).

Note that SQLX_OFFLINE_DIR can be set manually so cargo check without cargo sqlx prepare can generate query files---useful for the CI---but the environment variable doesn't override the directories used by sqlx prepare or sqlx prepare --check, should it? I'm concerned about data loss if it's set to another directory and sqlx prepare tries to delete it. Edit: changed removal behaviour so only query-*.json files are deleted.

Loading offline queries

When loading query metadata offline, expand_input in sqlx-macros will check if the query is saved in SQLX_OFFLINE_DIR if set, then <package>/.sqlx, then <workspace>/.sqlx.

Checking offline queries

Running cargo sqlx prepare --check will generate queries and place them in target/sqlx-prepare-check and compare the contents to <package or workspace>/.sqlx, depending on the --workspace flag.

If there are queries in target/sqlx-prepare-check that aren't in <package or workspace>/.sqlx (indicating new queries were probably added), the command will fail with an error: prepare check failed: .sqlx is missing one or more queries; you should re-run sqlx prepare.

If there are unused queries in <package or workspace>/.sqlx that aren't in target/sqlx-prepare-check (indicated old queries were probably removed), the command will succeed with a warning: potentially unused queries found in .sqlx; you may want to re-run sqlx prepare.

Finally, it will compare the JSON contents of files in both directories to ensure they match.

@cycraig
Copy link
Contributor Author

cycraig commented Sep 21, 2022

Not quite sure why the CI is failing for MySQL 5.7: https://github.com/launchbadge/sqlx/actions/runs/3100589799/jobs/5021077726

Edit: it was running a test with MySQL 5.7 + rustls which is not supported, pushed a commit to conditionally exclude it from the CI like before.

Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message

Error: error communicating with database: received fatal alert: HandshakeFailure

Caused by:
    received fatal alert: HandshakeFailure

@abonander
Copy link
Collaborator

In #2039 I'm actually doing quite a number to the code for the macros right now. Are you okay waiting until I've finished that? I don't recommend basing on it right now because of how much in flux it is though.

@cycraig
Copy link
Contributor Author

cycraig commented Sep 21, 2022

Sure, I can certainly try when you're finished! No guarantee though, depending on how large the differences are it might be better to start from scratch and copy pieces over, rather than continue this PR and risk overwriting changes.

@abonander
Copy link
Collaborator

@cycraig I've finally merged #2039, are you still available to rebase this?

Co-authored-by: Jonas Platte <jonas@lumeo.com>
@cycraig
Copy link
Contributor Author

cycraig commented Feb 18, 2023

@abonander

👋 Sure!

I ended up rewriting several parts since the 0.7-dev refactor affected a lot of the offline code. Tried to retain commit authorship though. Updated the PR description and (some) documentation.

I'm unsure how popular this change will be. Displaying many query-*.json files in GitHub PRs, for instance, may be less preferable than a single easy-to-ignore file. Though the automatic file generation through SQLX_OFFLINE_DIR might be useful to some.

Another thing to consider is printing an error/warning when the installed sqlx-cli and sqlx dependency versions are mismatched, to alert developers to potential breaking changes automatically.

One (hacky) way to achieve this is to have sqlx prepare set an environment variable (e.g. SQLX_CLI_VERSION) passed to cargo check, then compare it against CARGO_PKG_VERSION when sqlx-macros is generating an offline query?
Or just parse Cargo.lock, not sure if there's a proper way to get the resolved version of a dependency through Cargo.

Co-authored-by: Austin Bonander <austin@launchbadge.com>
@CosmicHorrorDev
Copy link
Contributor

CosmicHorrorDev commented Feb 18, 2023

I'm unsure how popular this change will be. Displaying many query-*.json files in GitHub PRs, for instance, may be less preferable than a single easy-to-ignore file.

Just one anecdotal point, but the single sqlx-data.json file was a huge source of merge conflicts at my previous job. It led to a lot more churn where PRs would have to run cargo sqlx prepare to fix conflicts very frequently. This shouldn't be nearly as much of an issue with the separate files

Another plus is that this removes the need to cache the parsed sqlx-data.json files

@CosmicHorrorDev
Copy link
Contributor

Another thing to consider is printing an error/warning when the installed sqlx-cli and sqlx dependency versions are mismatched, to alert developers to potential breaking changes automatically.

+1 to this or some form of version checking. Problems due to a version mismatch seem to be a constant source of issues for people

@abonander abonander deleted the branch launchbadge:0.7-dev February 21, 2023 21:25
@abonander abonander closed this Feb 21, 2023
@CosmicHorrorDev
Copy link
Contributor

Looks like this automatically got closed from the target branch getting deleted or something along those lines

@abonander
Copy link
Collaborator

Oh yeah, I merged 0.7-dev to main so I didn't have to go around asking people to keep rebasing their PRs. I think I can change the base on this one and reopen it.

@abonander
Copy link
Collaborator

@cycraig looks like I can't do that, sorry. Can you rebase back against main again and open a new PR? I promise that's the last base change for now. (I'll probably avoid doing dev on a separate branch from now on because I really don't have the bandwidth to backport fixes and work on new development at the same time anyway.)

@abonander
Copy link
Collaborator

Perhaps when I cut a release I should immediately bump the version number on main so people know what version they're writing PRs for.

@cycraig
Copy link
Contributor Author

cycraig commented Feb 21, 2023

Probably my fault for not clicking the "Allow edits and access to secrets by maintainers" checkbox!

Opened #2363 targeting main.

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