Issue: Playhouse Migrator does not honour model schema
Author: berzi
State: closed
Created: 2023-02-16 09:41:36

Sorry for not including a full code example: I'm using playhouse.migrate in a utility and it would be more than overkill to include all of it, not to mention it's closed-source company code I'm not allowed to share. But I believe there's enough information to tackle the issue.

In all the scenarios below, I call `migrate(*actions)` where `actions = [migrator.add_column(table_name, "fieldname", TextField(default=""))]` or similar. `table_name` is always `MyPeeweeModel._meta.table_name` and the model always has `Meta.schema` set to a constant value. Note that I'm using Postgresql (both for the database and the migrator, obviously).

Note: if I want to create the table for my model it seems my only options are either raw SQL or `database.create_tables()`. Setting aside the fact that I don't want to use the latter because it creates fields for the *current* state of the model, which isn't what I want, if I do use it, it **does** create the table in the correct schema.

Further note: there seems to be no way to create the schema itself, so I resorted to doing this manually. I can't remember if `.create_tables()` attemps doing it at all but as I said, I need to bypass that and create the table manually to prevent inconsistencies between the table's initial state and any migrations.

**Scenario 1:** I attempt:
```python
migrate(
    # table_name is simply MyPeeweeModel._meta.table_name
    migrator.add_column(table_name, "fieldname", TextField(default="")),
)
```
*Result:* "peewee.ProgrammingError: relation "mytable" does not exist". The table *does* exist in the database at that point, so I must surmise that the schema is not prepended to the path to the table; in other words Peewee looks for the table without honouring the schema.

**Scenario 2:** I attempt:
```python
migrate(
    # table_name is a string like schema.table where the schema comes from MyPeeweeModel._meta.schema
    migrator.add_column(table_name, "fieldname", TextField(default="")),
)
```
*Result:* "peewee.ProgrammingError: relation "myschema.mytable" does not exist". Again, the table actually exists in the right schema. The relation which isn't found is incorrectly quoted, because it enquotes myschema.mytable entirely instead of the schema and the table separately.

**Scenario 3:** I attempt:
```python
migrate(
    # table_name is a pre-quoted string like "schema"."table"
    migrator.add_column(table_name, "fieldname", TextField(default="")),
)
```
*Result:* "peewee.ProgrammingError: relation ""myschema"."mytable"" does not exist". Same as above: the whole thing is quoted regardless of the fact that the identifiers are already quoted.

**Scenario 4:** As I traced back the quoting Peewee does to the `Entity` class and I noticed its init:
```python
def __init__(self, *path):
        self._path = [part.replace('"', '""') for part in path if part]
```
takes an iterable, I tried giving it a tuple like `(schema, table)` but that didn't work, and if I try to place a breakpoint in the init, my debugger crashes every time while trying to collect data (presumably on `self`'s attributes), even if I monkeypatch the whole function (!). As such, I'm quite stuck and am also unable to open a pull request to fix the issue myself, since debugging it seems nigh on impossible.

I'm honestly surprised this hasn't come up before: has nobody ever tested or tried to use the migration helpers with a non-default schema?

