Refactor the diesel pool support

[weiznich]

This commit refactors the support for connection pools for diesel. Instead of only allowing r2d2 based pools we now abstract the actual pool away. It also adds support for deadpool-diesel based pools as alternative.

[maxcountryman]

Thanks for this.

I'm starting to wonder if it's not better to encourage folks to implement SessionStore directly as opposed to trying to support a generic implementation within the crate. I know of at least one example where the project took that path.

If we did take that path, we could instead have a Diesel example of a concrete implementation for a given store.

[codecov]

Codecov Report

Merging #59 (dd5e897) into main (a3694c5) will increase coverage by 15.98%.
The diff coverage is 74.60%.

@@             Coverage Diff             @@
##             main      #59       +/-   ##
===========================================
+ Coverage   64.67%   80.66%   +15.98%     
===========================================
  Files          13       13               
  Lines         569      574        +5     
===========================================
+ Hits          368      463       +95     
+ Misses        201      111       -90     

Files	Coverage Δ
src/session_store.rs	57.57% <0.00%> (+4.79%)	⬆️
src/diesel_store.rs	79.83% <75.80%> (+79.83%)	⬆️

[weiznich]

I'm starting to wonder if it's not better to encourage folks to implement SessionStore directly as opposed to trying to support a generic implementation within the crate. I know of at least one example where the project took that path.

I believe having a single generic implementation might be much easier for users than having to implement that again and again for slightly different use-cases. Especially given that some users might require more or less all that degrees on freedom in their own implementation.

[maxcountryman]

Okay I'm having difficulty migrating this as part of the work on #57. While I'm working on that patch, I've removed the Diesel store but I can try adding it based on the changes here.

[weiznich]

I'm happy to help with such problems.

[maxcountryman]

Thanks I appreciate that--I'll create a separate branch we can work from.

[weiznich]

I've rebased this on top of #64

[maxcountryman]

Hm seems like we're having the same issue: zero tests are selected for e.g. the r2d2 flag. I feel like we need a more reliable way to ensure these tests actually run.

[maxcountryman]

I'm also a bit concerned this requires hacking around the various backends to support generically. Doesn't that imply that it's going to be more efficient to implement this in a concrete way for each backend? For example, having to query the row first adds additional overhead that isn't necessarily required given a concrete database. In what circumstances would I use this over implementing UserStore for my specific backend?

[weiznich]

Hm seems like we're having the same issue: zero tests are selected for e.g. the r2d2 flag. I feel like we need a more reliable way to ensure these tests actually run.

These tests are enabled here: https://github.com/maxcountryman/tower-sessions/pull/59/files#diff-73e17259d77e5fbef83b2bdbbe4dc40a912f807472287f7f45b77e0cbf78792dR128-R137

I'm also a bit concerned this requires hacking around the various backends to support generically. Doesn't that imply that it's going to be more efficient to implement this in a concrete way for each backend? For example, having to query the row first adds additional overhead that isn't necessarily required given a concrete database. In what circumstances would I use this over implementing UserStore for my specific backend?

Well it's complicated. Yes implementing the trait for a specifc backend would allow you to write more efficient queries, but that raises the problem that users need to match feature flags for diesel and tower-sessions. Additionally tower-sessions would mean to provide support of all possible (third party) diesel backends. So essentially: You be willing to accept store implementations for the firebird sql adaptor for diesel? My feeling was that this would likely be not accepted, therefore I've chosen a generic implementation that hopefully works out of the box on all backends.
The next point is "Why have this in tower-sessions?". I would say for the same reasons you have integrations for other crates there as well. You want to make it easier for users to start using your crate in their infrastructure. Essentially you could ask the question for all the backend crates and either just remove all of them or support all of them.

That's mostly the generic answer. To give a more concrete answer to the specific problem: I've chosen for now not to change tower-sessions outside of the diesel_store implementation, but the real solution here would be to just split the functions for insert and update into seperate ones for the Store trait, because at that point you already know whether or not a session exists. So it's easy to call different functions here and here.

