Skip to content

Conversation

@ChristophWurst
Copy link
Member

This is the result of a bit of discussion about #24410 with @tobiasKaminsky and affectively an alternative solution. In the first PR we settled with an extension of the API so that each result entry can have a type and a object ID as that seemed generic enough. Now we discussed whether file search results should use the file ID, the remote file ID or the remote path for the ID. This lead me to the idea that we may want to be more flexible, as in allow each result to have any arbitrary number of attributes. Then we can put type, IDs and anything else there. The API is restrictive enough to only allow strings. Also if you use a composite key (Deck card on a specific board for example) you can now specify the key with two attributes and don't have to serialize it into the single field of the other PR.

@blizzz @MorrisJobke @nickvergessen @rullzer what do you think? Which solution would you prefer?

I'd summarize that #24410 is more restrictive and this one more adaptable for future changes and adding more than one way to identify an object – files could specify all three of the IDs mentioned above and the clients use whatever is most convenient for them.

@blizzz
Copy link
Member

blizzz commented Dec 2, 2020

@blizzz @MorrisJobke @nickvergessen @rullzer what do you think? Which solution would you prefer?

this one sounds good and flexible 👍 i have other places where i serialize data into one field. It works, but is useless overhead.

@tobiasKaminsky
Copy link
Member

I favor this over #24410

I am now unsure, if we first want to test this with e.g. Deck @juliushaertl and Deck app (@stefan-niedermann) or if we assume that this is flexible enough to handle everything (and that they how we do it, is ok).

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Seems reasonable 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 3, 2020
@MorrisJobke
Copy link
Member

@ChristophWurst Mind to rebase and squash?

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@faily-bot
Copy link

faily-bot bot commented Dec 3, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 156: failure

mysql8.0-php7.4

Show full log
There was 1 error:

1) Test\Files\Storage\Wrapper\PermissionsMaskTest::testScanNewWrappedFiles
Error: Call to a member function getPermissions() on bool

/drone/src/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php:126

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

@ChristophWurst ChristophWurst merged commit 32ded87 into master Dec 4, 2020
@ChristophWurst ChristophWurst deleted the enhancement/unified-search-result-attributes branch December 4, 2020 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish client: 🤖🍏 mobile feature: search

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants