Add domain types configuration #4594
Conversation
| #[derive(diesel::query_builder::QueryId, Clone, diesel::sql_types::SqlType)] | ||
| #[diesel(postgres_type(name = "posint"))] | ||
| pub struct Posint; |
There was a problem hiding this comment.
Just laying some thoughts here (but I'm not sure we should do what's suggested below): this loses information of what the underlying type is, which makes it the responsibility of the user to make sure that serialization is appropriate.
We might want to add this data to the type here, such as a new Domain derive and underling_type = "..." annotation to the SqlType derive, that would implement a corresponding Domain trait, and allow the user to make use of this type in their implementations of FromSql and ToSql (or even make the relevant implementations conditionally on implementations of some new traits that would define the mapping between input rust types and rust types that have ToSql/FromSql<UnderlyingType>) to fully close the door for mistakes here.
If we were to add that later by default, that would be a breaking change for users who already manually define their FromSql/ToSql implementations for these types.
But I'm not sure if that's ever worth the extra maintenance cost within Diesel itself...
Probably that wouldn't be very consistent with Diesel not supporting enums out-of-the-box, where ideally the CLI would also infer enum variants...
There was a problem hiding this comment.
Yeah I fully agree that some information is being lost here and this should be avoided. I'd be willing to try something along the lines you suggested with some help (since I'm a newbie in this project).
And about this breaking compatibility, is there a way in diesel to add features in an "unstable" manner so it can be worked over multiple releases before being stabilized? If so, maybe we should think of this feature in layers, but I'm not sure about it either.
There was a problem hiding this comment.
@Ten0 I'm re-reading your comment and I'm positive this is the right path, but I'm not sure how to begin to propose a solution. Is there any pointer you could give me to similar code I could read up on in order to get up to speed on this implementation?
Edit:
Or even, how would the Domain trait look like? From what I can gather, you are suggesting: (1) the cli should output postgres_type = "posint" but also underlying_type = "bigint" for example, and (2) the derive should understand the "underlying type" property and generate a Domain implementation (3) could be used on the actual user-level From/ToSql implementations without messing up the type. Does this tracks or I'm way off mark?
In this situation, the Domain trait would be responsible for carrying the information about the "bigint" which was lost when "postgres_type" was set for the domain name.
(I'm just phrasing it so I can serialize my current understanding haha)
Edit2:
or even make the relevant implementations conditionally on implementations of some new traits that would define the mapping between input rust types and rust types that have ToSql/FromSql
I didn't fully grasp this one but it seems interesting too if you're willing to elaborate
There was a problem hiding this comment.
but I'm not sure how to begin to propose a solution
I understand that this comment is blocked on me giving this issue some more thought. I'll come back to you when I have time to do that 😊🙏
For me the main question is whether we even want to do this here:
But I'm not sure if that's ever worth the extra maintenance cost within Diesel itself...
Probably that wouldn't be very consistent with Diesel not supporting enums out-of-the-box, where ideally the CLI would also infer enum variants...
Maybe other reviewers will have an opinion.
There was a problem hiding this comment.
Of course, I wasn't trying to pressure you or anything @Ten0 , sorry if it came off this way.
Is there any way we could release something without having stability guarantees on it? Maybe a feature a feature or something, maybe this could relieve some pressure and give us some time to think on this.
There was a problem hiding this comment.
We do currently not have a way to say: This feature is unstable, that by itself would need to be implemented first. That written: I'm not opposed to have something like that as unstable feature that can be explicitly enabled via an unstable.domain_type = true flag in the diesel.toml configuration.
Finally for the actual discussion: Given that domain types often introduce custom requirements on the underlying types it's hard to see how providing the built-in FromSql/ToSql impls for the underlying types would be helpful. I would guess that users ~always want to express these requirements in their rust code as well, so having these impls by default might be not helpful for them.
As for the discussion whether this should be a boolean flag or a list of type names: If somewhat possible I would prefer a list of type names or even a list of type name regexes there so that users can have finer control around where thy want domain types and where not.
There was a problem hiding this comment.
Hey @weiznich , thanks for the feedback!
I like the idea of prefixing with unstable, seems like a simpler way to avoid this possible improvement. That said, I'll leave this discussion for when we're comfortable with the PR so we know exactly what implementation we are talking about!
About the list of type names, I updated the implementation on 14cbf02 trying to implement a version of this idea. I think it looks way better than the boolean, do you mind checking it out and giving feedback?
I updated the tests too to reflect usage.
There was a problem hiding this comment.
Should I do anything special for the diesel.toml file to be read on the test? I swear the commit passed the test once but I changed something and can't seem to do it again. haha.
|
@Ten0 What do you think of instead of a boolean we have a list of type names to be considered as is? The reason I suggest this is that the implementation would look very similar, while adding more control for the users. Instead of all-or-nothing domain types, the user could allowlist the domains they want. If it makes sense, we could have "true" representing all, and a list representing allowlist, or something along these lines. I'm not sure about it, but I'd like to hear what you think about it |
This PR intends to be the point of discussion of a possible feature discussed in #4592.
Concept
Domain types are regular types with some added constraints. Reference.
The current behavior when using them with diesel is that diesel prints the underlying type, which makes sense as a default. As an alternative behavior, this PR proposes that a new type is generated if the user wants to add behavior to the domain type on the application layer.
Details
I'm not entirely happy with the last commit because I had to use "boxed" in order to have two distinct queries in the same variable, which still having to add "if" on the flag. If you can think of a better way to express the logic in diesel I'd love to learn it and change the implementation.
Questions