I monkeypatched this on my end like so:
```python
def __sql__(self, ctx):
    return ctx.literal(".".join(self._path))
Entity.__sql__ = __sql__
```
and then I pass `schema.table` as table name to each migrator action, but that gets rid of quoting entirely (and I can't even manually add them in my table path manually because, for reasons unclear to me, Entity does `.replace('"', '""')`, causing zero-length identifiers in the query), so this is not a general solution that I could submit as a pull request. I don't have the time to delve deeper into Peewee's code to know what the most effective fix would be, much less write tests for these cases.

But I suspect it isn't actually a hard fix for someone who knows the codebase well, so I hope for a quick reaction to this issue.

Have you tried the suggestion from the docs?

>Postgres users may need to set the search-path when using a non-standard schema. This can be done as follows:

```
new_field = TextField(default='', null=False)
migrator = PostgresqlMigrator(db)
migrate(
    migrator.set_search_path('my_schema_name'),
    migrator.add_column('table', 'field_name', new_field))
```

http://docs.peewee-orm.com/en/latest/peewee/playhouse.html#PostgresqlMigrator.set_search_path

Doing this on the migrator seems rather hard to find for the user. I
understand why it makes sense that it's there from Peewee's perspective,
but is there no way for Peewee to get it automatically from the model?

After all, different models might be on different schemas and that would
mean having to either make multiple migrators or change the migrator's path
mid-way, which hinders and complicates more generic systems that install
migrations seemingly unnecessarily.

It's also, honestly, a hard to discover edge case.

On Thu, 16 Feb 2023, 17:35 Charles Leifer ***@***.***> wrote:

> Have you tried the suggestion from the docs?
>
> Postgres users may need to set the search-path when using a non-standard
> schema. This can be done as follows:
>
> new_field = TextField(default='', null=False)
> migrator = PostgresqlMigrator(db)
> migrate(migrator.set_search_path('my_schema_name'),
> migrator.add_column('table', 'field_name', new_field))
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/coleifer/peewee/issues/2678#issuecomment-1433372035>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHY3U437IIXFWCJX2CQW6FLWXZJM5ANCNFSM6AAAAAAU55VVAQ>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>


Moreover, it just doesn't work: I get the same error if I use `migrator.set_search_path('my_schema_name')`: Peewee quotes the whole thing and not the schema and table separately.

I also found out that for any ForeignKeyField, the related model's table is looked up without schema.

That doesn't make sense because it should be setting the schema on the connection so you don't specify it anywhere. Note that the migrator does not operate on a model, it takes a table name. Can you explain how it's putting the schema and table in quotes?

I'm not really sure, the logic inside Peewee is rather convoluted and I don't know it well enough. To be clear: when testing your suggestion I disabled my monkeypatching entirely, so I'm not meddling with Peewee's code at all when it fails.


Regarding this:

> I also found out that for any ForeignKeyField, the related model's table is looked up without schema.

I'm pretty sure this happens in `SchemaMigrator.add_inline_fk_sql()`, where:

```python
def add_inline_fk_sql(self, ctx, field):
    ctx = (ctx
           .literal(' REFERENCES ')
           .sql(Entity(field.rel_model._meta.table_name))  # <== schema is not taken into account here
           .literal(' ')
           .sql(EnclosedNodeList((Entity(field.rel_field.column_name),))))
    if field.on_delete is not None:
        ctx = ctx.literal(' ON DELETE %s' % field.on_delete)
    if field.on_update is not None:
        ctx = ctx.literal(' ON UPDATE %s' % field.on_update)
    return ctx
```

I think something is wrong, because it works fine for me. Example script:

```python

class User(db.Model):
    key = CharField()
    class Meta:
        schema = 'custom'
        table_name = 'users'

class Tweet(db.Model):
    content = TextField()
    class Meta:
        schema = 'custom'

db.create_tables([User, Tweet])

print(db.get_columns('tweet', 'custom'))

# Uh-oh, forgot to add the "user_id" foreign-key from Tweet -> User.
user = ForeignKeyField(User, field=User.id, null=True)

migrator = PostgresqlMigrator(db)
migrate(
    migrator.set_search_path('custom'),
    migrator.add_column(Tweet._meta.table_name, 'user_id', user),
)

print(db.get_columns('tweet', 'custom'))
print(db.get_foreign_keys('tweet', 'custom'))
```

The output is correct:

```
# Before migrate
[
  ColumnMetadata(name='id', data_type='integer', ...), 
  ColumnMetadata(name='content', data_type='text', ...)
]

# After migrate
[
  ColumnMetadata(name='id', data_type='integer'...),
  ColumnMetadata(name='content', data_type='text', ...),
  ColumnMetadata(name='user_id', data_type='integer', ...)
]

# Our foreign-key shows up fine:
[ForeignKeyMetadata(column='user_id', dest_table='users', dest_column='id', table='tweet')]
```