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

start migration more web endpoints to sqlx #2311

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

syphar
Copy link
Member

@syphar syphar commented Nov 9, 2023

refs #874

This migrates more web endpoints to sqlx.

Notable omissions around Limits and CrateDetails, which should be async too, but I wanted to leave them for another PR.

Also I want to find a better way to use async in tests, bestrefs #874

This migrates more web endpoints to sqlx.

Notable omissions around Limits and CrateDetails, which should be async too, but I wanted to leave them for another PR.

Also I want to find a better way to use async in tests, best case even using tokio::test or even sqlx::test, at the least some async_wrapper that enables you to write .await in tests. Also for a separate PR. case even using tokio::test or even sqlx::test, at the least some async_wrapper that enables you to write .await in tests. Also for a separate PR.

Last think I also didn't do is parallelization of independent queries in a single handler, which could be investigated separately.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Nov 9, 2023
@syphar
Copy link
Member Author

syphar commented Nov 9, 2023

interesting test failure, I remember running the tests locally and it worked, will try again fixed

src/web/sitemap.rs Outdated Show resolved Hide resolved
@Nemo157
Copy link
Member

Nemo157 commented Nov 9, 2023

at the least some async_wrapper that enables you to write .await in tests.

I had a quick look at this. The biggest blocker is that all the database calls in the test fakes need to be moved over too, otherwise they error like

thread 'web::releases::tests::get_releases_by_stars' panicked at postgres-0.19.7/src/config.rs:449:44:
Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.

It's also non-trivial because of the higher-ranked lifetimes meaning that closures don't work, I think the best approach might be to use a macro to wrap the tests giving an API like

async_tests! {
    async fn foo(env: &TestEnvironment) -> Result<()> {
        ...
    }
}

You can see my experiment in the wrapper here, with the inner async fn used instead of a closure to have the lifetimes work: master...Nemo157:docs.rs:test-async-wrapper.

@syphar
Copy link
Member Author

syphar commented Nov 10, 2023

It's also non-trivial because of the higher-ranked lifetimes meaning that closures don't work, I think the best approach might be to use a macro to wrap the tests giving an API like

I remember I made it work by using an async closure + Rc for the TestEnvironment, but in general I'll look into this separately.

I had a quick look at this. The biggest blocker is that all the database calls in the test fakes need to be moved over too, otherwise they error like

yep, the sync/async mix makes some things a little ugly until more is migrated.

@syphar syphar requested a review from Nemo157 November 10, 2023 05:21
@syphar syphar force-pushed the more-web-sqlx branch 5 times, most recently from 4b8f02c to eb72599 Compare November 15, 2023 10:39
@syphar
Copy link
Member Author

syphar commented Nov 15, 2023

I accidentally amended other changes from #2312 to the release-time-nullable commit, so now I just squashed them all to avoid more confusion.

@syphar syphar merged commit 30e2f2c into rust-lang:master Nov 15, 2023
7 checks passed
@syphar syphar deleted the more-web-sqlx branch November 15, 2023 11:42
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 15, 2023
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 16, 2023
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