Implement group backend

[PVince81]

Implement group manager.

To test:

1. Enable app
2. Create three users "user1", "user2", "user3"
3. insert into oc_custom_group values (NULL, 'myfriends', 'My friends');
4. insert into oc_custom_group_member values (1, 'user2', 1);
5. insert into oc_custom_group_member values (1, 'user3', 0);
6. Login as "user1"
7. Create a folder "test"
8. Share with "My fri" let it autocomplete: select "customgroup_1" (need future work to support display names)
9. Login as "user2" and see that the folder is visible

Some concerns:

• current impl lets any user share with anybody, unless admin ticks in admin page "only allow sharing with groups in which user is member", this could be fine as is...
• need display name support in core, might need to change some group manager APIs to support returning arrays instead of just the group ids

@DeepDiver1975 @butonic @jvillafanez what do you think ?

[CLAassistant]

All committers have signed the CLA.

[PVince81]

also:

• custom groups are visible in user manager. Can't hide them because making getGroups return an empty would break searching in the sharing autocomplete. Might need changing core APIs to let the manager specify if listable or not.

[jvillafanez]

I don't think it's a good idea to hardcode this. The extra cost of calculating the string's length should be meaningless, and we would only maintain one constant.

[jvillafanez]

Do we want lowercase comparations?

[PVince81]

For display names I'd say yes

[jvillafanez]

Isn't this a bit complex? Why do we want to use prefix?

[PVince81]

The prefix is to avoid collisions with groups created in other backends like LDAP, etc.

Also we're using a numeric group id in this backend internally to be able to rename the display name.
Another alternative would be to generate a random hash string id instead of numeric... not sure what's best, I thought this approach would be the simplest.

[jvillafanez]

I don't think it's the backend responsability to resolve the collisions nor to prevent them. Any other backend could have a group with the same name "customgroup_12", and there shouldn't be any reason to avoid it.

I think it's core the one who should take care of how to resolve the collisions. Generally, just saying "I want this group" is enough to know what is the group you're refering to, but maybe sometimes you should say "I want this group which is located in this backend" so there is no doubt about the group you want.

[PVince81]

While I agree that it's not this backend's responsibility, we'll still get into trouble if all our groups are called "1", "2", "3", etc... And I don't intend to fix core to have its own collision detection / namespacing as part of this task, which is likely a bigger task on its own which might not be backward compatible.

[PVince81]

• TODO: add unit tests
• TODO: remove LEN constant

[jvillafanez]

Just a couple of extra things:

