search

diesel-rs / diesel

A safe, extensible ORM and Query Builder for Rust
https://diesel.rs
Apache License 2.0
12.74k stars 1.07k forks source link

Support CString #1495

Closed SoniEx2 closed 6 years ago

SoniEx2 commented 6 years ago

Problem Description

What are you trying to accomplish?

I'd like to be able to use CString in my model.rs.

Checklist

sgrif commented 6 years ago

Can you give some more context on why you need it, and why you think it's a good fit for Diesel to support this type? CString is meant for heavy interaction with C code, not databases.

SoniEx2 commented 6 years ago

I use CString as a form of NulString with unspecified encoding. This is extremely useful to me because I do a lot of interactions between embedded systems and they use C strings with "unspecified" encoding in their protocols, and some of those C strings are used when interacting with the DB and vice-versa. Validating that there are no nuls on the way in and on the way out would be nice, as well as avoiding any copies and conversions until the thing hits the Diesel or DB layer.

sgrif commented 6 years ago

as well as avoiding any copies and conversions until the thing hits the Diesel or DB layer.

There's actually no copies involved in converting to an &str, and Diesel won't copy it until we need to in order to send to the DB. In terms of data coming from the DB, the same amount of copies will be required regardless of whether it's a CString or String.

My gut reaction is to close this. We don't generally add support for types which don't have a direct mapping to a SQL type for a given database. I suspect Vec<u8> is probably a more appropriate type here. You can pretty easily add support for this in your own app by using a wrapper struct, as well.

That said, given that this is a type in the standard library (e.g. there's zero chance of you opening a PR on the other side adding Diesel support), and this is more reasonable for us to support than most other types proposed, I'm going to leave this open for a few days to think about it and for others to give feedback.

SoniEx2 commented 6 years ago

Actually, in this case CString has an "unspecified" encoding. It's basically a nul-terminated array of bytes for all intents and purposes. This has a slight (read: pretty big) difference compared to String.

I'd either have to convert encoding at protocol boundary (fast, but may cause unnecessary/unused copies), or every time at DB/Diesel boundary (lots of redundant copies, but avoids the previous issue). I don't like either.

sgrif commented 6 years ago

I suspect Vec<u8> is probably a more appropriate type here.

SoniEx2 commented 6 years ago

Ok. How do I put a Vec into a varchar while avoiding copies?

sgrif commented 6 years ago

Right now you'd need to convert to a &str first (which you can do for free and without copies) -- we should probably add ToSql and FromSql impls for Vec<u8> and &[u8] for text columns.

sgrif commented 6 years ago

I actually take that back -- there's no reason for us to support types other than String or &str on text columns, and you do need to ensure that they're UTF-8 encoded. We are specifying the client encoding on both MySQL and PG, which means that the database is sending us data encoded as UTF-8, and expects any text data in that encoding. SQLite only supports UTF-8 for text columns to begin with. You should probably use a binary column if you actually want unspecified encodings.

SoniEx2 commented 6 years ago

So what I should do is forget Diesel and use raw SQL?

sgrif commented 6 years ago

Your database is going to need a client encoding regardless of how you access it. Text columns have encodings, and wire protocols also have encodings. If you don't want them, you should use a different data type. That's not a diesel thing, that's how databases work.

On Thu, Jan 18, 2018, 4:55 PM Soni L. notifications@github.com wrote:

So what I should do is forget Diesel and use raw SQL?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/diesel-rs/diesel/issues/1495#issuecomment-358821677, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdWK77PWXJpECZF6j86dQsTPkX_vfhBks5tL9n8gaJpZM4RjXkS .

SoniEx2 commented 6 years ago

Shouldn't you try to match wire protocol encoding with text column encoding to reduce conversions as much as possible?

sgrif commented 6 years ago

It's a connection level setting. Not something that can really be handled at a column level.

On Thu, Jan 18, 2018, 5:33 PM Soni L. notifications@github.com wrote:

Shouldn't you try to match wire protocol encoding with text column encoding to reduce conversions as much as possible?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/diesel-rs/diesel/issues/1495#issuecomment-358828870, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdWKwChnwsztjnQgnq2oEVgD07_x4i8ks5tL-LzgaJpZM4RjXkS .