Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share tags #11383

Merged
merged 11 commits into from Oct 16, 2014
Merged

Share tags #11383

merged 11 commits into from Oct 16, 2014

Conversation

@ockham
Copy link
Contributor

ockham commented Oct 1, 2014

Supersedes #11351.

*
* @param IDb $db Instance of the Db abstraction layer.
*/
public function __construct(IDb $db, $tableName) {

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Oct 1, 2014

Member

missing param $tableName

return $this->findEntities($sql, array_merge($owners, array($type)));
}

public function tagExists($tag) {

This comment has been minimized.

Copy link
@LukasReschke
* @param string $name The tag name to look for.
* @return string|bool The tag's id or false if it hasn't been saved yet.
*/
private function getTagId($name, $owner = null) {

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Oct 1, 2014

Member

missing $owner param in PHPDoc

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 1, 2014

CCing @babelouest

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tagging/tag.php Outdated
*/
class Tag extends Entity {

public $uid; // owner

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

why not name it owner? ;)

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tagging/tag.php Outdated

public $uid; // owner
public $type;
public $category; // tag name

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Why not name it name?

Self-descriptive code ... you know what I want to point out ;)

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Isn't there an ID for an tag? Because what happens when the tag name is changed? You don't want to update all those foreign keys that points to a tag ;)

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Absolutely. I would love to do the obvious, but unfortunately that's a limitation of the Entity/Mapper classes which requires those members to be named like the table columns they correspond to.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

There is an ID, yes -- that's automatically taken care of by Entity and Mapper, too.

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Then rename the table columns or is this mechanism already used?

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

cc @Raydiation ;)

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Well I bugged him about an ORM last night, so he knows. I guess I just need to sit down and file a bug report with a couple of good arguments.

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

Go ahead and create a PR if you want a "descriptive/config" way to do this by for instance adding an array with column to property mappings. Otherwise simply overwrite the columnToProperty and propertyToColumn method in your childclass, see https://github.com/owncloud/core/blob/master/lib/public/appframework/db/entity.php#L159

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

Example:

<?php

public function columnToProperty($column) {
    if ($column === 'category') {
        return 'name';
    } else {
        return parent::columnToProperty($column);
    }
}

This comment has been minimized.

Copy link
@ockham

ockham Oct 2, 2014

Author Contributor

Good one, thx.

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tagging/tagmapper.php Outdated
* @param string $tableName The name of the DB table to which we're mapping Tag objects.
*/
public function __construct(IDb $db, $tableName) {
if ($db === null) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

It should ALWAYS be injected.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Okay, I probably haven't fully grasped the injection pattern yet. You mean I need to get rid of the if ($db === null) check and make sure $db is always passed by the caller?

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Yes. Because the Mapper is created in the server container (that manages all the dependencies). And you just request an instantiated object of the type "TagMapper" ;)

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Hm. I'm not even quite sure how Tags have related to the server container so far. With all those static OCP\DB:: calls, I have a feeling that it didn't use injection much...

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Therefore we maybe can reuse the effort of touching all this and use dependency injection here too

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

The check is not needed since PHP enforces the value to not be null if you annotate it with a type

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tagging/tagmapper.php Outdated
\OCP\IDb;

/**
* Thin wrapper around \OCP\AppFramework\Db\Mapper.

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Not really helpful. But I also would just write "Mapper for Tag entity" or something like that (to make the API generator happy ;)).

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 13, 2014

Contributor

Can you change that to Mapper for Tag entity? Thanks

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
* @param string $user The user whose tags should be returned.
* @return array An array of Tag objects.
*/
public function getTagsForUser($user = null) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Shouldn't that be also put into the interface?

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Yeah, maybe. My original goal here is to change as little as possible with regard to the interface -- unless you set the $includeShared parameter in Tags' constructor, you shouldn't notice any difference in the behavior at all, as I wanted to make sure I'm not breaking an other apps that are already using Tags.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Not sure if this and getTagsForUser() are that useful to the outside, though -- if you want a list of tags that are relevant for a given user (i.e. owned by him, and shared with him if ou have $includeCollections=true), you can use getTags() like before.
I think I'd rather not put them into the interface for now; if need arises, it's easy to add them.

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

As this are just additional functions, there is no problem of having them. Otherwise you can't use them ;)

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

I'm only using them internally. I've been considering making them private or at least protected as for now, there more of an implementation detail really, I think...

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Then they shouldn't be public. That's right

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

