kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.8k stars 275 forks source link

Document that Postgres queries pass numerical args as text #732

Closed mezuzza closed 10 months ago

mezuzza commented 1 year ago

This following query breaks for me.

function clampedValue<DB>(
  n: number
): RawBuilder<number> { 
  return sql`LEAST(${MAX_VALUE}, GREATEST(${MIN_VALUE}, ${n}))`;
}

The error provided is:

error: column "blah" is of type integer but expression is of type text

It makes sense that number parameters are inherently a bit awkward to handle since javascript doesn't support real integer values. Still, I didn't expect this and knowing how to cast these values wasn't obvious. My original query actually looked like

function clampedValue(
  n: number | Expression<number>
): Expression<number> { 
  const eb = expressionBuilder<DB, never>();                          
  const min_clamped = eb.fn("greatest", [eb.val(MIN_AFFECTION), eb.val(n)]);

  return eb.fn("least", [eb.val(MAX_AFFECTION), min_clamped]);              

It really wasn't obvious how to cast any of these expressions without dropping to the raw sql. The documentation isn't clear about this pitfall. I'm assuming that it comes from pg which does document it, but it's pretty hard to notice. Some things that could help:

  1. Document what types to expect to receive and what types you'd expect to send to the DB.
  2. Provide some way to cast (Though you guys are already on it - #616)
  3. Probably not possible and probably way out of scope, but it would be nice if you could send an actual number to the db. Maybe you could add a function like eb.type.int(2) which would convert the number to a raw byte array and send that along if a given client supports this behavior.
koskimas commented 1 year ago

We should rather document that kysely doesn't touch the data in any way. Each dialect returns and accepts what the underlying driver does. We're also planning on adding a recipe that shows how to change the default behavior. Most drivers have their own mechanisms for mapping db types to javascript types.

Kysely doesn't have the needed information to make the transformations since the Dialect API is on a higher level of abstraction. That decision was made to make 3rd party dialects easier to write (which clearly worked).

If we documented every dialect's input and output formats in every case, we'd duplicate the underlying driver's documentation and we'd need to keep them up to date.

mezuzza commented 1 year ago

Fair enough. There's a gap in documentation around the types that are sent and received. It would help to point to the driver in a documentation page. That would certainly be sufficient.

igalklebanov commented 1 year ago

Fair enough. There's a gap in documentation around the types that are sent and received. It would help to point to the driver in a documentation page. That would certainly be sufficient.

we already do that in "getting started" when picking a dialect. any other places?

mezuzza commented 1 year ago

The problem is that it's not obvious at first what the philosophical role of a Dialect is. Your decision to not touch the typing that the dialect accepts as input or produces as output makes sense, but that's not explicit in the documentation.

There are two places that talk about the dialect. One is in the getting started page:

For Kysely's query compilation and execution to work, it needs to understand your database's SQL specification and how to communicate with it.

The other is in the Dialects page:

A dialect is the glue between Kysely and the underlying database engine

Neither of these explain clearly enough what the dialect does. It's obvious if you think about it after you're already used to the product, but when you're just getting started, it's a lot to grok. I think a little bit of prose could solve a couple hours of headaches for every new user.

Personally, I think there should be a section of the dialects page which explicitly talks about the relationship between kysely and dialects. Further, there should be a section that's devoted to "database types" which would surface well on the documentation search widget.

I had no idea whether kysely was hacking things together and sending data as strings to the underlying dialect or if the dialect was doing that, but because kysely is my frontend, I'm going to search through its documentation first to see if I'm doing something wrong. Honestly, the first thing I thought was that I screwed up the query somehow and that the I searched for types and didn't find anything about database typing at all, so I had to think about the architecture of kysely to figure out that the dialect is probably responsible. This process took a couple hours and a mention about database types and casting would solve that completely.

Something like:

The types you send to and receive from your database are dependent on the underlying dialect. If you have a question on why your database query is failing a type check, see the documentation for your dialect. Here's a link for each of the first party dialects.

If you need to cast your types, please use the cast function or use a sql template tag and implement it according to your database as we don't currently support this feature, but we're working on it #616.

I'm happy to add that documentation as well btw assuming that all I need to do is edit existing .md or similar files.

koskimas commented 1 year ago

So you didn't read the getting started section? Documentation is tricky. Your fixes would fix the issue for YOU but probably nobody else. I wouldn't understand what your suggestion means. Our dialect documentation doesn't document the types since it's the underlying 3rd party driver that's responsible for the conversions. Not the dialect.

mezuzza commented 1 year ago
  1. I did read the getting started section. I'm saying that it doesn't say enough about what dialects do and don't do.
  2. Documentation is tricky. I agree. I took into account whether this fixes it only for me or is a general problem before I made this issue. I think a few paragraphs would help a lot of new people coming to your library.
  3. I know your dialect isn't responsible for type conversions, but that isn't mentioned anywhere in your documentation.

I was only providing feedback and a suggestion. If you don't like it, it's fine, just close the issue and if it pops up again, fix the documentation.

koskimas commented 10 months ago

This is now documented here https://kysely.dev/docs/recipes/data-types