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

Migration error #111

Closed
NewDay1313 opened this issue Jun 14, 2021 · 10 comments
Closed

Migration error #111

NewDay1313 opened this issue Jun 14, 2021 · 10 comments

Comments

@NewDay1313
Copy link

NewDay1313 commented Jun 14, 2021

Running Postgres version 13.3
Running migrations ...
All migration ids = ['2021-06-14T23:35:51', '2021-06-14T23:45:55', '2021-06-14T23:53:23', '2021-06-14T23:57:34']
Haven't run = ['2021-06-14T23:35:51', '2021-06-14T23:45:55', '2021-06-14T23:53:23', '2021-06-14T23:57:34']
Running MigrationManager ...
The command failed.
user is a protected name, please give your table a different name.

I don't have any table with this name.

Before that there was table naming error!

manager.add_table("Sessions", tablename="sessions")
manager.add_table("SessionsBase", tablename="sessions")  
@dantownsend
Copy link
Member

@NewDay1313 Piccolo checks in case a Table has a tablename of 'user'. For example, either of these:

class User(Table):
    ...

class MyTable(Table, tablename='user'):
    ...

It's because user is a reserved word in Postgres, and having a table with the same name would cause issues.

Do you think that's the case here?

@NewDay1313
Copy link
Author

It's because user is a reserved word in Postgres, and having a table with the same name would cause issues

Got it! It explains everything!

Do you think that's the case here?

Definetly It takes place here!

@Anonymous1313-tech
Copy link

So will you change that table name in piccolo?

@dantownsend
Copy link
Member

I don't think any of the builtin migrations for Piccolo use a tablename of user.

Check any migration files which Piccolo has created for you, in case any of the reference a table called user.

If you want you can paste the code here and I'll take a look.

@heliumbrain
Copy link

heliumbrain commented Jun 28, 2021

@dantownsend playing around with this last night I noticed that there might be some inconsistency in how the PROTECTED_TABLENAMES check is applied.

in piccolo/table.py:148

tablename = tablename if tablename else _camel_to_snake(cls.__name__)

if tablename in PROTECTED_TABLENAMES:
    raise ValueError(
        f"{tablename} is a protected name, please give your table a "
        "different name."
    )

Can't figure out why, but it seems that tablename will always resolve to cls.__name__

For example, when inheriting from BaseUser:

class User(BaseUser, tablename="my_users"):
    pass

The migrations are succesfully generated (using piccolo migrations new my_app --auto) with manager.add_table("User", tablename="my_users")

How ever, trying to push the migrations with piccolo migrations forwards my_app gives the same error as mentioned in this thread:

user is a protected name, please give your table a different name.

@dantownsend
Copy link
Member

dantownsend commented Jun 28, 2021

@heliumbrain You're right, there is an issue. Good catch.

The problem is here:

_Table: t.Type[Table] = type(
add_table.class_name,
(Table,),
{
column._meta.name: column
for column in add_table.columns
},
)
_Table._meta.tablename = add_table.tablename

Piccolo creates a Table class, but only sets the tablename afterwards, so it's triggering this warning when the table is called 'user'. I'll have a read of the docs for type to see if parameters can be passed to it.

@dantownsend
Copy link
Member

Looks like types.new_class will do the trick:

https://docs.python.org/3/library/types.html#types.new_class

@heliumbrain
Copy link

Yeah, looks good! I can have a shot at fixing it tomorrow, if you're not on it already @dantownsend :)

@dantownsend
Copy link
Member

Should be fixed now - I've push a new build to PyPI.

@dantownsend
Copy link
Member

Closing for now - let me know if the problem still persists. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants