store unencrypted size in the unencrypted_size column #31966

icewind1991 · 2022-04-13T14:09:29Z

The column is right there

Store the unencrypted size in the unencrypted_size column instead of the size column, fixes some issues where other code (object storage) stores the encrypted size in the cache.

icewind1991 · 2022-04-22T12:21:05Z

To test:

setup instance with s3 primary storage
enable encryption
upload some files, verify that the listed size is correct
verify that the files can be downloaded a few times and the size stays correct
keep an eye out for things exploding :)

lib/public/Files/Cache/ICacheEntry.php

CarlSchwan

Code looks 👍

Just some minor comments that can be ignored

lib/public/Files/Cache/ICacheEntry.php

lib/private/Files/Storage/Wrapper/Encryption.php

PVince81 · 2022-04-25T12:43:10Z

as discussed in chat:

check if/how unencrypted_size must be propagated like size, which means it will exist on folders and might affect the logic that detects "-1" sizes in some scenarios

PVince81 · 2022-05-10T13:46:20Z

lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php

 		$x = $this->helper->quoteColumnName($x);
 		$y = $this->helper->quoteColumnName($y);
-		return $this->expressionBuilder->comparison($x, $operator, $y);
+		return new QueryFunction($this->expressionBuilder->comparison($x, $operator, $y));


is this a public API change?

would need to be mentionned in #32117

@icewind1991 can you clarify ?

In guess so strictly speaking, but the result here only really gets used by other query builder parts so no app should care about the actual type

I applied this PR to my (somewhat customized) Docker image that I am developing. It worked just fine a few days ago, but when I ran it just now, it threw the following error upon trying to access the root page:

{ "reqId": "EIZn0TU6px16gqETseAb", "level": 3, "time": "2022-05-30T00:09:52+00:00", "remoteAddr": "192.168.0.202", "user": "--", "app": "PHP", "method": "GET", "url": "/", "message": "Declaration of OC\\DB\\QueryBuilder\\ExpressionBuilder\\ExpressionBuilder::comparison($x, string $operator, $y, $type = null): OCP\\DB\\QueryBuilder\\IQueryFunction must be compatible with OCP\\DB\\QueryBuilder\\IExpressionBuilder::comparison($x, string $operator, $y, $type = null): string at /var/www/html/lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php#119", "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/97.0.4692.71 Safari/537.36", "version": "24.0.1.1" }

I'm fairly adept at debugging Nextcloud and know my way around the code, but this particular PR is quite involved, so I'm not sure where to start to provide more information. Feel free to ask, though - I'm happy to help!

lib/private/Files/Cache/Cache.php

lib/private/Files/Storage/Wrapper/Encryption.php

PVince81 · 2022-05-23T17:40:39Z

@icewind1991 some conflicts

this allows passing the expressions to further expressions without them being escaped as column names Signed-off-by: Robin Appelman <robin@icewind.nl>

Signed-off-by: Robin Appelman <robin@icewind.nl>

PVince81

👍

DaphneMuller · 2022-06-13T09:38:03Z

@CarlSchwan could you please review this?

DaphneMuller · 2022-06-13T09:38:47Z

@szaimen could you please review?

PVince81 · 2022-06-13T09:56:08Z

oh, so the backport got merged before master: #32708

CarlSchwan · 2022-06-13T10:55:53Z

This breaks circle

Return value of OCA\Circles\Tools\Db\ExtendedQueryBuilder::exprLimit() must be of the type string, object returned
Fichier : /var/www/html/apps-extra/circles/lib/Tools/Db/ExtendedQueryBuilder.php

PVince81 · 2022-06-13T10:56:59Z

@icewind1991 can you communicate the API change somehow and put it in #32117

CarlSchwan · 2022-06-13T11:01:20Z

nextcloud/circles#1052

ChristophWurst · 2022-06-13T17:04:06Z

This also breaks the Mail app
TypeError: OCA\Mail\Db\MailboxMapper::eqOrNull(): Return value must be of type string, OC\DB\QueryBuilder\QueryFunction returned at https://github.com/nextcloud/mail/blob/f1f4a287ce3b2074759ef959f42af1639ab4daa6/lib/Db/MailboxMapper.php#L214-L226

So yeah, this is a breaking change.

ChristophWurst · 2022-06-13T17:14:02Z

lib/public/DB/QueryBuilder/IExpressionBuilder.php

 	 * @since 8.2.0 - Parameter $type was added in 9.0.0
 	 *
 	 * @psalm-taint-sink sql $x
 	 * @psalm-taint-sink sql $y
 	 * @psalm-taint-sink sql $type
 	 */
-	public function eq($x, $y, $type = null): string;
+	public function eq($x, $y, $type = null): IQueryFunction;


from a naming perspective this should have been something like an IExpression

icewind1991 · 2022-06-20T15:48:54Z

Given the unexpected breakage I guess reverting this and using the more "minimal" version from #32708 instead.

icewind1991 · 2022-06-20T16:01:03Z

#32943

icewind1991 added the 2. developing Work in progress label Apr 13, 2022

icewind1991 force-pushed the unencrypted-size branch 5 times, most recently from 6dd55d3 to ab499e8 Compare April 20, 2022 12:44

blizzz added this to the Nextcloud 25 milestone Apr 21, 2022

icewind1991 force-pushed the unencrypted-size branch 2 times, most recently from 075c050 to 9467d9e Compare April 22, 2022 12:16

icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 22, 2022

icewind1991 requested review from a team, ArtificialOwl, CarlSchwan and vanpertsch and removed request for a team April 22, 2022 12:19

