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

Support CString #1495

Closed
SoniEx2 opened this Issue Jan 18, 2018 · 12 comments

Comments

Projects
None yet
2 participants
@SoniEx2

SoniEx2 commented Jan 18, 2018

Problem Description

What are you trying to accomplish?

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

Checklist

  • I have already looked over the issue tracker for similar issues.
@sgrif

This comment has been minimized.

Member

sgrif commented Jan 18, 2018

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

This comment has been minimized.

SoniEx2 commented Jan 18, 2018

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

This comment has been minimized.

Member

sgrif commented Jan 18, 2018

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

This comment has been minimized.

SoniEx2 commented Jan 18, 2018

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

This comment has been minimized.

Member

sgrif commented Jan 18, 2018

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

@SoniEx2

This comment has been minimized.

SoniEx2 commented Jan 18, 2018

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

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 18, 2018

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

This comment has been minimized.

Member

sgrif commented Jan 18, 2018

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

This comment has been minimized.

SoniEx2 commented Jan 18, 2018

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

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 19, 2018

@SoniEx2

This comment has been minimized.

SoniEx2 commented Jan 19, 2018

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

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 19, 2018

@sgrif sgrif closed this Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment