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 Oracle query limit compliance #1804

Merged
merged 1 commit into from
Dec 21, 2021
Merged

fix Oracle query limit compliance #1804

merged 1 commit into from
Dec 21, 2021

Conversation

to-kn
Copy link
Contributor

@to-kn to-kn commented Dec 7, 2021

these changes fixed the Problems with #1702 for me

fschrempf
fschrempf requested changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@fschrempf fschrempf left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I think it would be better to add the chunking directly in getFoldersForGroups() instead of implementing it in multiple places. And a comment with the reason for the chunking would be nice.

@fschrempf fschrempf mentioned this pull request Dec 7, 2021
Closed
@fschrempf
Copy link
Contributor

You can have a look at nextcloud/server#27187 or nextcloud/mail#4947 for other places where this was fixed. I also opened an issue (#1805) to check any other queries that might hit the limitations.

@to-kn to-kn requested a review from fschrempf December 17, 2021 11:38
@to-kn
Copy link
Contributor Author

to-kn commented Dec 17, 2021

HI @fschrempf do you mean like that? To be honest i did not had the possibility to validate the fix like that...

fschrempf
fschrempf requested changes Dec 19, 2021
View reviewed changes
Copy link
Contributor

@fschrempf fschrempf left a comment

Choose a reason for hiding this comment

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

The code looks good to me. If you could add the comment I was asking for and squash the two commits into a single one, I think we are good to go.

@fschrempf
Copy link
Contributor

And we nee to find out why the unit tests fail. Might not be related to this PR, I haven't checked. Maybe rebasing already does the trick.

@to-kn to-kn requested a review from fschrempf December 20, 2021 07:27
@to-kn
Copy link
Contributor Author

to-kn commented Dec 20, 2021

@fschrempf should be good

CarlSchwan
CarlSchwan approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Code looks fine, let's wait for the tests results

lib/Folder/FolderManager.php Outdated Show resolved Hide resolved
lib/Folder/FolderManager.php Outdated Show resolved Hide resolved
CarlSchwan
CarlSchwan requested changes Dec 20, 2021
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Tests are still failing and it seems to be directly related to this PR

@to-kn
Copy link
Contributor Author

to-kn commented Dec 20, 2021

@CarlSchwan thank you for your feed-back. I did not manage to get the Tests running locally so i was not able to check it beforehand...

@to-kn to-kn requested a review from CarlSchwan December 20, 2021 09:06
@to-kn
fix Oracle query limit compliance
57cf151 
Signed-off-by: Tobias Knipping <[email protected]>
@to-kn
Copy link
Contributor Author

to-kn commented Dec 20, 2021

@CarlSchwan since tests are running now, i sqashed the commits again. Let me know if anything is missing

CarlSchwan
CarlSchwan approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

LGTM

@CarlSchwan CarlSchwan merged commit 694e1b3 into nextcloud:master Dec 21, 2021
@CarlSchwan
Copy link
Member

/backport to stable23

@CarlSchwan
Copy link
Member

/backport to stable22

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 21, 2021
Merged
@CarlSchwan
Copy link
Member

/backport to stable21

This was referenced Dec 21, 2021
Merged
Merged
@solracsf solracsf linked an issue Dec 30, 2021 that may be closed by this pull request
NC22 Doctrine\DBAL\Query\QueryException: More than 1000 expressions in a list are not allowed on Oracle #1702
Closed
@solracsf solracsf mentioned this pull request Dec 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fschrempf fschrempf fschrempf approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees
No one assigned
Labels
backport-request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NC22 Doctrine\DBAL\Query\QueryException: More than 1000 expressions in a list are not allowed on Oracle
3 participants
@to-kn @fschrempf @CarlSchwan