Ff00ff/mammoth

star() function returns results with snake_case fields

Closed this issue · 4 comments

echentw commented

Querying a table

const dogs = defineTable({
  breedName: text().notNull(),
});

with

const rows = await db.select(star()).from(db.dogs);

Results in rows having a type of {breedName: string}[], but the actual value that gets returned is of the form {breed_name: string}[].

martijndeh commented

I was about to create this issue. Mammoth is automatically transforming train_case to camelCase, but when doing a star() the actual returned columns are not transformed because the resulting query is just a SELECT * FROM dogs (vs SELECT breed_name "breedName" FROM dogs). Transformations do not happen in the result set because of performance concerns. In your opinion, how should this be fixed?

  1. Change the star() implementation to not write a * in the SQL but just write all columns with the camelCase alias e.g. breed_name "breedName"? This fixes the issue but moves away from pure SQL version. Also there is a slight difference in behaviour when you introduce a new column it's not automatically included when referring to the columns explicitly.
  2. Remove the camelCase transformation from Mammoth completely so * just works as expected. This is a breaking change and I feel it makes using Mammoth less practical.
  3. Do the transformation by going through the result set and set aside the performance concerns. Maybe try to optimise to only do this when there is a select star().

What do you think?

cakoose commented

Option 1 is my favorite. Option 2 is my least favorite because I love the camelCase transformation.

I like option 3 because doing transformations enables things like #211, but that seems like a big change.

Also there is a slight difference in behaviour when you introduce a new column it's not automatically included when referring to the columns explicitly.

This will only happen when they add a column to the DB but not to the Mammoth definitions, right? In that case, the TS types would also be missing the field, so it would only affect people who circumvent the TS types, which I think is rare?

But maybe to avoid confusion, star can be renamed to something else, like allFields?

  • 👍2
AuspeXeu commented

Any update on this? I also stumbled over it just now :)

martijndeh commented

This is fixed. Option 1 is implemented.

I tested it in different situations and seems to work nicely. Both when selecting or in with (CTE) queries.

In returning clauses star wasn't completely supported yet. This is still the case but it's on my list as well.

Because it's potentially a breaking change this will probably be released in a major version in the coming days.

  • 🎉1