Skip to content
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

postgres camelCase columns #689

Closed
powellnorma opened this issue Nov 28, 2022 · 13 comments
Closed

postgres camelCase columns #689

powellnorma opened this issue Nov 28, 2022 · 13 comments
Projects

Comments

@powellnorma
Copy link

Hello again, I tried out PostgresEngine but noticed that using camelCase column names leads to problems. Is this something the ORM could potentially "abstract away", or would that be a bad idea?

@dantownsend
Copy link
Member

It's generally safest to use snake case column names.

We had some bugs in the past with Piccolo Admin and camel case column names, and I think we fixed those. There could be outstanding bugs in Piccolo with camel case column names - we would have to write a bunch of unit tests to see.

@powellnorma
Copy link
Author

In my case I got an UndefinedColumnError.

Ok, so let's say I use snake_case for all my columns. Is there an easy (as in few lines of code) way to convert these to camelCase once I pass it to the json endpoint? I guess one could write an function which converts all dictionary keys to camelCase?

@dantownsend
Copy link
Member

In my case I got an UndefinedColumnError.

UndefinedColumnError probably means that somewhere we're not wrapping the column name in double quotes.

Ok, so let's say I use snake_case for all my columns. Is there an easy (as in few lines of code) way to convert these to camelCase once I pass it to the json endpoint? I guess one could write an function which converts all dictionary keys to camelCase?

Yes, writing a function to convert the names should be quite straight forward. Or just do this in the query:

await MyTable.select(MyTable.some_column.as_alias('someColumn'))

@powellnorma
Copy link
Author

UndefinedColumnError probably means that somewhere we're not wrapping the column name in double quotes.

I found some code to reproduce this:

from piccolo.columns.column_types import Varchar, ForeignKey
from piccolo.engine.postgres import PostgresEngine
from piccolo.table import Table
from starlette.applications import Starlette
import uvicorn


DB = PostgresEngine(config={
    'host': 'localhost',
    'database': 'test',
    'user': 'test',
    'password': ''
})

class MyOtherEntity(Table, db=DB):
    firstName = Varchar()

class MyEntity(Table, db=DB):
    firstName = Varchar()
    otherEntity = ForeignKey(MyOtherEntity)

async def on_startup():
    e_other = await MyOtherEntity.objects().get_or_create(MyOtherEntity.firstName == "Clark")
    e = await MyEntity.objects().get_or_create((MyEntity.firstName == "James") & (MyEntity.otherEntity == e_other))
    print(e)
    e = await MyEntity.select(
        MyEntity.otherEntity.all_columns(),
    ).first()
    print(e)

app = Starlette(debug=True, on_startup=[on_startup])

if __name__ == '__main__':
    MyOtherEntity.create_table(if_not_exists=True).run_sync()
    MyEntity.create_table(if_not_exists=True).run_sync()
    uvicorn.run(app, host='127.0.0.1', port=64215)

It would be great if you could take a look!

Or just do this in the query

Yes, but that way I would have to do this for each column (that needs to be converted) explicitly, which could get a bit verbose

@sinisaos
Copy link
Member

@powellnorma You are right and this doesn't work, although I don't understand why because the querystring looks fine to me.
camelCase:
SELECT "my_entity$otherEntity"."id" AS "otherEntity$id", "my_entity$otherEntity"."firstName" AS "otherEntity$firstName" FROM my_entity JOIN my_other_entity my_entity$otherEntity ON (my_entity.otherEntity = my_entity$otherEntity.id) LIMIT 1;
snake_case:
SELECT "my_entity$other_entity"."id" AS "other_entity$id", "my_entity$other_entity"."first_name" AS "other_entity$first_name" FROM my_entity LEFT JOIN my_other_entity my_entity$other_entity ON (my_entity.other_entity = my_entity$other_entity.id) LIMIT 1;
but with camelCase columns there is an error
asyncpg.exceptions.UndefinedColumnError:column my_entity.otherentity does not existHINT: Perhaps you meant to reference the column "my_entity.otherEntity".
even though that column exists in the querystring.

It might be best to use snake_case and then convert it to camelCase something like this:

def to_camel_case(s) -> str:
    return s.title().replace("_", "")

and then you can use that function in all yours json endpoints something like this:

async def index(request) -> JSONResponse:
    data = await MyEntity.select(
        MyEntity.all_columns(), MyEntity.other_entity.all_columns()
    ).first()
    return JSONResponse({to_camel_case(k): v for k, v in data.items()})

@dantownsend I think it would be best if we set some coding style in the docs (something like Django) because camelCase is not common in either Python or Postgres, and that would solve these kind of confusions and awkward bugs that are hard to detect.

@powellnorma
Copy link
Author

@sinisaos Thank you for your response. I think the problem is that postgres converts upper case letters to lower case, unless one quotes the column names. This phenomenon is described in more detail here: https://stackoverflow.com/a/20880247

So we probably just have to add some quotes somewhere, but I am not yet sure where

@sinisaos
Copy link
Member

@powellnorma You are right about that and it would be easy if we write raw sql queries like this one

# same query as data = await MyEntity.select(MyEntity.otherEntity.all_columns()).first()
data = await MyEntity.raw(
        'SELECT o.* FROM my_entity m LEFT JOIN my_other_entity o ON ("m"."otherEntity" = o.id) LIMIT 1;'
    ) 

but in orm we would have to check if the column is snake_case or camelCase and then create a querystring for one and the other case, check edge cases etc. I think it would be much easier to prescribe (and indicate in the docs) that the column name must be lower-case snake_case (like Django and like is advised in that SO response) and avoid potential confusions and awkward bugs that are hard to detect.

@powellnorma
Copy link
Author

powellnorma commented Nov 29, 2022

But wouldn't it be enough to just always use quotes? Or are there downsides to always using quotes? Also, what if someone used another ORM before, which allowed camelCase columns, but now wants to switch to piccolo? I personally think it would be great if piccolo would support this!

@sinisaos
Copy link
Member

@powellnorma I changed this line

_joins.append(
f"LEFT JOIN {right_tablename} {table_alias}"
" ON "
f"({left_tablename}.{key._meta.name} = {table_alias}.{pk_name})" # noqa: E501
)
to always use double quotes like this

_joins.append(
    f'LEFT JOIN "{right_tablename}" "{table_alias}"'
    " ON "
    f'("{left_tablename}"."{key._meta.name}" = "{table_alias}"."{pk_name}")'  # noqa: E501
)

and it seems to work for both snake case and camelCase. I'll run a unit test against it now and see if there are any edge cases, but @dantownsend need to see and aprove that.

@dantownsend
Copy link
Member

@sinisaos Yeah, we should wrap the columns names in double quotes. Good catch.

@sinisaos
Copy link
Member

@dantownsend The unittest passed and I can do a PR if you want. @powellnorma suggested that change.

@powellnorma
Copy link
Author

powellnorma commented Nov 29, 2022

@sinisaos Thank you for your patch, it is very useful to me. I can now use my existing schema without problems. I haven't had any other camelCase related bugs/problems since. So it lgtm

@dantownsend
Copy link
Member

I've merged in the fix. Will do a new Piccolo release later on.

@dantownsend dantownsend added this to Needs triage in Bugs via automation Nov 30, 2022
@dantownsend dantownsend moved this from Needs triage to Closed in Bugs Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Bugs
Closed
Development

No branches or pull requests

3 participants