[maxcountryman]

It's also possible to implement this as its own crate. Perhaps more generally that's the better pattern to follow (not just for Diesel, but all storage backends). I'd like to cut a new release so it's a bit awkward with Diesel changes pending (again, I want to point point that's not a fault of Diesel but rather the fact we're bundling built in backends).

[maxcountryman]

Just wanted to make sure that it's clear that in this PR the Diesel integration tests are not being selected and therefore not running. I can't merge this until those tests are passing. At the same time I want to cut a new release so what I may do is exclude Diesel in main for the time being--I'm sorry to have to do that again, but I am blocked on this and I don't want to push out changes that aren't covered by the integration tests. Please let me know what you think about breaking this out into its own crate.

[weiznich]

I'm not familiar with the CI setup of this repo, so its hard to tell for me why these the tests are not run. All I can say is that using the listed flags in a local test run results in the tests being executed successfully. I'm glad to change the integration in the CI setup if you provide some pointers about what needs to be changed.

Also: The changes are covered by integration tests. You can verify that locally if it really blocks you. (Although I can understand the need to run those tests on CI)

[maxcountryman]

I believe this is an issue with cargo and the feature flags independent of CI. You might want to look at how the flags are being passed to the runner tho. Again the issue is that no tests are selected which seems to be because the features aren't matching.

[maxcountryman]

This PR also has numerous merge conflicts so to merge this we need to resolve those conflicts and get the integration tests to select the proper tests and pass.

[maxcountryman]

For what it's worth, the test selection issue does not seem to be related to CI. For example, when running this branch locally, here's what I see:

cargo nextest run diesel_store_test --test integration-tests --features diesel-sqlite-store,diesel-r2d2
   Compiling scheduled-thread-pool v0.2.7
   Compiling diesel_derives v2.1.2
   Compiling rmp-serde v1.1.2
   Compiling r2d2 v0.8.10
   Compiling diesel v2.1.3
   Compiling tower-sessions v0.3.3 (/Users/max/Projects/tower-sessions)
    Finished test [unoptimized + debuginfo] target(s) in 5.64s
    Starting 0 tests across 1 binary (20 skipped)
------------
     Summary [   0.000s] 0 tests run: 0 passed, 20 skipped

Via commit 7e59cd03ecc417b341c9282ed06f9d16409a8490 (HEAD, weiznich/deadpool).

Looking at the Cargo.toml, I think the feature flags haven't been updated. Oddly GH formats this differently in the UI, but looking directly at the branch, they aren't there.

I think this might be a consequence of 7e59cd0 being force-pushed onto the branch.

[weiznich]

For future changes: It would be great if you could at least indicate whether or not you've merged submitted fixes to prevent spending time on debugging the same issues again and again. It's really not helpful to ignore fixes and disable the CI instead as you've done that with #64 without any clear communication..

[maxcountryman]

The issue here was caused by the force push--that commit removed the feature flags which meant the tests were not selected. It wasn't related to CI. I'm unclear what you're finding confusing about the CI workflow, which is based directly on cargo, but I'm happy to answer any questions you have.

As for the commit I excluded in main, I did so because it was only created to demonstrate the issue with the tests not running and revealed an issue with the implementation itself which caused a failed test run. It wouldn't have made sense to include it in main since it prevented the rest of the suite from running. However, I needed to move forward with pending changes and couldn't wait for this PR to be updated. I'm sorry about that, I should have made that clear. Fixing that particular issue should be as easy as rebasing and excluding that commit.

[weiznich]

The issue here was caused by the force push--that commit removed the feature flags which meant the tests were not selected.

It wasn't related to CI. I'm unclear what you're finding confusing about the CI workflow, which is based directly on cargo, but I'm happy to answer any questions you have.

