Add AppFramework/Db base classes

[rullzer]

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

[rullzer]

oooo the irony... this now fails because I made the classes strict.
But the setLastCheck is typehinted with an int. but we didn't convert the value yet.
Time to extract this as well

[ChristophWurst]

2. developing

What's missing?

[rullzer]

rebase and passing tests I think ;)

[ChristophWurst]

Let's do this then :)

[ChristophWurst]

Looks fine otherwise!

[juliusknorr]

Looks good 👍

[kesselb]

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.

[ChristophWurst]

It's not about having no primary key. But allowing other types or a composite key.

[kesselb]

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

[ChristophWurst]

Or no primary key. A id column was enforced before. I would prefer a getPrimaryKey method to be overwritten by subclasses

That should be doable. Like simple method that returns an array.

a default update method using getPrimaryKey

Should work as well if we know the types of the columns.

Laravel is using a similar approach

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.

[nickvergessen]

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

[kesselb]

I would suggest to rename session to id, disable autoincrement, and use session as value. That should be possible without any change in server.

[nickvergessen]

server/lib/public/AppFramework/Db/QBMapper.php

Line 195 in 5bf3d1b

$qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))

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?

[kesselb]

😖

[nickvergessen]

So something like #19659 should solve this?

[kesselb]

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.

[nickvergessen]

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.

[kesselb]

All right. Another rebase and let's get this in 🎉

[nickvergessen]

You misunderstood.... I think we should "just" merge #19659

[ChristophWurst]

You misunderstood.... I think we should "just" merge #19659

And this PR :)

[rullzer]

so going over this again...

what is the verdict. Composite keys? Or will that do boom.

[nickvergessen]

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

[nickvergessen]

Can't find it anymore

[kesselb]

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

Sounds like #16311