Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Support user-specified field types #207

Open
kennethuil opened this issue Mar 26, 2023 · 5 comments
Open

Feature request: Support user-specified field types #207

kennethuil opened this issue Mar 26, 2023 · 5 comments
Labels
feature New feature or request needs discussion Requires discussion to reach an actionable state..

Comments

@kennethuil
Copy link

The SQL annotations should let you name a preexisting field type for specific fields. That field type would of course have to implement ToSql/FromSql.

That would let users use their chosen field types without round-tripping through the whitelisted field types (especially whitelisted field types that allocate). It would also make it easier to migrate from handrolled tokio-postgres code by not requiring the user to change any of the types involved (for instance, they could keep using chrono instead of migrating to time).

@jacobsvante
Copy link
Contributor

I think that's a great idea. @Virgiel @LouisGariepy What do you think?

@Virgiel
Copy link
Member

Virgiel commented Mar 31, 2023

We have two options for supporting user-specified field types. We can generate code with user-specified types based on a generation configuration, which would imply that all occurrences of a postgres type would use the same user-specified type. Or we generate code with a generic but typed wrapper, that the user must match to their own type, allowing the use of a specific type for every field.

The first solution is the simplest and closest to what we currently do, the second is better but will stress the ability of the Rust programming language to be dynamic and ergonomic at the same time.

I really want this feature, but it is really hard to implement in a satisfactory way.

@jacobsvante
Copy link
Contributor

jacobsvante commented Apr 3, 2023

which would imply that all occurrences of a postgres type would use the same user-specified type

This is a fair assumption to make IMO. I can't come up with a good example of when users would want something else.

@kennethuil
Copy link
Author

I can think of an example:

I've got one short (or maybe just usually short) VARCHAR column I want to represent with something like a CompactString that gets used all over the place, and some other description VARCHAR columns where CompactString doesn't do any good at all. Maybe a column has a VARCHAR limit small enough that I'd want to just use a heapless String for it. And making a custom Postgres type for it only introduces complications to app developers in other languages (assuming that changing the schema is even feasible).

@LouisGariepy LouisGariepy added feature New feature or request needs discussion Requires discussion to reach an actionable state.. labels May 17, 2023
@LouisGariepy
Copy link
Member

IMO this would mostly be useful when using pre-existing types (you mention compact_str and heapless), but you can't implement ToSql/FromSql for types you don't own.

There's also the fact that ToSql/FromSql should not be implemented recklessly as that will cause runtime errors. Implementing them correctly requires either round-tripping to "whitelisted" types, or having a good knowledge of the postgres wire format.

One thing I'm worried about is a situation where we allow user-specified types, the code generator will happily chug along, but in the end, the user might have a ton of errors in the generated code: types not found, types not implementing ToSql/FromSql. One of my goals with Cornucopia is to give diagnostics at generation time, and AFAICT we wouldn't be able to inform the user: they'd just get a ton of errors at build time.

I'm also not convinced by the "allocation" argument since we offer a non-allocating mapper, which allows you to pass directly from a borrow to your custom type, without intermediate allocations.

One limitation of the mapper is that you must end up with an owned type. This is not a problem for the types you highlighted though. For example heapless::String::from_str copies the bytes under the hood anyway. Generally speaking this limitation is not a problem.

There are some cases where this limitation is unwanted though, for example when you want to query data from the database and serialize it directly to send over an API. In that case, it'd be preferable to do it all with borrowed data. But this is something we could (and should) fix in the current framework, without it requiring direct user type support.


TLDR

  1. I'm worried about the robustness and usability of this feature.
  2. I think we should encourage/contribute to upstream crates to implement ToSql/FromSql when it makes sense.
  3. Otherwise, I think we should encourage the use of the non-allocating mapper.
  4. If the non-allocating mapper has some limitations that prevent this, we should solve them.

I'm not opposed to this feature wholesale, but I do believe it has some drawbacks that definitely warrant in-depth design discussions before going into implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request needs discussion Requires discussion to reach an actionable state..
Projects
None yet
Development

No branches or pull requests

4 participants