I must disagree here. This was purely related to how your CI setup works, as just doing a cargo test --features $features_from_rust.yaml executes the relevant tests. There is no real indication that your CI setup perform additional filtering based on some special names. This is really confusing for me, as I see the feature thing and say: That's what I want, dump the relevant features in there and it should work. There is just no indication that the store_name field needs to be used in several variations in different places. It would have been helpful to point that out at the initial PR, where I've explicitly asked how these tests should be integrated.

As for the commit I excluded in main, I did so because it was only created to demonstrate the issue with the tests not running and revealed an issue with the implementation itself which caused a failed test run. It wouldn't have made sense to include it in main since it prevented the rest of the suite from running.

The relevant PR has a passing CI test suite: https://github.com/maxcountryman/tower-sessions/pull/64/checks, therefore I really cannot follow your point why this would "prevent the rest of the suite from running".

However, I needed to move forward with pending changes and couldn't wait for this PR to be updated. I'm sorry about that, I should have made that clear. Fixing that particular issue should be as easy as rebasing and excluding that commit.

Maybe we misunderstood each other here. This PR was built on top of the minimal fix. You could have merged that fix and moved on, while waiting for that PR being fixed.

Anyway this PR is now ready for review and passes all tests on CI, so please let me know what else you expect here.

[maxcountryman]

I'm sorry to argue here but that just is not correct: the force push is what broke this PR. If you find the Actions workflow confusing, that's fair but that has nothing to do with the force push removing the flags altogether, which is what happened. That's a n issue with force pushing to a remote branch and generally discouraged re git.

[maxcountryman]

Maybe we misunderstood each other here. This PR was built on top of the minimal fix. You could have merged that fix and moved on, while waiting for that PR being fixed.

This is not correct. The test suite was not selecting tests for Diesel. I could not merge that.

[weiznich]

I feel like we are arguing in circles here. The fundamental issue why the diesel tests were not executed from the start is this line in your CI setup. The run ${{ matrix.store }}_test filtered away any tests that do not have a matching name in the integration_tests file. This resulted in ignoring the diesel tests from the very beginning and is arguably an issue with my initial PR. It's not related to force pushing or feature flags or something like that.

[maxcountryman]

I feel like we are arguing in circles here. The fundamental issue why the diesel tests were not executed from the start is this line in your CI setup. The run ${{ matrix.store }}_test filtered away any tests that do not have a matching name in the integration_tests file.

The tests were working before the force push excluded the feature flags thanks to the fix you made in a separate PR.

Is the CI interface obvious? No. I agree with you. (And it would be helpful to have contributors improve this as they encounter these things.)

However, the issue remains the exclusion of the flags in the force push in this PR. Locally I was able to run the tests yesterday by using the included flags.

[maxcountryman]

Here's where I'm at: this is a ton of friction. I don't really know where things went wrong here but what I do know is I cannot maintain this implementation.

You've offered to help (thank you so much for that, it's truly appreciated). But you're encountering friction given how this repo is setup (again I'm sorry about that, that isn't what I would want).

This implementation itself doesn't seem ideal: it's not async but is in the core of a crate that's fundamentally async. It also has to compromise on performance in order to be generic. I agree with your points around making things easy for on ramping, so this is not to say those are bad decisions.

However, I'm not sure this doesn't belong in its own crate. This is the same approach that async-session took: it demarcates boundaries between the core session management bits and individual backends that implement SessionStore. This also means that changes to the core crate aren't entangled with the stores which would relieve the tension we've encountered here. It was probably a mistake for me to bundle any backends, but so it goes.

[weiznich]

The tests were working before the force push excluded the feature flags thanks to the fix you made in a separate PR.

No, the tests were not executed before the force push. See for example this CI run from #55.

However, the issue remains the exclusion of the flags in the force push in this PR. Locally I was able to run the tests yesterday by using the included flags.

