Feedback needed

[dantownsend]

Piccolo is now used in production on several projects. The API is pretty stable. I'd appreciate any beta testers, who will install the library, provide feedback on the docs, and highlight any issues they have.

[gazorby]

Hi!

First, thanks for this project!

I started using piccolo on a new project and I really like the SQL like syntax, auto migrations, modularization and the overall simplicity without doing too much under the hood.

After playing with it some hours, here are some issues I encountered :

• In the docs, Authentication>Registering the app, it says to add piccolo.apps.user.piccolo_conf to the AppRegistry, but shouldn't it be bepiccolo.apps.user.piccolo_app instead? I got python complaining about not finding the module and I solved it by changed this path.

• I ran into some troubles when trying to add a column to a table with existing rows. The auto migration didn't forwards because my field was non nullable (but with a default) and something was needed to populate existing rows. Did I miss something? Or it's just that it's not possible yet?

• Is there a way to access the Table instance after calling save()? The save method returns an Insert object but I didn't see how to get inserted values with it.
  I ask this because I have a foreign key in a table and I have to create and save the referenced table's instance first, then query it just after and finally instanciate the first instance using the id from the queried result as the foreign key. If the save() method could have returned inserted values it could have saved one query in this particular situation (because there is only one foreign key in the table).
  Maybe I just didn't understand well how to do it, so I'd be glad If you could enlighten me.

Long life to piccolo ;)

[dantownsend]

Hi Matthieu! Thanks for the feedback, it's much appreciated.

With regards to your first point - good catch, that is a typo, which I will fix shortly.

There is currently a limitation with auto migrations when adding a new column to an existing table, it has to be set to null=True. I'll add this to the docs, and will create an issue to get this rectified.

With the save method, it can be a bit confusing - you still have to run it. Effectively the save method is just a proxy to an insert or update query. To get the table instance you do something like this:

person = Person(name="Bob")
await person.save().run()
print(person.id)
>>> 1

If you want to fetch the instance from the database:

person = await Person.objects().where(Person.name == "Bob").first().run()

# We can then update it:
person.name = "Fred"
await person.save().run()
print(person.name)
>>> Fred

With a schema like this:

class Company(Table):
    name = Varchar()

class Person(Table):
    name = Varchar()
    company = ForeignKey(Company)

You can handle foreign keys as follows:

company = Company(name="Widget Co")
await company.save().run()

person = Person(name="Bob", company=company.id)
await person.save().run()
print(person.company)
>>> 1

You can also get the referenced table instance as follows:

person = await Person.objects().where(Person.name == "Bob").first()
manager = await person.get_related('manager').run()
print(manager.name)
>>> Widget Co

Hopefully that helps - any other questions, feel free to ask.

[gazorby]

Thanks for your explanation about the save method, I understand better how to use it.

Some other questions that comes up to me :

• Regarding the schema : is there any side effects when extend an existing table by inheritance? For example I extended the BaseUser table to add some fields, but I wonder if it's not better to use a foreign key or if it's just depends of the use case.

• It is possible to leverage postgre's full text search in piccolo? In Django ORM we can define search fields to ease the use of full text search engine, is there anything approaching it in piccolo?

[dantownsend]

I'd recommend using foreign keys to other tables, rather than inheritance at this time. Inheritance should still work, but there are some tricky edge cases.

Take this example - Piccolo auto migrations don't currently know that NameMixin is just an abstract table which shouldn't be created in the database:

class NameMixin(Table):
    name = Varchar()

class Person(NameMixin):
    pass

The workaround for now is to use tags.

Full text search isn't currently supported, but you do have quite a bit of control over text filtering using like and ilike.

await Person.objects().where(Person.name.like("%Jones%")).run()

For any query features not currently supported, you can drop down to raw SQL:

await Person.raw('my custom query').run()

[sinisaos]

Hi Daniel!
I’m not a professional, but I love trying out new libraries and I really like your project. There were no problems on localhost and everything works really well (migration, admin, etc.) but when try deploy site on Heroku (free account), I had two problems.
The first problem was heroku postgres (hobby / dev) SSL check failed for asyncpg, but I can fix this as sugested on MagicStack/asyncpg#238 (I didn't have such a problem with encode databases or tortoise orm). After that, i can successfully connect to db, start migrations and create an admin user.
Second problem is that I can't log in as admin (403 Forbidden) in piccolo-admin even though sessions and piccolo_user tables are in the database and admin user is set to True.
Is this axios and vue problem or? Any sugestions.

You can view the simple website at https://github.com/sinisaos/starlette-piccolo-orm.