Why can user be null? If you want to return all tags of a user, simply create a second method, thats simpler

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
* @param string $user The user for whose tags to check.
* @return bool
*/
public function userHasTag($name, $user = null) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Put this into interface too?

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

again, no idea why user can be null :)

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
*/
public function __construct($user, $type, $defaultTags = array()) {
public function __construct($user, $type, $defaultTags = array(), $includeShared = false, IDb $db = null) {
if ($db === null) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Same with the injection here.

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
if ($db === null) {
$db = new \OC\AppFramework\Db\Db();
}
$this->mapper = new TagMapper($db, self::TAG_TABLE_NAME);

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Forgot about the injection of $db. Just inject the mapper and use specify the table name there. Or should there be multiple tables for all tags? But if I understood the proposal correct, there should be just one type of tags, that can be used by all apps.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

This PR isn't really about proposal #3812, it's a requirement for fixing owncloud/contacts#38.
As for tables, there really is another table we're already using here, the relation table, for which I'm not using any mapper currently. I'm really not that acquainted with the injection pattern, but to me, it sounds better to inject $db into Tags, and from there into TagMapper than injecting a (possibly varying) number of mappers into Tags. (And before all that, have TagManager inject it into Tags.)

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

But once we touch this, we could do it in a proper way.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Yeah. I just need some guidance here. Is what I sketched about right?

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

After my first look at the code it looks quite good.

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Thx ;-) Actually, I meant if the following was right:

inject $db into Tags, and from there into TagMapper than injecting a (possibly varying) number of mappers into Tags. (And before all that, have TagManager inject it into Tags.)

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Therefore I need to better understand all the stuff here, but it's already late and I'm not able to do that now

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

Sure, no problem. Same here, actually.

$this->owners = array($this->user);
if ($this->includeShared) {
$this->owners = array_merge($this->owners, \OC\Share\Share::getSharedItemsOwners($this->user, $this->type, true));
$this->backend = \OC\Share\Share::getBackend($this->type);

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

inject the backend too. But maybe I just not get what the backend does, yet.

@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
if (\OCP\DB::isError($result)) {
\OCP\Util::writeLog('core', __METHOD__. ', DB error: ' . \OCP\DB::getErrorMessage($result), \OCP\Util::ERROR);
}
$this->tags = $this->mapper->loadTags($this->owners, $this->type);
} catch(\Exception $e) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Catch really needed? Shouldn't that be handled by the mapper?

This comment has been minimized.

Copy link
@ockham

ockham Oct 3, 2014

Author Contributor

Hm. We're basically just logging the exception (currently through the use of a static function, which should probably also be replaced by an injected logger object.) So is it better to have the mapper to take care of that, then? (We still might need the Tags class itself to log other possible errors, too.)

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 3, 2014

Contributor

In general its best to handle exceptions on the level where they can be handled. Another best practice is to prevent leaky abstractions, e.g.: your Tags class should not throw exceptions from the database mapper but catch them and throw a separate exception. The question is why an error is just silently swallowed (if one is expected)

This comment has been minimized.

Copy link
@ockham

ockham Oct 3, 2014

Author Contributor

Okay, so in this case I guess I'll move that try... catch block to the mapper.

This comment has been minimized.

Copy link
@ockham

ockham Oct 3, 2014

Author Contributor

Or do I? On second reading, your comment isn't quite clear which option is preferrable...

This comment has been minimized.

Copy link
@ockham

ockham Oct 3, 2014

Author Contributor

About silently swallowing the error, good question, pre-existing code here, to. I guess it was meant to guard against any kind of DB access related error. Is it safe to just drop the try/catch here?

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 3, 2014

Contributor

I dont know why there should be an exception anyways. IMHO if you dont expect an exception, dont catch one. If some exception is thrown its better to not do anything and let the usercode handle the unexpected exception

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 3, 2014

Contributor

About the previous comment: If you expect a DB exception, catch it and throw a service exception to prevent a leaky abstraction. The loadTags method should be a simple select so i have no idea why an exception is being caught

This comment has been minimized.

Copy link
@ockham

ockham Oct 3, 2014

Author Contributor

Me neither, really; as I said, legacy code. Going to remove it.

@@ -203,7 +261,22 @@ public function getIdsForTag($tag) {

if(!is_null($result)) {
while( $row = $result->fetchRow()) {
$ids[] = (int)$row['objid'];

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

What about using the mapper for this and the above stuff?

This comment has been minimized.

Copy link
@ockham

ockham Oct 1, 2014

Author Contributor

This is the relation table, I don't have a mapper for that.

This comment has been minimized.

Copy link
@MorrisJobke
@MorrisJobke
MorrisJobke reviewed Oct 1, 2014
View changes
lib/private/tags.php Outdated
unset($this->tags[$key]);
try {
$this->mapper->delete($tag);
} catch(\Exception $e) {

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Oct 1, 2014

Contributor

Shouldn't that be caught by the mapper?

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 1, 2014

I just read over the code. It looks good, but I need additional time to test against the mechanism.

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 1, 2014

You maybe should explain (or point to the explaination) what are those types and backends. That would be nice.

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 1, 2014

The backend stuff is kinda tricky, I explained it in one commit in my contacts related PR:

Fix and re-activate Contact sharing backend.

While not needed for actual contacts sharing (which is solely done via
the Addressbook sharing backend), we need its isValidSource() function
in core's \OC\Tags::getIdsForTag() (which implements \OCP\ITags::getIdsForTag()).
It is required there to test if the items that are tagged with a given
tag are actually allowed to be accessed by the user, i.e. owned by the
user or someone who is sharing it with the user.

Does that make it clearer?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Oct 2, 2014

@ockham your sql statements are invalid on postgres and oracle - please have a look: https://ci.owncloud.org/job/pull-request-analyser/7675/console

In case you need support on this feel free to get in touch with me - THX

@BernhardPosselt
BernhardPosselt reviewed Oct 2, 2014
View changes
lib/private/tagging/tag.php Outdated
* in parentheses, like "Tag (owner)".
*/
public function getDisplayName() {
if ($this->uid != \OCP\User::getUser()) {

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

The user needs to be passed in, either through the constructor or the method, but Id prefer getDisplayName($userName), but I'm unsure if it is a good idea to do this in the backend. IMHO this belongs to the frontend

@BernhardPosselt
BernhardPosselt reviewed Oct 2, 2014
View changes
lib/private/tags.php Outdated
*
* @var bool
*/
private $includeShared = null;

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

it is null if not initialized

@BernhardPosselt
BernhardPosselt reviewed Oct 2, 2014
View changes
lib/private/tags.php Outdated
*
* @var TagMapper
*/
private $mapper = null;

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

same here

@BernhardPosselt
BernhardPosselt reviewed Oct 2, 2014
View changes
lib/private/tags.php Outdated
*
* @var \OCP\Share_Backend
*/
private $backend = null;

This comment has been minimized.

Copy link
@BernhardPosselt

BernhardPosselt Oct 2, 2014

Contributor

and here :P

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 13, 2014

@MorrisJobke ok, one moment

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 13, 2014

Wait a second, aren't those failures due to some CI server's full hard disk?

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 13, 2014

@ockham THey were fixed today.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Oct 13, 2014

The inspection completed: 20 new issues, 20 updated code elements

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 13, 2014

@ockham Usually you don't need to say "retest this" as jenkins will notice changes (if you push to a branch) So the retest was just used to test even if there was no change (regarding the commit sha sum of the last test) and somehow the jenkins infrastructure was fixed in the meantime (as this caused the false negative)

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 14, 2014

@DeepDiver1975 Looks like a problem with the filesystem:


Generating code coverage report in HTML format ...PHP Fatal error:  Uncaught exception 'RuntimeException' with message 'Could not write to coverage-html-sqlite/index.dashboard.html: failed to open stream: Read-only file system' in /usr/share/php/Text/Template.php:164
@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Oct 14, 2014

@MorrisJobke We're on it.

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Oct 14, 2014

@LukasReschke Thanks

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 15, 2014

Review, please?
Pinging @Raydiation @DeepDiver1975

@ockham

This comment has been minimized.

Copy link
Contributor Author

ockham commented Oct 15, 2014

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 15, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/259/
🚀 Test PASSed. 🚀

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Oct 16, 2014

Well, let's make @ockham happy: 👍

LukasReschke added a commit that referenced this pull request Oct 16, 2014
@LukasReschke LukasReschke merged commit 5f3ddf5 into owncloud:master Oct 16, 2014
1 check passed
1 check passed
default Merged build finished.
Details
@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Oct 16, 2014

Merging as @MorrisJobke also 👍'd this before.

@ockham Thanks a lot for your contribution! - You rock!

@ockham ockham deleted the ockham:share-tags2 branch Oct 23, 2014
@jbtbnl

This comment has been minimized.

Copy link
Member

jbtbnl commented on lib/private/tags.php in 7e9baaf Nov 2, 2014

This breaks group removal as described in https://github.com/owncloud/contacts/issues/691 since a string containing a number will be treated as numeric as well

@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.