fix: allow sqlx chrono and time features to be enabled

[Stupremee]

Description

Similar to pgmq/pgmq#401

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Testing

• I have added tests that prove my fix is effective or that my feature works
• I have run the existing tests and they pass
• I have run cargo fmt and cargo clippy

Checklist

• My code follows the code style of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Additional Notes

Any additional information, context, or screenshots about the pull request here.

[geofmureithi]

Have you looked at https://github.com/orgs/apalis-dev/discussions/649?

[Stupremee]

Ah, I did not see that! That would be another option.

For me personally, and I think in general, both are suitable solutions.
If you make the time backend configurable, would probably be the more complete solution.

I would still add the explicit type casts (like in this PR), to avoid the case, where a user wants to use the chrono timestamps, but has sqlx with both features enabled in his dependency tree.

[geofmureithi]

I will look at this tomorrow, but its always encouraged to create an issue/discussion before a PR.

[Himmelschmidt]

Wouldn't this approach force time users to use chrono and convert it everywhere? This seems a bit undesired

[geofmureithi]

@Stupremee Please respond to the question above

[geofmureithi]

Your PR doesnt seem to solve the actual issue at hand. Can you actually demonstrate that you can choose between chrono and time? adding an example and some tests should be the bear minimum.

[geofmureithi]

This is not about cargo test

[geofmureithi]

Your goal should be to allow both crates to be usable using a feature flag

[Stupremee]

It's not about choosing time or chrono for apalis-postgres. This pull request is only meant to make it possible to compile apalis-postgres if sqlx has both features enabled.

[Stupremee]

I will look at this tomorrow, but its always encouraged to create an issue/discussion before a PR.

@geofmureithi I had this change lying around on my PC for quite a while now, because I was using a vendored version of the crate. Then I figured I would just open a PR real quick. Sorry for the inconvenience, I should have opened an issue first!

Wouldn't this approach force time users to use chrono and convert it everywhere? This seems a bit undesired

I didn't change anything in the public API, so the crate for the user still stays the same. Right now, the user is just not able to have a sqlx dependency in his tree where both time and chrono is enabled. Because then apalis-postgres simply fails to compile.

You can still easily add configurable time backends. This pull request only makes sure, that apalis-postgres is able to compile successfully if sqlx is enabled with time and chrono features. It can easily happen if any dependency in the tree has the features enabled due to feature unification. E.g. if you use sea-orm-migration in your project (as I do in one of mine) and apalis-postgres, you will encounter this bug and wont be able to compile your project.

In this repository you can find the reproduction of the bug I mean.

[geofmureithi]

The problem here seems to be trying to do this:

sqlx = { version = "0.8.6", features = ["time", "chrono"] }

Instead, the recommended approach IMO is to separate the database/model from the worker logic. In your showed example, I would create two crates and have each crate with the specific features.

[Stupremee]

But if I want to queue jobs from my backend application, I must have apalis-postgres in the dependencies of my backend crate or am I missing something?

Because if that's the case, then the compilation error occurs, because I also have sea-orm-migration in my dependency tree, which currently enables both features on sqlx leading to a compilation error

[geofmureithi]

You dont have to have all the deps in the backend's Cargo.toml, you can create another crate and depend on it in the backend application. I would encourage experimenting with that first. Something like:

package.name = "backend"
...
[dependencies]
worker-crate = "..path1"
models-crate = "..path2"

[Stupremee]

This will not work because of feature unification. cargo will collect all sqlx dependencies in the whole tree, and unify all features. I've updated my reproduction repository to include a workspace with multiple crates

[geofmureithi]

You still have:

sqlx = { version = "0.8.6", features = ["time", "chrono"] }

[Stupremee]

Let's say I use the time crate in my database model, if I add sqlx like this to my db crate, it will not compile.

sqlx = { version = "0.8.6", features = ["time"] }

[geofmureithi]

This problem seems to be more related to sqlx instead of apalis.
I faced a similar issue and did not receive any response.
launchbadge/sqlx#4066

As noted, there is already a discussion on this issue and I think that will solve your current problem. I dont currently have the bandwidth to help you debug your problem. I will make a basic attempt on the approach discussed and update my findings there.

[Stupremee]

Yes it's a common pitfall with sqlx. There are multiple crates with the same problem.

This pull request fixes it by explicitly casting all date time values to chrono types. If you switch out apalis-postgres with my fork, the project compiles fine.

[geofmureithi]

As I said, this is not a complete solution because it does not:

1. Add actual time support.
2. Fix the issue upstream in apalis-sql hence would not be in effect for apalis-sqlite or apalis-mysql.

Your PR should remain open as I think through this and experiment a little bit more. You should be able to use your fork as a dependency via git for now.

[Stupremee]

Yes full time support would be cooler, but I thought it would be nice to have a temporary solution, so it can at least compile the project successfully. The fix doesn't change anything about the crate

[geofmureithi]

Is this issue happening in 0.7.x?

[geofmureithi]

Are you sure?? 0.7 does not use sqlx prepare

[Stupremee]

My bad, 0.7.x is not affected 😅
It does not use the query macros, so there is no type checking

[geofmureithi]

I think then it should be ok for now. 0.7 should be what you should be using if you need a stable experience. 1.0 is still not stable and should be considered the wild west. I will spend some more time on this hopefully tomorrow and will do your contribution justice. I dont think this waiting some few more days would be a major issue.