Details

Reviewers

Group Reviewers

Restricted Project

Commits

rK9d6803eabc7e: Collection support for filesystem items
rKf7597e9a4021: Collection support for filesystem items
rK5e4dd07ba498: Collection support for filesystem items
rKe283207dcdad: Collection support for filesystem items
rKfee143802a25: Collection support for filesystem items
rK7aa229833b5b: Collection support for filesystem items
rKb05eceb4d9b6: Collection support for filesystem items

Summary

This patch adds support for collections and relations to group items
into collections.

This patch also:

Replaces the FilesController with FsController
Grants the companion app access to the files api (via scope)
Allows muliple files with the same name
Allows the client to set the mimetype

Fixed filestorage tests

Support for collections

Create collections with some metadata

UI support for collections

Diff Detail

Repository

rK kolab

Lint

Lint Errors

Severity	Location	Code	Message
Error	src/app/Fs/Item.php:178	phpstan	Class App\Fs\Fs\Relation not found.
Error	src/app/Fs/Item.php:188	phpstan	Method App\Fs\Item::children() should return Illuminate\Database\Eloquent\Relations\HasMany but returns Illuminate\Database\Eloquent\Relations\BelongsToMany<App\Fs\Item>.
Error	src/app/Fs/Item.php:198	phpstan	Method App\Fs\Item::parents() should return Illuminate\Database\Eloquent\Relations\HasMany but returns Illuminate\Database\Eloquent\Relations\BelongsToMany<App\Fs\Item>.
Error	src/app/Fs/Relation.php:17	phpstan	Property App\Fs\Relation::$fillable (array<string, string>) does not accept default value of type array<int, string>.
Error	src/app/Http/Controllers/API/V4/FsController.php:395	PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExact	Generic.WhiteSpace.ScopeIndent.IncorrectExact
Error	src/app/Http/Controllers/API/V4/FsController.php:395	PHPCS.E.Squiz.WhiteSpace.ScopeClosingBrace.Indent	Squiz.WhiteSpace.ScopeClosingBrace.Indent
Error	src/app/Http/Controllers/API/V4/FsController.php:418	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::attach().
Error	src/app/Http/Controllers/API/V4/FsController.php:471	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::attach().
Error	src/app/Http/Controllers/API/V4/FsController.php:475	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::attach().
Error	src/app/Http/Controllers/API/V4/FsController.php:515	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::sync().
Error	src/app/Http/Controllers/API/V4/FsController.php:519	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::attach().
Error	src/app/Http/Controllers/API/V4/FsController.php:523	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::detach().
Error	src/routes/api.php:105	phpstan	Class App\Http\Controllers\API\V4\FilesController not found.
Error	src/routes/api.php:107	phpstan	Class App\Http\Controllers\API\V4\FilesController not found.
Error	src/tests/Feature/FsTest.php:936	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::attach().
Error	src/tests/Feature/FsTest.php:957	phpstan	Call to an undefined method Illuminate\Database\Eloquent\Relations\HasMany<App\Fs\Item>::detach().

Unit

No Test Coverage

Build Status

Buildable 42089
Build 16937: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Mar 23 2023, 9:01 AM

mollekopf created this revision.

Harbormaster completed remote builds in B42089: Diff 11966.Mar 23 2023, 9:01 AM

mollekopf retitled this revision from Collection support for filesystem items to WIP: Collection support for filesystem items.Mar 23 2023, 9:09 AM

mollekopf added a reviewer: Restricted Project.

Cleanup

Harbormaster completed remote builds in B42092: Diff 11972.Mar 23 2023, 9:22 AM

The api will require a bit of a cleanup in that I'm currently using /items/ but some existing code still uses /files/. I thought about using /fs/ instead.
Should I standardize everything on /fs/ ?

Updated description

Harbormaster completed remote builds in B42095: Diff 11978.Mar 23 2023, 9:27 AM

