Page MenuHomePhorge

Collection support for filesystem items
ClosedPublic

Authored by mollekopf on Mar 23 2023, 9:01 AM.
Tags
None
Referenced Files
F12283047: D4190.id12266.diff
Mon, May 20, 9:18 PM
F12272016: D4190.diff
Mon, May 20, 1:33 PM
Unknown Object (File)
Sat, May 18, 7:50 PM
Unknown Object (File)
Sat, May 18, 7:29 PM
Unknown Object (File)
Sat, May 18, 4:33 PM
Unknown Object (File)
Sat, May 18, 4:30 PM
Unknown Object (File)
Sat, May 18, 3:45 PM
Unknown Object (File)
Sat, May 18, 3:44 PM
Subscribers

Details

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf created this revision.
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.
mollekopf retitled this revision from WIP: Collection support for filesystem items to Collection support for filesystem items.

Cleanup

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/ ?

mollekopf edited the summary of this revision. (Show Details)

Updated description

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

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.

412

I think we should have some sanity checks on the input.

419

Since the input is JSON, we can just have a "properties" (or "settings") property.

420

Key length is limited to 191 chars (or actually bytes). It should be validated too, e.g. accept only letters, digits and '-' or dots.

763

Item::

766

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
114

Duplicated routes.

This revision now requires changes to proceed.Mar 30 2023, 2:34 PM
mollekopf marked 11 inline comments as done.

Addressed comments

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
419

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).

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

When creating a collection we could use JSON input, but ok. For consistency we should probably use headers, e.g. X-Kolab-Property-*

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

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'
machniak added inline comments.
src/app/Http/Controllers/API/V4/FsController.php
433

You have to pass the 'attribute' value to trans() here.

436

It would be good to use trans() here.

438

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.

475

self::TYPE_COLLECTION

537

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.

546

This is redundant if we have X-Kolab-Parents. Also, I'd move parents handling to be after the try/catch block below.

570

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
mollekopf marked 9 inline comments as done.

Addressed comments

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

Why? We always have the database imposed limits.

546

We're using this to set the parent via parameters.

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

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
This revision was automatically updated to reflect the committed changes.