Add DispatchingTermLookup #151

jakobw · 2016-10-12T11:22:12Z

adds a service that picks a TermLookup for the corresponding repository for a given EntityId
implemented on top of Add DispatchingEntityLookup #150 in order to reuse UnknownForeignRepositoryException

Task on Phabricator: https://phabricator.wikimedia.org/T149583

manicki

Looks good at glance, generally follows the way other dispatching services have been proposed so far.

Found two minor things, one being my mistake actually.

I see you have separate tests for "entity from local repo" and "entity from foreign repo" cases. I am a bit torn on this (I originally also made such division in DispatchingEntityLookup's tests). On one hand local repo case is a bit special and different from the foreign entity case. On the other hand, technically local repo and any foreign repo are pretty much the same thing for the dispatcher, so I started to think that maybe it would be enough to just check in tests that some things work for the known repo (be it local or foreign, it does not matter), and that for unknown repo exceptions are thrown etc.
I'd really appreciate other's opinions on this. We're getting more and more of those dispatchers, so it would be nice to have tests consistent at the early stage, I think.

According to docs all public methods of TermLookup throw TermLookupException. Those exception should not be caught be the dispatching wrapper. From looking at the code I can tell it does not happen but some small unit tests proving this would be nice too.

manicki · 2016-10-12T13:47:22Z

src/Lookup/DispatchingTermLookup.php

+
+	/**
+	 * @param TermLookup[] $lookups associative array with repository names (strings) as keys
+	 *                                and TermLookup objects as values. Empty-string key


indentation is a bit off here and line below

manicki · 2016-10-12T14:02:44Z

src/Lookup/DispatchingTermLookup.php

+	private function getLookupForEntityId( EntityId $entityId ) {
+		$repo = $entityId->getRepositoryName();
+		if ( !isset( $this->lookups[$repo] ) ) {
+			throw new UnknownForeignRepositoryException( $entityId, 'Unknown repository: ' . $repo );


Just realized that the second parameter passed to the constructor could be possibly skipped in all dispatching service patches as it is not really relevant, ie. ''Unknown repository name: foo" vs "'Unknown repository: foo".
I'll remove it my patches. If shorter message is preferred, it should be changed in the exception. I have no preference here.

manicki · 2016-10-12T14:06:53Z

tests/unit/Lookup/DispatchingTermLookupTest.php

+		] );
+
+		$this->setExpectedException( UnknownForeignRepositoryException::class );
+		$lookup->$method( $item, $language );


Let's see if we will have @JeroenDeDauw ranting on this line :)

:)
I was too lazy to write the same thing 4x but I'm fine with changing it.

I think for the negative test, this is ok.

jakobw · 2016-10-12T14:54:14Z

Thanks @manicki for the review!

I agree with what you said about testing the two cases for each method and would also like to hear a third opinion on it. I have no strong preference.

Regarding TermLookupException I felt like it would be enough to have the @throws-tag and have people trust that it won't secretly be caught by the dispatching service. I'd also be fine with adding a test though.

manicki · 2016-10-12T15:17:44Z

Thanks for quick fixes!

mariushoch · 2016-10-12T16:26:21Z

src/Lookup/DispatchingEntityLookup.php

@@ -0,0 +1,92 @@
+<?php


I don't think this should be part of this PR, should it?

mariushoch · 2016-10-12T16:27:45Z

src/Lookup/DispatchingTermLookup.php

+	private function getLookupForEntityId( EntityId $entityId ) {
+		$repo = $entityId->getRepositoryName();
+		if ( !isset( $this->lookups[$repo] ) ) {
+			throw new UnknownForeignRepositoryException( $entityId );


First parameter to the constructor of UnknownForeignRepositoryException is string $repoName not an entity id.

Indeed, thanks!

mariushoch · 2016-10-12T16:28:37Z

Not sure what happened to this PR, but it seems there are to many commits and way to many changes in here.

jakobw · 2016-10-13T09:00:23Z

@mariushoch the PR looks like that because it's depending on @manicki's PR #150 and includes all of its commits.

jakobw · 2016-10-14T14:20:06Z

Rebased against #154 to avoid all the commits from #150 flying around.

brightbyte

Looks pretty good, some small requests

brightbyte · 2016-10-27T17:24:10Z

src/Lookup/DispatchingTermLookup.php

+		Assert::parameter(
+			!empty( $lookups ) && array_key_exists( '', $lookups ),
+			'$lookups',
+			'must must not be empty and must contain an empty-string key'


Why is that required? Do we rely on that anywhere?

Not really, but that would require changes to the error handling so that it can handle that special case in a meaningful way.
In general, I could see that you want to treat all lookups equally "remote"… but I don't think that matches up with what was done in DM, does it?

brightbyte · 2016-10-27T17:25:52Z

tests/unit/Lookup/DispatchingTermLookupTest.php

+		] );
+
+		$this->setExpectedException( UnknownForeignRepositoryException::class );
+		$lookup->$method( $item, $language );


I think for the negative test, this is ok.

brightbyte · 2016-10-27T17:27:27Z

tests/unit/Lookup/DispatchingTermLookupTest.php

+		];
+	}
+
+	public function testGetLabelFromLocalRepo() {


The dispatcher should not really know or care about local and foreign. It uses whatever prefix is in the ID - if the prefix is the empty string, it should just work like any other. That should be tested, but it doesn't need a separate function.

brightbyte · 2016-10-31T14:56:05Z

I'm confused about the Travis failures. It seems that it is using the old DataModel version for some test runs, but not for others... is there a way to re-trigger the test runs?

brightbyte · 2016-10-31T14:56:23Z

Except for the Travis failures, I think this is good to merge now.

jakobw · 2016-10-31T15:20:29Z

@brightbyte travis config is fixed now.

brightbyte · 2016-10-31T15:28:35Z

src/Lookup/DispatchingTermLookup.php

+		Assert::parameter( !empty( $lookups ), '$lookups', 'must must not be empty' );
+		Assert::parameterElementType( TermLookup::class, $lookups, '$lookups' );
+		Assert::parameterElementType( 'string', array_keys( $lookups ), 'array_keys( $lookups )' );
+		foreach ( array_keys( $lookups ) as $repositoryName ) {


should probably wait for #155, so we can get rid of this.

brightbyte · 2016-10-31T15:29:06Z

@jakobw do we have a ticket for this somewhere?

jakobw · 2016-10-31T15:30:20Z

@brightbyte Not yet, I'll create one.

JeroenDeDauw · 2016-10-31T23:41:53Z

tests/unit/Lookup/DispatchingTermLookupTest.php

+		];
+	}
+
+	public function testGetLabel() {


Not a very descriptive name

manicki · 2016-11-01T09:25:21Z

As said above, the code is looking pretty good. I am just having some problems now to see this service being used in WikibaseClient: https://phabricator.wikimedia.org/diffusion/EWBA/browse/master/client/includes/WikibaseClient.php
It seems to be that client mostly uses the BufferingTermLookup: https://phabricator.wikimedia.org/diffusion/EWBA/browse/master/lib/includes/Store/BufferingTermLookup.php
So I am now thinking do we want a separate buffering term lookup for each repository, or maybe rather a single buffer for multiple term lookups/term indexes? If the former is the case, we would probably rather need a dispatching buffering lookup service then?

I am not sure if this PR is a right place to discuss such issues, if not let's have it somewhere else.

brightbyte · 2016-11-01T17:53:18Z

As I said in https://phabricator.wikimedia.org/T148141#2760606, we indeed do need a DispatchingTermBuffer (which is also a dispatching TermLookup), but probably no "plain" DispatchingTermLookup. Sorry about that...

jakobw · 2016-11-07T11:32:13Z

Closed for the reasons mentioned in the comment above

jakobw force-pushed the dispatching-term-lookup branch from 93bf750 to 146ffdb Compare October 12, 2016 11:25

manicki requested changes Oct 12, 2016

View reviewed changes

manicki approved these changes Oct 12, 2016

View reviewed changes

mariushoch reviewed Oct 12, 2016

View reviewed changes

src/Lookup/DispatchingEntityLookup.php

@@ -0,0 +1,92 @@

<?php

Copy link

Member

mariushoch Oct 12, 2016

Choose a reason for hiding this comment

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

I don't think this should be part of this PR, should it?

mariushoch reviewed Oct 12, 2016

View reviewed changes

manicki mentioned this pull request Oct 14, 2016

Add UnknownForeignRepositoryException #154

Merged

jakobw force-pushed the dispatching-term-lookup branch from cca40ac to 94bb198 Compare October 14, 2016 14:17

brightbyte suggested changes Oct 27, 2016

View reviewed changes

jakobw added 7 commits October 31, 2016 13:25

Add DispatchingTermLookup

3a1cdae

Fix return types.

0078028

Improve tests.

c162348

Fix indentation in doc block.

8f8a219

Remove redundant parameter.

8a2dac5

Fix UnknownForeignRepositoryException parameter.

5e6577d

Use existing repo variable.

41ab122

jakobw force-pushed the dispatching-term-lookup branch from 94bb198 to 41ab122 Compare October 31, 2016 12:26

Fix dependencies.

b927186

manicki mentioned this pull request Oct 31, 2016

Add functions to assert a value is an allowable repository name, or an array indexed by repository names #155

Closed

Do not give the empty repo prefix special treatment.

5ee3a53

brightbyte approved these changes Oct 31, 2016

View reviewed changes

Fix DM version for travis.

4af4004

brightbyte suggested changes Oct 31, 2016

View reviewed changes

JeroenDeDauw reviewed Oct 31, 2016

View reviewed changes

tests/unit/Lookup/DispatchingTermLookupTest.php

];

}

public function testGetLabel() {

Copy link

Contributor

JeroenDeDauw Oct 31, 2016

Choose a reason for hiding this comment

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

Not a very descriptive name

jakobw closed this Nov 7, 2016

Ladsgroup deleted the dispatching-term-lookup branch January 8, 2020 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DispatchingTermLookup #151

Add DispatchingTermLookup #151

jakobw commented Oct 12, 2016 •

edited

Loading

manicki left a comment

manicki Oct 12, 2016

jakobw Oct 12, 2016

manicki Oct 12, 2016 •

edited

Loading

jakobw Oct 12, 2016

manicki Oct 12, 2016 •

edited

Loading

jakobw Oct 12, 2016

brightbyte Oct 27, 2016

jakobw commented Oct 12, 2016

manicki commented Oct 12, 2016

mariushoch Oct 12, 2016

mariushoch Oct 12, 2016 •

edited

Loading

jakobw Oct 13, 2016

mariushoch commented Oct 12, 2016

jakobw commented Oct 13, 2016

jakobw commented Oct 14, 2016

brightbyte left a comment

brightbyte Oct 27, 2016

mariushoch Oct 28, 2016

brightbyte Oct 27, 2016

brightbyte Oct 27, 2016

brightbyte commented Oct 31, 2016

brightbyte commented Oct 31, 2016

jakobw commented Oct 31, 2016

brightbyte Oct 31, 2016

brightbyte commented Oct 31, 2016

jakobw commented Oct 31, 2016

JeroenDeDauw Oct 31, 2016

manicki commented Nov 1, 2016

brightbyte commented Nov 1, 2016

jakobw commented Nov 7, 2016

Add DispatchingTermLookup #151

Add DispatchingTermLookup #151

Conversation

jakobw commented Oct 12, 2016 • edited Loading

manicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manicki Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manicki Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobw commented Oct 12, 2016

manicki commented Oct 12, 2016

Choose a reason for hiding this comment

mariushoch Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariushoch commented Oct 12, 2016

jakobw commented Oct 13, 2016

jakobw commented Oct 14, 2016

brightbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brightbyte commented Oct 31, 2016

brightbyte commented Oct 31, 2016

jakobw commented Oct 31, 2016

Choose a reason for hiding this comment

brightbyte commented Oct 31, 2016

jakobw commented Oct 31, 2016

Choose a reason for hiding this comment

manicki commented Nov 1, 2016

brightbyte commented Nov 1, 2016

jakobw commented Nov 7, 2016

jakobw commented Oct 12, 2016 •

edited

Loading

manicki Oct 12, 2016 •

edited

Loading

manicki Oct 12, 2016 •

edited

Loading

mariushoch Oct 12, 2016 •

edited

Loading