FWIW: Not using auto-increment on the pivot table fs_relations (I think that's the name in laravel?) IMO makes no sense. The only reason there is an id it all is because laravel needs it. Logically it should be a composite primary key from the two foreign keys.

Deduplication during upload for files, fixed updating parents

Harbormaster completed remote builds in B42134: Diff 12026.Mar 25 2023, 3:01 PM

Moved to api/v4/fs/

Harbormaster completed remote builds in B42269: Diff 12146.Mar 29 2023, 3:07 PM

machniak requested changes to this revision.Mar 30 2023, 2:34 PM

machniak subscribed.

machniak added inline comments.

src/app/Backends/Storage.php
130	I'm not sure we should trust the client in this. This potentially might become a security issue.
src/app/Fs/Item.php
172	s/user/item/
182	The descriptions of these three methods should probably explain the difference between them in some way.
src/app/Fs/Relation.php
10	s/item/relation/
13	Missing $item property. I'm also not sure about these column names. It looks to me that it might get confusing and/or harder to use if we create a record that has item=collection_id,related=file_id and another with item=file_id,related=collection_id. Also in `fs_properties` we have `item_id` column, so here it also should be `item_id` not `item`.
src/app/Http/Controllers/API/V4/CompanionAppsController.php
250	The scope should be "fs" maybe?
src/app/Http/Controllers/API/V4/FsController.php
384	You dropped TYPE_INCOMPLETE here.
408	I think we should have some sanity checks on the input.
415	Since the input is JSON, we can just have a "properties" (or "settings") property.
416	Key length is limited to 191 chars (or actually bytes). It should be validated too, e.g. accept only letters, digits and '-' or dots.
706	Item::
709	I not "collection" but type & Item::TYPE_FILE then "file", else "incomplete" or "unknown" (or null). Also these should be constants probably
src/resources/js/files.js
116	config.params.parent = params.parent
src/routes/api.php
102	Duplicated routes.

This revision now requires changes to proceed.Mar 30 2023, 2:34 PM

Addressed comments

Harbormaster completed remote builds in B42332: Diff 12194.Apr 3 2023, 3:53 PM

Some more input validation

Harbormaster completed remote builds in B42335: Diff 12197.Apr 3 2023, 4:09 PM

mollekopf added inline comments.Apr 3 2023, 4:09 PM

src/app/Backends/Storage.php
130	There may be a security argument to be had, but the client just has better mimetype info in the case of image uploads it seems. Also, I think generally mimetype info is just file-metadata that the client should supply, because it's just not included in the actual file. It's possible to derive a mimetype from known file-formats, but it's not generally included. So for a generic file storage the client must supply the mimetype. Doing security checks such as "you may not supply a file that doesn't match the mime-type" might an interesting enhancement, but I don't think the mimetype deduction logic is currently there, and I think we should do it in a separate step.
src/app/Fs/Relation.php
13	I agree that relating collections to items doesn't currently make much sense, perhaps it does in the future with a relation-type introduced (but for now we just don't). I think the non-confusing approach would be to completely separate files and collections and not have the generic item concept. We went with the item concept because we liked that it's generic and it's potential performance benefits for queries. IMO it's manageable as it is, so I'm not too worried about it.
src/app/Http/Controllers/API/V4/FsController.php
415	I'm using the prefix because the mobile app uses query parameters to set the properties. We can't really use the json body as is, because the body is the file. The alternatives would be to somehow use headers (prefix or json blob), or use http multipart with a json part at first (more complex).

machniak added inline comments.Apr 3 2023, 6:02 PM

src/app/Http/Controllers/API/V4/FsController.php
415	When creating a collection we could use JSON input, but ok. For consistency we should probably use headers, e.g. X-Kolab-Property-*

mollekopf added inline comments.Apr 5 2023, 11:32 AM

src/app/Http/Controllers/API/V4/FsController.php
415	Right, I am actually using a json body for collections. Laravel's input() picks data up from parameters or json bodies, so the the controller this becomes transparent. In the mobile app I'm using json for collections and parameters for files, which I agree is not fantastic, but works out fine on the controller side due to laravels abstraction. I suppose we could use headers, I didn't because the API felt a bit clunkier, and we've been using parameters for e.g. the name. If we do use headers, we should use them across the board for: 'name' 'collectionType' 'parent' 'deviceId' 'property-localId' 'deduplicate-property' 'deduplicate-value'

mollekopf marked 2 inline comments as done.Apr 5 2023, 11:36 AM

machniak requested changes to this revision.Apr 14 2023, 11:55 AM

machniak added inline comments.

src/app/Http/Controllers/API/V4/FsController.php
429	You have to pass the 'attribute' value to trans() here.
432	It would be good to use trans() here.
434	I'd suggest to create $properties array and pass it to setProperties() once at the end. Also, I'd move this whole foreach before `deduplicateOrCreate()` call, as it's merely a validation logic. As it is right now it can create the item and then return 422 on propertyKey validation error.
444	`self::TYPE_COLLECTION`
471	We need the same properties validation logic as in createCollection(). I also think that maybe we should for now limit the size of their values.
480	This is redundant if we have X-Kolab-Parents. Also, I'd move parents handling to be after the try/catch block below.
513	I'd move parents handling after any input validation.
src/app/Providers/PassportServiceProvider.php
23	It should be 'fs' , right?
src/resources/vue/File/Info.vue
111	files -> fs
116	files -> fs
src/resources/vue/File/List.vue
40	fs -> files

This revision now requires changes to proceed.Apr 14 2023, 11:55 AM

Addressed comments

Harbormaster completed remote builds in B42467: Diff 12260.Apr 17 2023, 10:08 AM

mollekopf added inline comments.Apr 17 2023, 10:57 AM

src/app/Http/Controllers/API/V4/FsController.php
471	Why? We always have the database imposed limits.
480	We're using this to set the parent via parameters.

lint and trans changes

Harbormaster completed remote builds in B42473: Diff 12266.Apr 17 2023, 10:59 AM

another lint issue

Harbormaster completed remote builds in B42476: Diff 12272.Apr 17 2023, 11:02 AM

machniak added inline comments.Apr 17 2023, 11:17 AM

src/app/Http/Controllers/API/V4/FsController.php
471	Ah, I forgot that TEXT type is limited to 65,535 bytes. So, I think it's not that big of a deal.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2023, 11:42 AM

Closed by commit rK9d6803eabc7e: Collection support for filesystem items (authored by mollekopf). · Explain Why

This revision was automatically updated to reflect the committed changes.

Collection support for filesystem items
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 11966

src/app/Backends/Storage.php

src/app/Fs/Item.php

src/app/Fs/Relation.php

src/app/Http/Controllers/API/V4/CompanionAppsController.php

src/app/Http/Controllers/API/V4/FilesController.php

src/app/Http/Controllers/API/V4/FsController.php

src/app/Providers/PassportServiceProvider.php

src/app/Rules/FileName.php

src/database/migrations/2023_03_05_100000_fs_relations_table.php

src/resources/js/files.js

src/resources/js/user/routes.js

src/resources/lang/en/app.php

src/resources/lang/en/ui.php

src/resources/vue/File/Info.vue

src/resources/vue/File/List.vue

src/resources/vue/Widgets/ListTools.vue

src/routes/api.php

src/tests/Feature/Controller/FilesTest.php

src/tests/Feature/EntitlementTest.php

src/tests/Feature/FsTest.php

Collection support for filesystem itemsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 11966

src/app/Backends/Storage.php

src/app/Fs/Item.php

src/app/Fs/Relation.php

src/app/Http/Controllers/API/V4/CompanionAppsController.php

src/app/Http/Controllers/API/V4/FilesController.php

src/app/Http/Controllers/API/V4/FsController.php

src/app/Providers/PassportServiceProvider.php

src/app/Rules/FileName.php

src/database/migrations/2023_03_05_100000_fs_relations_table.php

src/resources/js/files.js

src/resources/js/user/routes.js

src/resources/lang/en/app.php

src/resources/lang/en/ui.php

src/resources/vue/File/Info.vue

src/resources/vue/File/List.vue

src/resources/vue/Widgets/ListTools.vue

src/routes/api.php

src/tests/Feature/Controller/FilesTest.php

src/tests/Feature/EntitlementTest.php

src/tests/Feature/FsTest.php

Collection support for filesystem items
ClosedPublic
Actions

Revision Contents
Changeset List