-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add AppFramework/Db base classes #17486
Conversation
oooo the irony... this now fails because I made the classes strict. |
What's missing? |
rebase and passing tests I think ;) |
Let's do this then :) |
cdba08b
to
ff54ecc
Compare
472989f
to
c2a8624
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine otherwise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
873e56a
to
001c624
Compare
Does that still make sense? Having tables without a primary key. Percona already has some issues with those tables and also MySQL is adding a flag to enforce primary keys: #16311 (comment). It seems to be required for row based replication. 👍 for the change in general (it's much cleaner now) but we should probably enforce a primary key. |
It's not about having no primary key. But allowing other types or a composite key. |
Or no primary key. A id column was enforced before. I would prefer a getPrimaryKey method to be overwritten by subclasses and a default update method using getPrimaryKey. Laravel is using a similar approach: https://github.com/laravel/framework/blob/7efc5377ada189e707855b9e17e6159a1d350073/src/Illuminate/Database/Eloquent/Model.php#L45-L50 |
That should be doable. Like simple method that returns an array.
Should work as well if we know the types of the columns.
I remember fighting with composite keys and Laravel's Eloquent. They only allow a single key, right? All you can do is move it to another column. |
So, how to continue? We kind of need this in talk for the sessions table (nextcloud/spreed#2020). There is a unique string column "session" var(255), so there is no need for a second column to be unique |
I would suggest to rename session to id, disable autoincrement, and use session as value. That should be possible without any change in server. |
There is code assuming id is int, so not without any changes at least. But other places can be overwritten. So we just fix that one thing and carry on? |
😖 |
So something like #19659 should solve this? |
Hmm. That might work. It was probably a bad suggestion to enforce a primary key code wise. It's still possible to use the query builder then. We might document (don't use tables without primary keys to make sure also row based replication works well) somewhere but don't enforce it. |
That has nothing to do with the entities however. We want to move more things over to those entities, instead of having the same customer stuff everywhere, because there are artificial limitations in the abstract base class. |
All right. Another rebase and let's get this in 🎉 |
This will allow apps to use Entities that do not have an id attribute. They can have other unique columns forcing an id is not ideal in all cases. For this two base classes are introduced and the current Entity and QBMapper are actually based on the base classes. The Entity is really simple and just adds the id property. The BaseMapper is a bit less clean. As we can't know the where clause for insert/delete or update parts. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
001c624
to
c44511b
Compare
You misunderstood.... I think we should "just" merge #19659 |
And this PR :) |
so going over this again... what is the verdict. Composite keys? Or will that do boom. |
Therewasa topic/issue somewhere with a couple of galera cluster people which apparently can not handle tables without a primary key. Let me find it and see if we can ask the experts |
Can't find it anymore |
Sounds like #16311 |
This will allow apps to use Entities that do not have an id attribute.
They can have other unique columns forcing an id is not ideal in all
cases.
For this two base classes are introduced and the current Entity and
QBMapper are actually based on the base classes.
The Entity is really simple and just adds the id property.
The BaseMapper is a bit less clean. As we can't know the where clause
for insert/delete or update parts.
Signed-off-by: Roeland Jago Douma roeland@famdouma.nl