Thanks for a great project!

[dantownsend]

@sinisaos Thanks for trying the project out - I'm really impressed with the app you've built.

I haven't tried it with Heroku yet. When I use SSL with a version of Postgres I've installed myself it seems OK, so I need to investigate further.

With the admin, did you create the user using piccolo user create?

Have you posted your project on the Encode forums? I think other people would be interested to see a complete Starlette app like this. I'll also put a link to it in the Piccolo docs.

[sinisaos]

First of all thank you for liking app, I really appreciate it.

Yes. I used piccolo user create to create the administrator and everything is fine in the database. The user was created successfully, and the admin column was set to True. Checked it in the postgres console.

Feel free to use anything from the github repo if you find something interesting.

Thanks for putting the link in the piccolo docs (it's my honor), maybe it be useful to someone.

[dantownsend]

@sinisaos I ran it locally, and it worked great for me. I just updated the requirements file slightly - have opened a pull request. The admin is a bit vague if login fails. I'll try and make it more helpful.

[sinisaos]

I see that you have cleaned and updated the requirements.txt (I'm just pip freeze my whole virtualenv), but for example I need bcrypt for auth non admin users. Maybe we did not understand each other but like I previously wrote admin works perfectly locally but does't work on Heroku.

[dantownsend]

@sinisaos Ah, I see! My bad. Feel free to ignore that pull request. Time for me to get a Heroku account!

[sinisaos]

@dantownsend No problem and thanks for your time to investigating. Heroku is really good for free account and you can deploy 5 apps.

[sinisaos]

@dantownsend When I have time I will try to deploy piccolo-admin example to Heroku. There may be a conflict between piccolo authentication and my user authentication although it doesn't make sense because picollo-admin is standalone asgi app mount to starlette app.

[dantownsend]

@sinisaos Thanks. It might be that Heroku is stripping or malforming the headers, and CSRF is failing as a result. This can happen when a request gets redirected. I'll have a look.

[sinisaos]

First of all thanks for putting link to app in piccolo docs.
I can confirm that neither piccolo-admin example does't work on Heroku. Same problem.

If you want I can post .har file from chromium or send you as email.

[dantownsend]

@sinisaos Thanks for that. I see the problem now - it's with the CSRF middleware. When serving under HTTPS, the CSRF middleware requires anallowed_hosts argument, which currently isn't exposed by create_admin. I haven't encountered this before, as I tend to deploy it behind Nginx (Nginx terminates the HTTPS, and proxies to the admin as HTTP). I'll work on a fix - it won't take long.

[arunabhdas]

So, here's what's happening.

I wanted to test if the migrations are working correctly when I add a new class

To test this, I added Todo to tables.py in addition to Task and Post as below -

class Todo(Table):
    """
    An example table.
    """

    title = Varchar()
    description = Varchar()
    completed = Boolean(default=False)

My tables.py now looks as below

from piccolo.table import Table
from piccolo.columns import Varchar, Boolean


class Task(Table):
    """
    An example table.
    """

    name = Varchar()
    completed = Boolean(default=False)


class Post(Table):
    """
    An example table.
    """

    title = Varchar()
    description = Varchar()
    completed = Boolean(default=False)


class Todo(Table):
    """
    An example table.
    """

    title = Varchar()
    description = Varchar()
    completed = Boolean(default=False)

I then ran -

piccolo migrations new home --auto

This created home_2023_03_20t05_41_22_594033.py in piccolo_migrations folder as below

from piccolo.apps.migrations.auto.migration_manager import MigrationManager
from piccolo.columns.column_types import Boolean
from piccolo.columns.column_types import Varchar
from piccolo.columns.indexes import IndexMethod


ID = "2023-03-20T05:41:22:594033"
VERSION = "0.109.0"
DESCRIPTION = ""


async def forwards():
    manager = MigrationManager(
        migration_id=ID, app_name="home", description=DESCRIPTION
    )

    manager.add_table("Todo", tablename="todo")

    manager.add_column(
        table_class_name="Todo",
        tablename="todo",
        column_name="title",
        db_column_name="title",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Todo",
        tablename="todo",
        column_name="description",
        db_column_name="description",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Todo",
        tablename="todo",
        column_name="completed",
        db_column_name="completed",
        column_class_name="Boolean",
        column_class=Boolean,
        params={
            "default": False,
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    return manager

However, when I now run

piccolo migrations forwards home

The command fails as below

==> piccolo migrations forwards home
👍 2 migrations already complete
⏩ 2 migrations not yet run
🚀 Running 2 migrations:
  - 2023-03-20T04:57:20:322489 [forwards]... The command failed.
relation "task" already exists
For a full stack trace, use --trace

forwards
========
Runs any migrations which haven't been run yet. 

Usage
-----
forwards app_name [--migration_id=X] [--fake] [--preview]

Args
----
app_name 
The name of the app to migrate. Specify a value of 'all' to run
migrations for all apps.

migration_id [default="all"]
Migrations will be ran up to and including this migration_id.
Specify a value of 'all' to run all of the migrations. Specify a
value of '1' to just run the next migration.

fake [default=false]
If set, will record the migrations as being run without actually
running them.

preview [default=false]
If true, don't actually run the migration, just print the SQL queries

Aliases
-------
f, forward

Please note that I previously had home_2023_03_20t04_57_20_322489.py in my piccolo_migrations folder (prior to adding Todo model), so the piccolo_migrations listing is as follows

├── home_2023_03_20t04_57_20_322489.py
└── home_2023_03_20t05_41_22_594033.py

[dantownsend]

@arunabhdas Something weird has happened. It might be worth just starting from scratch - delete the migration files, recreate the database, and try again.

What's the contents of the home_2023_03_20t04_57_20_322489.py file?

[arunabhdas]

Contents of home_2023_03_20t04_57_20_322489.py file -

from piccolo.apps.migrations.auto.migration_manager import MigrationManager
from piccolo.columns.column_types import Boolean
from piccolo.columns.column_types import Varchar
from piccolo.columns.indexes import IndexMethod


ID = "2023-03-20T04:57:20:322489"
VERSION = "0.109.0"
DESCRIPTION = ""


async def forwards():
    manager = MigrationManager(
        migration_id=ID, app_name="home", description=DESCRIPTION
    )

    manager.add_table("Task", tablename="task")

    manager.add_table("Post", tablename="post")

    manager.add_column(
        table_class_name="Task",
        tablename="task",
        column_name="name",
        db_column_name="name",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Task",
        tablename="task",
        column_name="completed",
        db_column_name="completed",
        column_class_name="Boolean",
        column_class=Boolean,
        params={
            "default": False,
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Post",
        tablename="post",
        column_name="title",
        db_column_name="title",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Post",
        tablename="post",
        column_name="description",
        db_column_name="description",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    manager.add_column(
        table_class_name="Post",
        tablename="post",
        column_name="completed",
        db_column_name="completed",
        column_class_name="Boolean",
        column_class=Boolean,
        params={
            "default": False,
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

    return manager

So it seems to me that the

piccolo migrations new home --auto

command is creating the migration for Task which already exists in the DB.

It should be skipping over the migrations for Task since Task already exists in the DB right?

[dantownsend]

@arunabhdas When Piccolo creates migrations, it doesn't look at what tables are already in the database. It looks at what's in tables.py, and what's in the migration files, and then works out what has changed.

If the tables already exist in the database because they were added manually, you can fake apply the migrations. piccolo migrations forwards home --fake.

[arunabhdas]

A bit confused.

What would be the correct way to start from scratch in a way that doesn't generate the default Task model?

When I initially ran

piccolo asgi new

It generated the Task model in tables.py automatically.

So how can I avoid Task model gettting generated by default?

Also, what should my FastAPIWrapper block in app.py look like?

Currently, it looks like

###############################################################################
# Rather than defining the FastAPI endpoints by hand, we can use
# `FastAPIWrapper`, which can save us a lot of time.

FastAPIWrapper(
    "/posts",
    fastapi_app=app,
    piccolo_crud=PiccoloCRUD(Post, read_only=False),
    fastapi_kwargs=FastAPIKwargs(
        all_routes={"tags": ["Post"]},
    ),
)


###############################################################################

Please note that I plan to add the following models - Post, Note, Idea and others incrementally and would prefer to use FastAPIWrapper, so as not to have to write each CRUD operation manually by hand?

Appreciate your quick response

[dantownsend]

@arunabhdas When you use piccolo asgi new it has an example table called Task in tables.py. If you don't want that table, just delete it from tables.py.

In your case something weird happened with the database where it can't apply the migrations because they already exist in the database for some reason.

I'd do the following if I was you:

• Recreate the database, or just delete any existing tables in there using a tool like pgadmin.
• Delete all your migration files from piccolo_migrations.
• Delete Task from tables.py if you want as you don't need it.
• Create the migrations again using piccolo migrations new home --auto, and apply them using piccolo migrations forwards home.

This tutorial might be helpful.

[arunabhdas]

Thanks @dantownsend

That works when I delete Task from tables.py and re-run the migrations for the new classes.

Maybe you might want to add that to the docs somewhere (just a suggestion)

The FastAPIWrapper is working as well.

Thanks for your prompt help!!

I'll try to go through the docs and come up with ideas for improving it a bit more.

Thanks again for your quick responses and great support @dantownsend !!

[dantownsend]

@arunabhdas No worries, thanks.

[arunabhdas]

@dantownsend : One more question.

Would you happen to have any docs / guidance or video tutorial on how to create one-to-many relationships using Piccolo?

Also, any docs or tutorial on how to have JWT authentication using Piccolo / FastAPIWrapper?

[dantownsend]

@arunabhdas You can do many-to-one using a ForeignKey, or many-to-many using M2M.

For JWT authentication, we have some middleware / endpoints for that.

I don't use JWT much though - I tend to use token auth or plain session auth.

To wrap FastAPIWrapper in auth, I usually have a private app which is wrapped in auth middleware, and mount that to a public app. These docs should give you a pretty good idea.

Create a discussion here if you need more info, because this thread is getting pretty long.

[arunabhdas]

Thanks @dantownsend

Do you mind taking a quick look at my question posted on discussions if possible?

[janusheide]

Even though this request for feedback is a bit old it is still open so it seemed appropriate to add this here, otherwise let me know.

Recently evaluated piccolo for use in an existing application of mine which is currently using FastApi and Tortoise Orm (among other things). I was considering trying out Litestar instead and since they currently only have examples for sqlalchemy and piccolo I looked at your library. In an earlier version I used sqlalchemy, I also revisited that and must say I really (still) dislike how their documentation is organized and that they have so many overlapping API’s, but that is maybe for another forum.

So here is what I found during a quick initial conversion from Tortoise to Piccolo for an existing project, first the good and then stuff I did not like as much. Spoilers I generally really liked it, I can easily see myself using it in the future.

The good

1. Documentation is good, easy to navigate and generally strikes a good balance. It was easy to get started.

2. Coming (primarily) from Tortoise I find the syntax easy to grasp, it is roughly compatible with Tortoise, I guess since both are looking to Django’s orm.

3. I like the trivial mapping of field names to postgres, something Tortoise does not have, but to be fair Tortoise also aims to support more databases.

4. The Email field is a great idea, maybe a similar Path field could also be useful.

5. And to many more things to mention, I liked almost everything, thanks for the nice project :)

Stuff I am missing or did not like

6. I am really missing hooks; post_delete, pre_delete, .... Tortoise has it, sqlalchemy has something that kinda works (which is maybe worse than nothing ;)

7. The piccolo_conf.py and similar the test configuration, this approach seems very implicit and having to export an environmental variable to change that file’s location (or to use the test settings) is something that I would predict would cause me confusion and errors down the road. Alternatively explicit injecting my test db into all of my tables I would start to mix production and testing code, which I really dont want to do. Maybe you could look to use pydantic’s basesettings for something that could be as convenient and less error prone as what you have now.

8. The finder magic here I just ended up manually importing the tables and passing them to create_db_tables(...), again I prefer explicit over implicit. It took me longer to figure out than it should have, so maybe a note there or just generate those examples so they include direct links to the api docs for the used classes/functions.

9. Missing support for sqlite in memory, this is convenient for simple unit tests of the models, yes I can just put the db in a temp file or something, but memory is easier.

10. In one of my tables I unfortunately have a field that is called “count”... that really does not play so nicely with your Table.count() method, so I ended up doing something like the following, which is not great.
    assert (await Image.select(Count()))[0]["count"] == 1

Again thanks for the awesome work, I hope to see more of this project in the future :)

[dantownsend]

@janusheide Thanks a lot for the feedback - I appreciate you taking the time to write it, and it's really helpful.

For point 10, this workaround might help:

class MyTable(Table):
    count_ = Varchar(db_column_name="count")

await MyTable.select(MyTable.count_)

[haaavk]

Hi,
Thank you for creating piccolo. I was waiting for async library like that for years.
The overall experiense is really good.
Some minor suggestions:

• Password minimum length could be stated in error message.
  In documentation there is only maximum listed. I ended up looking this info in source code.
  Maybe something like
  raise ValueError(f"The password is too short. (min {cls._min_password_length}")
  or
  raise ValueError(f"The password should be at least {cls._min_password_length} chars long.")
• In documentation update_password takes username as argument await BaseUser.update_password(username="bob", password="abc123").
  In code it take user argument. async def update_password(cls, user: t.Union[str, int], password: str):

[dantownsend]

@haaavk Thanks a lot for the feedback - those are all great suggestions!