• From my point of view ( @DeepDiver1975 correct me if I'm wrong) there should be only one group manager and user manager, which are the ones provided by core. Creating and using another manager shouldn't be possible because both core and any app are interested in using the "official" manager.
• Custom backends, such as LDAP or this one could be registered in the core manager. This also includes the DB backend which is used by default in ownCloud.

[PVince81]

@jvillafanez yes sorry, there is only one group manager.

What this PR implements is a group backend like LDAP does. (I mixed up both names...)

[PVince81]

• might need adjusting the whole PR depending on how we implement the new group backend interfaces as described in Group name vs display name core#7918 (comment) for display name support

[PVince81]

Anyway, my first goal with this PR was to experiment to find out whether sharing would work automatically with this approach. It did!

[PVince81]

• now requires Adding group display name support in group backends core#26750 for display name support and the new IGroupBackend interface

If you test this you'll see that the display name is correctly shown in the sharee list 😄

[PVince81]

• TODO: every member can also be a group admin, need to change the data structure

[PVince81]

Ok, I spent too much time already implementing the Sabre stuff but not enough writing actual tests for GroupsManager 😄

Will do tomorrow so we can get this first PR merged soon.

[PVince81]

I've added unit tests now for the group backend and group manager.
Note that for the tests to run properly you also need to checkout the matching core branch owncloud/core#26750
(or ideally review that one first).

I've removed the DAV stuff from this PR into a separate PR: #7

Please review @DeepDiver1975 @jvillafanez

[PVince81]

@DeepDiver1975 mind reviewing so I can continue with the DAV stuff ? Thanks

[jvillafanez]

I'm questioning the purpose of this class... does it make the code too complex if we move this code to the backend?

Anyway, we should rename the class. The backend class will have a manager above (provided by core, where the backend will be added) and also below (this class) which is confusing. CustomGroupsDBHandler seems a better choice.

[PVince81]

The purpose of the manager is to be used only by this one app to manage custom groups.

The backend itself only uses a subset of operations that it exposes to core because we don't want the users page to allow managing these, but we still want the search to work for the sharing autocomplete. Furthermore the backend class needs to work with core-style group ids (strings) while the manager works with this app's specific numeric id, so a conversion needs to be done.
I prefer to keep these separate to make the code clearer.

If I would move the extra operations to manage group members to the backend class as well, I don't want to expose this to the core interface. So we'd have two methods addToGroup, one that is empty for the backend and another one that is only known by the customgroups app. That's not very clean and clear to understand.

Hope this clears it out a bit 😄

[jvillafanez]

ok, got it, but I'd still rename the class to reduce confusion.

[PVince81]

you're right, this is more a database accessor. But we never used the name "DBHandler" before.

In core we used manager for "CommentsManager" and "SystemTagsManager" which do the same.
Some older call calls it "Mapper" like "TagMapper", but it's mostly because they use real objects instead of just arrays.

What do you think ?

[jvillafanez]

I'm not sure if we should add another index only for the user_id. I expect the queries for groups will use this index, but I don't think the queries for users will.

[PVince81]

Ah, I thought that this was the proper way to define multi-column primary unique keys ?
I copied this idea from core's group_user table.

@DeepDiver1975

[jvillafanez]

The multi-column index is fine because we want group_id + user_id be unique. I trust the DBMSs be smart enough to use this index when we query for group information, but I don't think this will happen for user information.

[jvillafanez]

I don't know where this come from.

[PVince81]

What do you mean ? This method is called in appinfo/app.php

[jvillafanez]

The \OCA\CustomGroups\CustomGroupsBackend needs to be register in the container, but I don't know where this registration is made.

[PVince81]

No, it doesn't. DI container power !

It works because the class name matches exactly so it's able to resolve it.
You only need explicit registration when the names different or when your constructor requires parameters that cannot be injected (ex: strings)

[jvillafanez]

😮 👏

[PVince81]

Restarted travis build now that master contains the code we need

[PVince81]

• rename CustomGroupsManager to something that sounds more like it's a DB accessor

[PVince81]

• challenge: implement initial DB structure with doctrine migration

[PVince81]

Looks like for the Mapper I can throw away almost all my code (and unit tests) and start over. I don't think the benefit of having the mapper pattern here is bringing any significant benefit at this point. (it might even introduce more hurdles for the API consumer)

I'll abandon this approach and simply rename the manager to CustomGroupsDatabaseHandler.

[PVince81]

Raised #10 to use doctrine migrations for initial DB.

I'd like to get this PR here merged so I can move forward with the Sabre plugins / API. That is, unless there is a no-go in this PR.

[jvillafanez]

we might want to document how the search is done, case-insensitive and in the middle of the string.

[jvillafanez]

$limit = 0 is acceptable?

[jvillafanez]

why don't return $result?

[PVince81]

because $result is false, not null...

[jvillafanez]

We should log this exception if possible. I think it's fine for now to keep the dependencies as simple as possible, but it's something to be considered.

[jvillafanez]

I'd wrap this in a transaction. If we want to delete a group, we want both actions to be executed or none of them. Something in the middle it be would be an unexpected result.

[PVince81]

Also, looks like sqlite not happy with my DB stuff. Will look into it. (Mysql worked fine)

[PVince81]

Something is wrong with the unique constraint. Looks like SQLite set the primary key only on "group_id" instead of both "group_id" + "user_id" 😢

[PVince81]

sqlite> pragma table_info(oc_custom_group_member);
cid|name|type|notnull|dflt_value|pk
0|group_id|INTEGER|1||1
1|user_id|VARCHAR(64)|1||0
2|is_admin|INTEGER|1|0|0

primary key is only set on group_id even though I asked it to set it on both:

    $table->setPrimaryKey(['group_id', 'user_id']);

debugging....

[PVince81]

grrr, this is the resulting SQL generated by Doctrine:

  $sql[2]                        = (string[156]) 'CREATE TABLE oc_custom_group_member (group_id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, user_id VARCHAR(64) NOT NULL, is_admin INTEGER DEFAULT 0 NOT NULL)';

I didn't even ask for autoincrement... Looks like a Doctrine bug.

We do have a table in core with a similar schema and that one looks correct:

sqlite> pragma table_info(oc_group_user);
cid|name|type|notnull|dflt_value|pk
0|gid|VARCHAR(64)|1|''|1
1|uid|VARCHAR(64)|1|''|2

so this means SQLite supports multi-column primary keys but Doctrine doesn't seem to be able to create that.

Googling now, I hope there is maybe a missing flag or something in the API.

[PVince81]

Facepalm, I forgot to remove "autoincrement" when copy-pasting the definitions.
Funnily enough, Mysql didn't complain.

[PVince81]

@jvillafanez I fixed all your comments and additionally fixed the DB schema.

[PVince81]

@DeepDiver1975 can you create the repo for clover coverage ? https://travis-ci.org/owncloud/customgroups/jobs/183218691#L831

[jvillafanez]

👍

[PVince81]

I'm going to remove the code coverage thing for now and let @DeepDiver1975 sort it out separately.

[PVince81]

Raised here #12