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

[stable21] Fix Oracle query limit compliance in Comments #27203

Merged
merged 3 commits into from
Sep 27, 2021
Merged

[stable21] Fix Oracle query limit compliance in Comments #27203

merged 3 commits into from
Sep 27, 2021

Conversation

backportbot-nextcloud[bot]
Copy link

backport of #27187

@backportbot-nextcloud backportbot-nextcloud bot added this to the Nextcloud 21.0.3 milestone May 28, 2021
@skjnldsv skjnldsv added the 4. to release Ready to be released and/or waiting for tests to finish label May 28, 2021
nickvergessen
nickvergessen reviewed May 31, 2021
View reviewed changes
lib/private/Comments/Manager.php Outdated
$query->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
$result = $query->execute();

$unreadComments += array_fill_keys($objectIds, 0);
Copy link
Member

Choose a reason for hiding this comment

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

since this uses $objectIds instead of $chunk it would have been enough to do it once before the loop even starts?

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Do you want me to make a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

We can fix it here and do a new PR for master only

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let me know if I can do anything.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed it, master PR in #27316

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jun 7, 2021
@blizzz blizzz mentioned this pull request Jun 23, 2021
Merged
9 tasks
@blizzz blizzz force-pushed the backport/27187/stable21 branch from 128df0c to 736898e Compare June 23, 2021 23:42
@magikmw magikmw mentioned this pull request Jun 24, 2021
Closed
@blizzz blizzz mentioned this pull request Jul 1, 2021
Merged
10 tasks
@blizzz
Copy link
Member

blizzz commented Jul 1, 2021

CI is still unhappy

@ChristophWurst ChristophWurst mentioned this pull request Jul 5, 2021
Closed
@ghost
Copy link

ghost commented Jul 6, 2021

Looks like this has been fixed in NC22 but still an issue on 21.0.3

@skjnldsv skjnldsv mentioned this pull request Jul 26, 2021
Merged
14 tasks
@skjnldsv
Copy link
Member

@Simounet what is the status here?

@Simounet
Copy link
Member

@skjnldsv I would love to help you here. I didn't know that you were waiting for me.

@skjnldsv skjnldsv mentioned this pull request Aug 3, 2021
Merged
12 tasks
@skjnldsv skjnldsv removed this from the Nextcloud 21.0.4 milestone Aug 5, 2021
@skjnldsv skjnldsv added this to the Nextcloud 21.0.5 milestone Aug 5, 2021
@fblz
Copy link

fblz commented Aug 22, 2021

Still an issue in 21.0.4

@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

/rebase

@szaimen szaimen self-assigned this Aug 31, 2021
artonge
artonge requested changes Sep 15, 2021
View reviewed changes
lib/private/Comments/Manager.php Outdated Show resolved Hide resolved
@szaimen szaimen removed their assignment Sep 15, 2021
@blizzz blizzz mentioned this pull request Sep 23, 2021
Merged
8 tasks
@blizzz
Copy link
Member

blizzz commented Sep 23, 2021

CI unhappy

@szaimen
Copy link
Contributor

szaimen commented Sep 23, 2021

/rebase

Simounet and others added 2 commits September 23, 2021 08:37
@Simounet @szaimen
Fix Oracle query limit compliance in Comments
f325a4a 
Signed-off-by: Simounet <contact@simounet.net>
@nickvergessen @szaimen
Fix populating the array and closing the cursors
8e7eea9 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@szaimen
Copy link
Contributor

szaimen commented Sep 23, 2021

@artonge do you mind fixing dco for your commit? I think CI should otherwise be green now :)

@artonge
Use legacy execute method
1297b04 
Use legacy execute method instead of executeQuery introduced in 22

Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the backport/27187/stable21 branch from 81bdc4d to 1297b04 Compare September 23, 2021 10:45
@szaimen
Copy link
Contributor

szaimen commented Sep 23, 2021 •
edited
Loading

@artonge thanks! :)

@szaimen
Copy link
Contributor

szaimen commented Sep 23, 2021 •
edited
Loading

failure is unrelated as far as I can see

@blizzz do you think that merging this now (after the RC) is feasible?

@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Sep 26, 2021
@blizzz
Copy link
Member

blizzz commented Sep 27, 2021

@blizzz do you think that merging this now (after the RC) is feasible?

Case by case. Since the original change was already shipped with 22 and because it is not a complex change, I think it's ok.

@blizzz blizzz merged commit 62a5d27 into stable21 Sep 27, 2021
@blizzz blizzz deleted the backport/27187/stable21 branch September 27, 2021 09:02
@blizzz blizzz mentioned this pull request Sep 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@nickvergessen nickvergessen nickvergessen left review comments

@artonge artonge artonge requested changes

@skjnldsv skjnldsv skjnldsv approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@PVince81 PVince81 Awaiting requested review from PVince81

@Simounet Simounet Awaiting requested review from Simounet

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Milestone
Nextcloud 21.0.5
Development

Successfully merging this pull request may close these issues.
8 participants
@blizzz @skjnldsv @Simounet @fblz @szaimen @nickvergessen @MorrisJobke @artonge