IF ELSE statements

[gronke]

PR Info

• Dependencies:
  • Triggers #824

New Features

• IF ELSE statements

Breaking Changes

• Add SimpleExpr::IfElse variant to public enum

[Expurple]

Haven't reviewed the rest yet. Consider running cargo semver-checks to find and document the breaking changes.

Disclaimer: I'm not a maintainer

[Expurple]

You should mention that this is a breaking change (adding a new variant to a public enum not marked as #[non_exhaustive]).

Perhaps, we should add #[non_exhaustive] in the next major version. That's also a breaking change. But after that, changes like yours can me merged anytime with no problems

[gronke]

Thank you for the hint, I've updated the PR body accordingly.

Perhaps, we should add #[non_exhaustive] in the next major version. That's also a breaking change. But after that, changes like yours can me merged anytime with no problems

At some point the pool of statements will be exhausted. I agree though that for the time being, we can expect more variants to appear. Without even noticing that introducing a new variant is of course a breaking change, I've opened this as a draft to collect feedback.

For context, when combined with triggers #824 and custom errors #829 it becomes possible to implement fully custom constraints with logic. Maybe it is justified group such breaking changes into one.

Instead of adding #[non_exhaustive] to the enum, I could also imagine creating a Milestone on GitHub with an exhaustive list of remaining missing variants across the supported backends.

[Expurple]

Instead of adding #[non_exhaustive] to the enum, I could also imagine creating a Milestone on GitHub with an exhaustive list of remaining missing variants across the supported backends.

This doesn't solve the problem with frequent breaking changes as each variant is implemented, unless you batch features into rare big major releases. But implementing every single database feature is unrealistic, especially in any short period of time. And these databases are going to gain new features in the future, anyway. Generally, I expect ORMs to always lag slightly behind in terms of database features. And I think, we need a way to continuously add new features without breaking changes. This is better than leaving unmerged PRs to rot until the next major release.

Since the list of database features is always a moving target, I don't see the point in keeping this enum exhaustive. But maybe a maintainer will clarify things. Maybe there's some practical benefit to exhaustiveness here. E.g. some external crate (like sea_orm) could rely on supporting sea_query exhaustively and doing something specific for each variant. And if SimpleExpr becomes #[non_exhaustive], that other crate would have to either change its signatures to be fallible, or lock the sea_query version to guarantee exhaustive coverage (and always remember to manually maintain it when bumping sea_query).

[Expurple]

Added #[non_exhaustive] to my list of wanted breaking changes (#795)

[Expurple]

I don't like this panic. See #829 (comment). Although, I don't know anything about the backend design in sea_query. Maybe that's just how we do things here.

Maybe, SimpleExpr just shouldn't contain non-portable variants and we should come up with a different way of handling IfElseStatement.

@tyt2y3 @Huliiiiii, do you have any thoughts?

[Huliiiiii]

Yes, our usual approach is to panic when features are not supported.

There are two ways to solve this problem:

1. Make IfElseStatement behind the feature gate
2. Modify these kind of functions to return result (breaking)

[Expurple]

Make IfElseStatement behind the feature gate

I think, it doesn't solve all the issues. It's nice that you won't be able to use if-else if you activate only sqlite backend. But if you activate e.g. sqlite+postgres backends, then this syntax becomes available and you can pass it to the sqlite backend and hit a runtime panic.

Ideally, it should be impossible to pass. But I'm not sure how achievable for us and convenient for the user is that. Someone needs to experiment and document the results.

Modify these kind of functions to return result (breaking)

This is a more achievable route that we could explore and see if it's annoying for the user. Sadly, I don't have time for this right now

[gronke]

We could implement backend specific features, closely resembling local specifics. Later then we could build a general IfElseStatement that normalizes the expression of logic and polyfills for each backend.

My personal use case for If-Else statements are triggers. Triggers could be made conditional on SQLite with WHEN clauses by splitting up the one trigger into multiple. Elsewhere our conditions could break down to CASE expressions. Would love to hear whether that would be a general direction to consider.

Meanwhile we could prepare such step by building backend specific features that only later become available globally. It would be nice to avoid panicking, but it would also be possible to solve that later by introducing a polyfill.