Use UUID v1.0 release

[marti4d]

UUID had their offical v1.0 release now 🎉

Postgres crates already support it via the with-uuid-1 option,
so let's do it!

Fixes #325

PR Info

• Closes Enable UUID v1.0 for Postgres #325

Changes

• Update UUID dependency to 1.0

[marti4d]

Note to review -- I already checked this using cargo tree -d to ensure that there is no mixing of uuid 0.8 and uuid 1.0, and there is not -- It's all 1.0 now!

[marti4d]

Ah, I see there are failures. These did not appear on my machine running cargo test. I'm looking into it now.

[marti4d]

Alrighty -- Seems to work and pass all tests. Should be ready to review 🙂

[ikrivosheev]

@marti4d thank you! LGTM!

[billy1624]

This doesn't seems right. uuid v1.x shouldn't need a separate ColumnType

[billy1624]

If the behaviour and API between uuid v0.8 and uuid v1.0 remains the same. We should switch the version of uuid to v1.0 if with-uuid-1 feature is enabled, instead of introducing a new UUID variant in Value enum.

[ikrivosheev]

You mean if uuid-8 is enabled, then we disable the previous uuid version? Maybe compile error?

[billy1624]

I mean swapping the version of uuid.

• If with-uuid is enabled but not with-uuid-1, then we import use uuid::Uuid;
• If with-uuid-1 is enabled, then we import use uuid_1::Uuid;

i.e. the same old Uuid(Option<Box<Uuid>>) unchanged. But we swap the inner uuid to either v0.8 / v1.0

[marti4d]

@billy1624 -- Thanks for the feedback. What you're describing is actually how I implemented this previously in this PR. Example from value.rs:

#[cfg(all(feature = "with-uuid", feature = "with-uuid-1"))]
compile_error!("You cannot use `with-uuid` and `with-uuid-1` together");

#[cfg(feature = "with-uuid")]
use uuid::Uuid;

#[cfg(feature = "with-uuid-1")]
use uuid_1::Uuid as Uuid;

The issue was that the automation builds use cargo build --all-features and cargo test --all-features, which tried to enable both with-uuid and with-uuid-1 at the same time, causing the build and tests to fail. In order to make everything work, I had to change the code to allow both uuid v0.8 and uuid 1.0 to co-exist in the same build.

I completely agree with you that it is better to have just one version of uuid enabled, but if we wanted that then we'd have to change the tests to run cargo test --features with-uuid,<all_other_features> and possibly another test with cargo test --features with-uuid-1,<all_other_features>

I don't know which option you like better? The other option, of course, is that we wait for Rusqlite and Sqlx to finally release UUID 1.0 support. You don't have to take this change -- I can continue using my own fork in my projects until UUID 1.0 lands in all dependencies. Just wanted you to have the option use the work I had to do anyway 🙂

[tyt2y3]

Ohhhhh! We might actually want to use this question mark across all our crates

The "package-name/feature-name" syntax will also enable package-name if it is an optional dependency. Often this is not what you want. You can add a ? as in "package-name?/feature-name" which will only enable the given feature if something else enables the optional dependency.

Note: The ? syntax is only available starting with Rust 1.60.

[tyt2y3]

Actually, I think this is a BIG problem the Rust ecosystem has to tackle as a whole.
It's way too easy to making breaking API changes and hence major bumps are unavoidable, and so all downstream has to bump as well.
I like your approach to let uuid-0 and uuid-1 coexist for now. Actually, I would prefer renaming everything from Uuid to Uuid0. It breaks everybody, but hey, it's a major upgrade, and this prompts people to actually upgrade their dependencies. Otherwise I am afraid uuid0 will stay as the default forever.

[marti4d]

I like your approach to let uuid-0 and uuid-1 coexist for now. Actually, I would prefer renaming everything from Uuid to Uuid0. It breaks everybody, but hey, it's a major upgrade, and this prompts people to actually upgrade their dependencies. Otherwise I am afraid uuid0 will stay as the default forever.

To make sure I'm understanding before I make the change -- Are you saying that you would prefer there to be Uuid0 and Uuid1 types in the code, or are you saying that there should be Uuid0 type for the old stuff and Uuid should now use Uuid v1.0?

[tyt2y3]

Uuid0 and Uuid1 types

Yup that's what I meant. 2.0 would one day come out

[marti4d]

Uuid0 and Uuid1 types

Yup that's what I meant. 2.0 would one day come out

Hmm... Now that I think about this more though, I'm not sure that approach will scale well in the code? There are also other dependencies like time and json that will also certainly go through their own version changes. I could see the code getting very large if we were to have uuid_0(), uuid_1(), time_0(), time_1(), time_2(), etc.

Perhaps it's better to just do dual-support when a transition is happening? IE change the default dependency to the new version and provide a "backward-compatibility" type for a while?

(I think the only reason I have to do this for my project is because we're in a weird place where the DB I want to use (Postgres) supports Uuid v1.0 and the others don't)

Assuming you like this plan, I made PR #329 that makes uuid ^1 the default Uuid type, and clients have to use uuid_0() functions if they want to use new sea-query versions but need uuid 0.8 support.