icewind1991 requested a review from come-nc April 25, 2022 12:24

szaimen reviewed Apr 25, 2022

View reviewed changes

lib/public/Files/Cache/ICacheEntry.php Outdated Show resolved Hide resolved

CarlSchwan approved these changes Apr 25, 2022

View reviewed changes

lib/public/Files/Cache/ICacheEntry.php Outdated Show resolved Hide resolved

lib/private/Files/Storage/Wrapper/Encryption.php Outdated Show resolved Hide resolved

lib/private/Files/Storage/Wrapper/Encryption.php Outdated Show resolved Hide resolved

PVince81 mentioned this pull request Apr 25, 2022

Server-side encryption is incompatible with using an object store as a primary data store #22077

Closed

icewind1991 force-pushed the unencrypted-size branch 3 times, most recently from 8f2d1f6 to 18ce8ec Compare April 28, 2022 13:31

icewind1991 added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 28, 2022

icewind1991 force-pushed the unencrypted-size branch 3 times, most recently from 526afc2 to 44a1b35 Compare April 28, 2022 14:13

come-nc removed their request for review May 5, 2022 14:59

PVince81 reviewed May 10, 2022

View reviewed changes

icewind1991 force-pushed the unencrypted-size branch from cb45fe3 to 636fe59 Compare May 25, 2022 14:02

icewind1991 requested a review from CarlSchwan May 25, 2022 14:26

icewind1991 added 3 commits June 2, 2022 15:52

make expression build return IQueryFunction instead of string

813b50e

this allows passing the expressions to further expressions without them being escaped as column names Signed-off-by: Robin Appelman <robin@icewind.nl>

add case statement to sql function builder

2a68819

Signed-off-by: Robin Appelman <robin@icewind.nl>

store unencrypted size in the unencrypted_size column

8238582

Signed-off-by: Robin Appelman <robin@icewind.nl>

icewind1991 force-pushed the unencrypted-size branch from b5c5969 to 8238582 Compare June 2, 2022 14:07

icewind1991 mentioned this pull request Jun 2, 2022

[24] store unencrypted size in the unencrypted_size column #32708

Merged

PVince81 approved these changes Jun 10, 2022

View reviewed changes

CarlSchwan approved these changes Jun 13, 2022

View reviewed changes

CarlSchwan added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 13, 2022

PVince81 merged commit 8809de1 into master Jun 13, 2022

PVince81 deleted the unencrypted-size branch June 13, 2022 09:55

ChristophWurst mentioned this pull request Jun 13, 2022

Work around breaking server change and QB expressions nextcloud/mail#6736

Merged

ChristophWurst reviewed Jun 13, 2022

View reviewed changes

icewind1991 mentioned this pull request Jun 20, 2022

Revert interface changes from "store unencrypted size in the unencrypted_size column" #32943

Merged

danxuliu mentioned this pull request Aug 8, 2022

Add integration tests for getting folder sizes #33484

Closed

juliusknorr mentioned this pull request Dec 7, 2022

Update the unencrypted file size when closing streams #35649

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store unencrypted size in the unencrypted_size column #31966

store unencrypted size in the unencrypted_size column #31966

icewind1991 commented Apr 13, 2022 •

edited

Loading

icewind1991 commented Apr 22, 2022

CarlSchwan left a comment

PVince81 commented Apr 25, 2022 •

edited

Loading

PVince81 May 10, 2022

PVince81 May 23, 2022

icewind1991 May 25, 2022

summersab May 30, 2022 •

edited

Loading

PVince81 commented May 23, 2022

PVince81 left a comment

DaphneMuller commented Jun 13, 2022

DaphneMuller commented Jun 13, 2022

PVince81 commented Jun 13, 2022

CarlSchwan commented Jun 13, 2022

PVince81 commented Jun 13, 2022

CarlSchwan commented Jun 13, 2022

ChristophWurst commented Jun 13, 2022 •

edited

Loading

ChristophWurst Jun 13, 2022

icewind1991 commented Jun 20, 2022

icewind1991 commented Jun 20, 2022

store unencrypted size in the unencrypted_size column #31966

store unencrypted size in the unencrypted_size column #31966

Conversation

icewind1991 commented Apr 13, 2022 • edited Loading

icewind1991 commented Apr 22, 2022

CarlSchwan left a comment

Choose a reason for hiding this comment

PVince81 commented Apr 25, 2022 • edited Loading

PVince81 May 10, 2022

Choose a reason for hiding this comment

PVince81 May 23, 2022

Choose a reason for hiding this comment

icewind1991 May 25, 2022

Choose a reason for hiding this comment

summersab May 30, 2022 • edited Loading

Choose a reason for hiding this comment

PVince81 commented May 23, 2022

PVince81 left a comment

Choose a reason for hiding this comment

DaphneMuller commented Jun 13, 2022

DaphneMuller commented Jun 13, 2022

PVince81 commented Jun 13, 2022

CarlSchwan commented Jun 13, 2022

PVince81 commented Jun 13, 2022

CarlSchwan commented Jun 13, 2022

ChristophWurst commented Jun 13, 2022 • edited Loading

ChristophWurst Jun 13, 2022

Choose a reason for hiding this comment

icewind1991 commented Jun 20, 2022

icewind1991 commented Jun 20, 2022

icewind1991 commented Apr 13, 2022 •

edited

Loading

PVince81 commented Apr 25, 2022 •

edited

Loading

summersab May 30, 2022 •

edited

Loading

ChristophWurst commented Jun 13, 2022 •

edited

Loading