Skip to content

Fix reading blob data as resource#33129

Merged
blizzz merged 1 commit intomasterfrom
fix-reading-blob-as-resources
Jul 25, 2022
Merged

Fix reading blob data as resource#33129
blizzz merged 1 commit intomasterfrom
fix-reading-blob-as-resources

Conversation

@tcitworld
Copy link
Member

@tcitworld tcitworld commented Jul 6, 2022

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 requested review from a team, ChristophWurst, PVince81, blizzz, come-nc and nickvergessen and removed request for a team July 6, 2022 07:58
@tcitworld
Copy link
Member Author

tcitworld commented Jul 6, 2022

/backport to stable24

@tcitworld
Copy link
Member Author

tcitworld commented Jul 6, 2022

/backport to stable23

@tcitworld
Copy link
Member Author

tcitworld commented Jul 6, 2022

/backport to stable22

@tcitworld
Copy link
Member Author

tcitworld commented Jul 6, 2022

@szaimen If you can try this patch

@nickvergessen
Copy link
Member

nickvergessen commented Jul 6, 2022

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

tcitworld commented Jul 6, 2022

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

@tcitworld
Copy link
Member Author

tcitworld commented Jul 7, 2022

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

@nickvergessen
Copy link
Member

nickvergessen commented Jul 7, 2022

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

tcitworld commented Jul 11, 2022

Made it happy

@github-actions
Copy link
Contributor

github-actions bot commented Jul 11, 2022

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 /

@nextcloud-command nextcloud-command force-pushed the fix-reading-blob-as-resources branch from 89c543f to 6f65b50 Compare July 22, 2022 21:24
@tcitworld
Copy link
Member Author

tcitworld commented Jul 23, 2022

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 nickvergessen force-pushed the fix-reading-blob-as-resources branch from 6f65b50 to 1d30fb7 Compare July 25, 2022 07:46
@nickvergessen
Copy link
Member

nickvergessen commented Jul 25, 2022

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
@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

None yet

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