cornucopia-rs / cornucopia

Generate type-checked Rust from your PostgreSQL.
Other
755 stars 31 forks source link

Feature request: Support user-specified field types #207

Open kennethuil opened 1 year ago

kennethuil commented 1 year ago

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 commented 1 year ago

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

Virgiel commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.