piccolo icon indicating copy to clipboard operation
piccolo copied to clipboard

Feedback needed

Open dantownsend opened this issue 5 years ago • 49 comments

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.

dantownsend avatar May 24 '20 21:05 dantownsend

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 ;)

gazorby avatar Jun 23 '20 00:06 gazorby

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.

dantownsend avatar Jun 23 '20 07:06 dantownsend

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?

gazorby avatar Jun 23 '20 09:06 gazorby

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()

dantownsend avatar Jun 23 '20 10:06 dantownsend

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 https://github.com/MagicStack/asyncpg/issues/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!

sinisaos avatar Oct 02 '20 07:10 sinisaos

@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.

dantownsend avatar Oct 02 '20 11:10 dantownsend

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.

sinisaos avatar Oct 02 '20 12:10 sinisaos

@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.

dantownsend avatar Oct 02 '20 15:10 dantownsend

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.

sinisaos avatar Oct 02 '20 16:10 sinisaos

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

dantownsend avatar Oct 02 '20 16:10 dantownsend

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

sinisaos avatar Oct 02 '20 16:10 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.

sinisaos avatar Oct 02 '20 16:10 sinisaos

@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.

dantownsend avatar Oct 02 '20 17:10 dantownsend

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.

console

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

sinisaos avatar Oct 03 '20 05:10 sinisaos

@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.

dantownsend avatar Oct 03 '20 11:10 dantownsend

@dantownsend Great. I will try and confirm if it works or not.

sinisaos avatar Oct 03 '20 12:10 sinisaos

@sinisaos I've released a new version of Piccolo admin - version 0.10.1.

If you add an allowed_hosts argument to create_admin then hopefully it should work!

app.mount(
    "/admin/",
    create_admin(tables=[Category, Question, Answer], allowed_hosts=['starlette-piccolo.herokuapp.com']),
)

dantownsend avatar Oct 03 '20 14:10 dantownsend

@dantownsend Wow! You found the bug really fast and fixed it. Everything works now. Thanks

Screenshot from 2020-10-03 16-42-34

sinisaos avatar Oct 03 '20 14:10 sinisaos

@sinisaos Cool, glad it's working. Let me know if you encounter any other issues, or have any ideas for improvements.

dantownsend avatar Oct 04 '20 06:10 dantownsend

@dantownsend Thanks again for a great project. I really enjoyed our conversation. The best thing about piccolo orm is the simplicity and ecosystem. User can make a fully functional app very quickly and that is biggest strength of piccolo. I'l try to write more apps and if I encounter any problem i'l posted here. For improvements maybe aggregation but why bodther because every thing is easely can write in raw sql.

sinisaos avatar Oct 04 '20 08:10 sinisaos

Hi Daniel, I made new piccolo example app and if you like it you can put in piccolo_examples repo. Maybe it be useful to someone. https://github.com/sinisaos/starlette-piccolo-rental

sinisaos avatar Oct 08 '20 12:10 sinisaos

@sinisaos Great work! I cloned it and got it working.

I made a couple of small changes:

  • In piccolo_conf.py I added 'ads.piccolo_app' so the migrations would run for it.
  • I had to comment out pkg-resources from requirements.txt as I think it's Ubuntu/Debian specific - see this link.

Otherwise, very cool. I'll add a link in the piccolo_examples repo.

dantownsend avatar Oct 08 '20 14:10 dantownsend

Thank you very much.

  • I probably accidentally deleted 'ads.piccolo_app' from piccolo_conf.py because I was able to run migrations.

  • I know about pkg-resources because Heroku always complains that he doesn't recognize the package.

I will fix this, add README.md and try deploy to Heroku.

sinisaos avatar Oct 08 '20 15:10 sinisaos

`class Patients(Controller):

@post("/patients")
async def patients(self, data: FromForm[Login]) -> Response:
    data = data.value
    if (Doctors.select().where(Doctors.login == data.login).run()) and (Doctors.select().where(Doctors.password == gen_hash(data.password)).run()):
        patients = Patients.select().run()
        response = self.view("patients" ,{"patients": patients})
        response.set_cookie(Cookie(str(AUTHORIZED), str(datetime.now())))
        return response
       
    return "{'message':'Неверный логин или пароль'}" `

Traceback (most recent call last): File "blacksheep\baseapp.pyx", line 78, in blacksheep.baseapp.BaseApplication.handle File "c:\program files\python38\lib\site-packages\blacksheep\middlewares.py", line 6, in middleware_wrapper return await handler(request, next_handler) File "c:\program files\python38\lib\site-packages\blacksheep\server\authentication.py", line 23, in authentication_middleware return await handler(request) File "c:\program files\python38\lib\site-packages\blacksheep\server\normalization.py", line 439, in handler response = await method(*values) File "C:\Users\1\Desktop\Avicena.\app\controllers\home.py", line 26, in patients patients = Patients.select().run() AttributeError: type object 'Patients' has no attribute 'select'

NewDay1313 avatar Jun 04 '21 12:06 NewDay1313

@NewDay1313 Looks like there's a table called Patients but also a controller called Patients - looks like the controller is being called when you want the table instead.

You could do from my_tables import Patients as PatientsTable and call await PatientsTable.select().run() instead.

dantownsend avatar Jun 04 '21 12:06 dantownsend

@NewDay1313 Looks like there's a table called Patients but also a controller called Patients - looks like the controller is being called when you want the table instead.

You could do from my_tables import Patients as PatientsTable and call await PatientsTable.select().run() instead.

Thank you :)

NewDay1313 avatar Jun 05 '21 14:06 NewDay1313

Hi Daniel, thanks for the work on Piccolo, looks like the most flexible python ORM :)

Just been through the initial setup. The things I liked:

  1. Simple setup.
  2. Easy to reproduce.

Things I think could be improved:

  1. Not immediately obvious that after installation I should go to Project/Apps section to kick off the integration.
  2. For nested apps, e.g. modules/my_app/repositories it's not immediately obvious that AppRegistry requires package names e.g. modules.my_app.repositories rather than app names. App name has to have slashes because it seems to be the only way to set up a nested app.
  3. Would be great to add github link to the docs website 😎
  4. UPD: running raw migrations backwards is quite confusing based on docs. At the moment working option is to use add_raw_backwards method in the Manager which is created inside forwards function - which is not documented.

liqwid avatar Aug 26 '22 14:08 liqwid

@liqwid Some great points there - thanks! I think they're all valid, and should be implemented.

dantownsend avatar Aug 26 '22 17:08 dantownsend

Hi! I've not used the Piccolo ORM yet but was thinking of using it and the admin for one of our projects. I would like to know if there are any query optimizations performed in the background especially for more complex queries (inner joins between tables, group by and such). Thanks!

cristina0botez avatar Oct 21 '22 14:10 cristina0botez

@cristina0botez The admin mostly does fairly simple queries - basic selects, inserts, updates and deletions.

We test the admin with 10,000s rows, and the performance is good. In particular, we did a lot of work around the foreign key selector (imagine you need to select a user, and there's thousands of users, some admin GUIs will load them all in a HTML select widget, and crash the browser).

One known performance bottleneck, is we use LIMIT OFFSET for pagination, which isn't optimal for millions of rows, but is still acceptable in most cases. We investigated using cursor based pagination, but it added a lot of complexity to the codebase, so haven't adopted it yet.

I hope that helps.

dantownsend avatar Oct 21 '22 17:10 dantownsend