I'm not sure what you are referring to but the last CI run executed all tests. They worked on the version from yesterday as well, the CI filtered them out for naming reasons.

Here's where I'm at: this is a ton of friction. I don't really know where things went wrong here but what I do know is I cannot maintain this implementation.

Well it would have been great if you had communicated that before I've spend quite a lot of time to implement all the necessary stuff. It's also frustrating to only get the message: "Tests are run working" without any indication that this is likely just a "broken" CI setup (which to be clear is not really related to the diesel backend at all.). I honestly start to consider that trying to contribute here is a mistake and I should just stop as my contributions are not wanted here.

This implementation itself doesn't seem ideal: it's not async but is in the core of a crate that's fundamentally async. It also has to compromise on performance in order to be generic. I agree with your points around making things easy for on ramping, so this is not to say those are bad decisions.

I've already pointed out that this "compromise on performance " can be easily removed.

However, I'm not sure this doesn't belong in its own crate. This is the same approach that async-session took: it demarcates boundaries between the core session management bits and individual backends that implement SessionStore. This also means that changes to the core crate aren't entangled with the stores which would relieve the tension we've encountered here. It was probably a mistake for me to bundle any backends, but so it goes.

In the end it's your decision as this is your crate. I would argue that you should handle sqlx support and diesel support in the same way. So if you move support for one into a separate crate you should handle support for the other one in the same way. If you keep one in the core crate you should keep the other one there as well. Also moving support for certain stores into separate crates does not really answer who should maintain them. To be very clear here: I do not have the capacity to maintain any additional crate.

[maxcountryman]

It's clear to me we won't be able to work together productively.

Thank you again for all the effort here, it might not feel like it's appreciated but it is.

I've decided this will be best implemented as a standalone crate.

[weiznich]

It's clear to me we won't be able to work together productively.

It would help if you would communicate clearly what you expect from changes, instead of changing your opinion every time. Also it would have been really helpful to actually suggest solutions instead of saying "It doesn't work, fix it now, its blocking me". That's an attitude that strange in the rust community. I will likely warn people about interacting with this project due to this attitude. Consider changing that if you want to make this project successful.

Thank you again for all the effort here, it might not feel like it's appreciated but it is.

Well, that confirms suspicions. My work is obviously not appreciated here.

I've decided this will be best implemented as a standalone crate.

It would have been great to know that as part of the initial issue or the initial PR. It would have saved a lot of work on both sides. Also it demonstrates quite good what I've wrote above about changing opinions all the time.

[maxcountryman]

"It doesn't work, fix it now, its blocking me"

Truly, I apologize: if that's what you heard, that's unfortunate. I'm sorry. I clearly communicated poorly and that's on me.

Thank you again for all the effort here, it might not feel like it's appreciated but it is.

Well, that confirms suspicions. My work is obviously not appreciated here.

😢 I'm not sure what I can do, but I am sorry you feel this way. I completely understand this is not the outcome you were hoping for, still it doesn't change the fact that I do appreciate the work you put into this.

It would have been great to know that as part of the initial issue or the initial PR.

I could not agree more: I am sorry that you spent time on this only to reach this conclusion. That said, the input for this decision largely came from this PR. For example, it's clear to me after having tried to work on this branch that I can't maintain this and you've now said you don't have the bandwidth either.

So with that, I don't think we can in good faith include this implementation: neither of us can maintain it. However, that was not clear when this PR was opened so it wouldn't have been possible to reach such a conclusion before we had the input that was uncovered over the course of working on this PR.

I understand your frustration. I am sorry you run into these issues--it sucks and as a crate author, it's not the outcome I'm aiming for.

I'm wishing you the best going forward and if there's anything I can do to show you my good faith, I'd be happy to do so.

[weiznich]

To bring this discussion back to something more productive: What would need to change that this becomes maintainable for you? Would it help to replace the generic implementation with something that is backend specific? That would probably mean its a bit more code and that it is not as universal as possible, but if that helps you to better work with the implementation thats a huge argument to go with that solution instead.

