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 reading blob data as resource #33129

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Fix reading blob data as resource #33129

merged 1 commit into from
Jul 25, 2022

Conversation

tcitworld
Copy link
Member

PostgreSQL returns data as resource when using IQueryBuilder::PARAM_LOB (which is used for QBMapper).

Previously we just converted this resource using settype, which produced things like "Resource id #14" instead of the actual resource data.

Now we read the stream correctly if the returned data is a resource

See context at #22472

Fixes #22439

@tcitworld tcitworld added bug 3. to review Waiting for reviews php Pull requests that update Php code labels Jul 6, 2022
@tcitworld tcitworld added this to the Nextcloud 25 milestone Jul 6, 2022
@tcitworld tcitworld added this to In progress in Thomas things to do via automation Jul 6, 2022
@tcitworld tcitworld requested review from PVince81 and come-nc and removed request for a team July 6, 2022 07:58
@tcitworld
Copy link
Member Author

/backport to stable24

@tcitworld
Copy link
Member Author

/backport to stable23

@tcitworld
Copy link
Member Author

/backport to stable22

@tcitworld
Copy link
Member Author

@szaimen If you can try this patch

@tcitworld tcitworld moved this from In progress to Pending review in Thomas things to do Jul 6, 2022
@nickvergessen
Copy link
Member

Can we get a test added?

@tcitworld tcitworld force-pushed the fix-reading-blob-as-resources branch 2 times, most recently from fdab572 to be520d7 Compare July 6, 2022 13:03
@tcitworld
Copy link
Member Author

There you go

nickvergessen
nickvergessen approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I actually meant a real test reading stuff from the database, but fine by me for now

Thomas things to do automation moved this from Pending review to Reviewer approved Jul 6, 2022
@tcitworld
Copy link
Member Author

Where would I create such a functional test? Still in AppFramework\DB?

@nickvergessen
Copy link
Member

Yes, tests/lib/AppFramework/DB/EntityTest.php
But since there are no tests yet, I guess it's quite some overhead

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 8, 2022
@blizzz
Copy link
Member

blizzz commented Jul 8, 2022

Linter is unhappy

@tcitworld tcitworld force-pushed the fix-reading-blob-as-resources branch from be520d7 to 89c543f Compare July 11, 2022 08:35
@tcitworld
Copy link
Member Author

Made it happy

@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
≠ /remote.php/dav/files/test/test.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
≠ /remote.php/dav/files/test/many_files with 1 queries removed
  - UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
= /remote.php/dav/files/test/new_file.txt
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)

@blizzz
Copy link
Member

blizzz commented Jul 22, 2022

/compile amend /

@tcitworld
Copy link
Member Author

Why the compile amend? :(

@tcitworld @nickvergessen
Fix reading blob data as resource
1d30fb7 
PostgreSQL returns data as resource when using IQueryBuilder::PARAM_LOB
(which is used for QBMapper).

Previously we just converted this resource using settype, which produced
things like "Resource id #14" instead of the actual resource data.

Now we read the stream correctly if the returned data is a resource

See context at #22472

Fixes #22439

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@nickvergessen
Copy link
Member

It was just the wrong command, I guess he wanted to rebase because that is what github complained about.

@blizzz
Copy link
Member

blizzz commented Jul 25, 2022 •
edited
Loading

yeah, sorry, was rushing through review requests

@blizzz blizzz merged commit bbe15b4 into master Jul 25, 2022
Thomas things to do automation moved this from Reviewer approved to Done Jul 25, 2022
@blizzz blizzz deleted the fix-reading-blob-as-resources branch July 25, 2022 15:56
This was referenced Jul 25, 2022
Merged
Merged
Closed
@skjnldsv skjnldsv mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@come-nc come-nc come-nc approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@PVince81 PVince81 Awaiting requested review from PVince81

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug php Pull requests that update Php code
Projects
No open projects
Thomas things to do
  
Done
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.

Contacts sync broken since NC 19upgrade
5 participants
@tcitworld @nickvergessen @blizzz @ChristophWurst @come-nc