QBMapper id doesn't have to be an int

[rullzer]

Found when looking at nextcloud/mail#1969

Signed-off-by: Roeland Jago Douma [email protected]

[kesselb]

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

Lines 123 to 125 in 5aeb8ea

	if($entity->id === null) {
	$entity->setId((int)$qb->getLastInsertId());
	}

If someone uses a string as id the value is already set before insert. So id is not null and getLastInsertId() is not called.

[rullzer]

Ah mmm right... meh. let me think some moreabout it then.

[kesselb]

Sorry 🙈 Let me rephrase it: I don't think we have to care about the case because its unlikely. If someone use a hash as id the hash should be set before the insert. We could throw a exception if the type is not integer and the id is null.

[ChristophWurst]

This was also my consideration. The only problem with this is the default type. It should be a new optional parameter with string as default and overwritten in this case

[ChristophWurst]

oh, id has a default type of integer set. so this should actually work then

[ChristophWurst]

I don't think we have to care about the case because its unlikely. If someone use a hash as id the hash should be set before the insert. We could throw a exception if the type is not integer and the id is null.

See mail. This happens when you try to run an update on an existing row. Nextcloud will just take the id as int, even if it's a string. Then the generated query results in a SQL error.

inserts actually work as for every column the custom type is respected.

[kesselb]

Sorry for the confusion 🙈

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

Lines 123 to 125 in 5aeb8ea

	if($entity->id === null) {
	$entity->setId((int)$qb->getLastInsertId());
	}

For entities without id the lastInsertId is set as id. A entity with /** @var string $id **/ and no value will 💥 (if the setId method is properly typed of course).

I don't think we have to care about the case because its unlikely. If someone use a hash as id the hash should be set before the insert. We could throw a exception if the type is not integer and the id is null.

I was wondering if we need to care about this edge case. If someone complains we can still fix it later. I think for the mail app you usually try to insert entities with ids.

[ChristophWurst]

Yep, lastInsertId is indeed a problem the way it's used here.

I was wondering if we need to care about this edge case. If someone complains we can still fix it later. I think for the mail app you usually try to insert entities with ids.

So the problem is actually inserting a row without an id value set and not having an auto increment id column. You can also have an int id without auto increment (for something where the id is provided and not auto assigned) and it will do 💥

[kesselb]

So the problem is actually inserting a row without an id value set and not having an auto increment id column. You can also have an int id without auto increment (for something where the id is provided and not auto assigned) and it will do boom

You may interested in this former discussion: #13086

[rullzer]

Ok maybe this isn't the most elegant way... let me consider a different approach later this evening.