[maxcountryman]

To bring this discussion back to something more productive: What would need to change that this becomes maintainable for you?

One problem I'm running into is that I haven't used Diesel for several years now (and unfortunately I'm not actively using it elsewhere at this time). This creates an organic limitation on my ability to be helpful with Diesel and is a barrier to my effectiveness in supporting it. Most likely I would still need the help of someone from the Diesel community, even if we had specific backend implementations, who would be willing to take point on the maintenance of this implementation.

This is also where I start to think that standalone crates make more sense: I'm not the best person to be providing support for an ecosystem I'm not current with and it's unfortunate for the core of this crate to be a bottleneck to developing and publishing support for store backends. If we essentially will require individuals to provide maintenance for implementations, then maybe that should be over the scope of a standalone crate.

Now one thing we could consider is if it would make sense to create a workspace of crates within this repo--then we don't need separate repos or if we should follow a pattern similar to async-session where crates live in their own repos. Of course, we have preexisting implementations, so those would also ideally be migrated to their own crates, whichever pattern we might follow should we pursue this.

Returning to your original question, it's absolutely possible that specific backends lower the maintenance burden such that I can independently manage them. But I want to be careful about promising that because I don't know enough about the current state of Diesel to confidently say one way or the other.

[weiznich]

Returning to your original question, it's absolutely possible that specific backends lower the maintenance burden such that I can independently manage them. But I want to be careful about promising that because I don't know enough about the current state of Diesel to confidently say one way or the other.

I've created a striped down version of the diesel store here. It only implement support for postgres for now. I removed the backend unspecific generic code + the flexibility to change the session table, which in turn removes most of the complicated looking generic code. I've opted for leaving support for several pooling solutions in place as that looked like "simple" code in my opinion.
For other backends we would essentially need to duplicate the impls that are for PgConnection (and likely adjust queries to something that's supported on that backend, like using ON DUPLICATED KEYS for mysql). It's up to you to judge whether or not that looks more maintainable. If you consider including that I'm happy to update the implementation to support sqlite and mysql as well + add some tests for all variants.

Code is here:
https://github.com/weiznich/tower-sessions/blob/9b7f37458ec8e0ad74e3876983717a389d993ff7/src/diesel_store.rs

[weiznich]

@maxcountryman Do you have any opinion on the simplified implementation?

[maxcountryman]

Sorry for the delay, I've been trying to get axum-login migrated.

I'm planning to take a closer look at the implementation this weekend.

My hope is to distill the store vendoring strategy further, which will also help me evaluate how to best support things like Diesel.

[maxcountryman]

I've got a PR open to address the workspace configuration: #86.

Hopefully I can get that merged soon and then I'll be returning to this. Thanks for your patience here.

[weiznich]

Thanks for keeping me updated 👍

[maxcountryman]

I had another look at this but realized that it's depending directly on Session. Previously this was okay because it was part of the crate directly, but now we've moved to having store implementations be their own crates.

[weiznich]

I'm not sure I can follow what you are trying to say there. As far as I can see from the API docs all relevant methods on Session are public API. At least those used by the implementation linked here: #59 (comment)

[maxcountryman]

Maybe I'm misunderstanding how Diesel works (very likely) but it seems like there's a trait that needs to be implemented for Session but now both that trait and Session are in external crates so that's not possible.

[weiznich]

It's not necessary to implement any trait for session. In fact the implementation linked above already works without such an trait implementation. You can always load data into a tuple and map afterwards (or take them from session into a tuple for inserts).

[weiznich]

@maxcountryman It would be great to have a response from you whether or not linked implementation would be acceptable.

[weiznich]

@maxcountryman This is another ping to get you feedback on the implementation linked above.

[weiznich]

@maxcountryman This is another ping to get your feedback on the implementation linked above.