Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: fix share reminder job for oracle #48182

Merged
merged 1 commit into from
Sep 18, 2024
Merged

fix: fix share reminder job for oracle #48182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/files_sharing/lib/SharesReminderJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ private function getShares(): array|\Iterator {
$qb->expr()->eq('s.share_type', $qb->expr()->literal(IShare::TYPE_EMAIL))
),
$qb->expr()->eq('s.item_type', $qb->expr()->literal('folder')),
$qb->expr()->gte('s.expiration', $qb->createNamedParameter($minDate->format('Y-m-d H:i:s'))),
$qb->expr()->lt('s.expiration', $qb->createNamedParameter($maxDate->format('Y-m-d H:i:s'))),
$qb->expr()->gte('s.expiration', $qb->createNamedParameter($minDate, IQueryBuilder::PARAM_DATE)),
$qb->expr()->lte('s.expiration', $qb->createNamedParameter($maxDate, IQueryBuilder::PARAM_DATE)),
$qb->expr()->eq('s.reminder_sent', $qb->createNamedParameter(
false, IQueryBuilder::PARAM_BOOL
)),
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Files/Cache/SearchBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class SearchBuilder {
'owner' => 'string',
];

/** @var array<string, int> */
/** @var array<string, int|string> */
protected static $paramTypeMap = [
'string' => IQueryBuilder::PARAM_STR,
'integer' => IQueryBuilder::PARAM_INT,
Expand Down
3 changes: 2 additions & 1 deletion lib/public/DB/QueryBuilder/IQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Doctrine\DBAL\ArrayParameterType;
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Types\Types;
use OCP\DB\Exception;
use OCP\DB\IResult;
use OCP\IDBConnection;
Expand All @@ -28,7 +29,7 @@ interface IQueryBuilder {
/**
* @since 9.0.0
*/
public const PARAM_BOOL = ParameterType::BOOLEAN;
public const PARAM_BOOL = Types::BOOLEAN;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a serious bug in doctrine which we should report and get addressed upstream?
The binding type should be ParameterType::
https://github.com/nextcloud/3rdparty/blob/9ca60e9d2b12f711a756c35d37fd19c178db685b/doctrine/dbal/src/Types/BooleanType.php#L56-L59
Doc block of ParameterType also states that it is for statement parameters:
https://github.com/nextcloud/3rdparty/blob/a9be8d0755e6d786c34e44ef29e88d8f9b90c65b/doctrine/dbal/src/ParameterType.php#L6

The Types are the dbal types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try if bumping doctrine to 3.9.1 fixes it #48306

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, still fails with 3.9.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I've debugged the weirdness with OCI and booleans a while ago: #45630

The type constants from doctrine/dbal allows us to use doctrine type conversion logic. I was unsure back then about changing it because apps, especially on OCI, may depend on the broken behavior.

Should we also update the other constants to use the doctrine types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also update the other constants to use the doctrine types?

As said I think the switch that was done here is actually wrong. It uses the DBAL architectural type now and not the PDO parameter type anymore.

Copy link
Contributor

@kesselb kesselb Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said I think the switch that was done here is actually wrong. It uses the DBAL architectural type now and not the PDO parameter type anymore.

Depends ;)

Using the DBAL type leads to doctrine taking care of the type juggeling for us. That's probably good, but also a risky change. We don't know if the results are still the same.

We are not using the pdo driver for OCI. I think that's the reason for the weird behavior with OCI, especially with booleans. The OCI driver just didn't know what to do with type "5" (because that's the pdo constant) and fallbacks to string as default. If we use the dbal type doctrine will convert the boolean to 0/1 and set type int.

I've discussed #45630 with Christoph a while ago, and we didn't know how to move forward 🤷

We updated doctrine since then, not sure if my observations are still correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we already use the DBAL types for datetime and json

/**
* @since 9.0.0
*/
Expand Down
Loading