EntityId changes

List overview All Threads
Download

newer

older

Clarifying role of different...

Re: [Wikidata-tech] [Wikidata-l]...

Jeroen De Dauw

23 Aug 2013 23 Aug '13

12:36 a.m.

Hey, After discussion with Danwe and Denny I decided to rewrite EntityId. It was the highest risk class according to PHPUnit (290 CRAP) in the DataModel component, which really is bad considering this is one of our most basic classes, which a lot of other code binds itself to. The goal of rewriting it is getting rid of needless complexity, needless bindings and making non-problematic use of the class easier. The rewrite currently includes: * Dropping of id prefix configurability (as previously discussed) * Separation of the id class and the entity id DataValue (EntityId no longer derives from DataValue. A new EntityIdDataValue composites in a EntrityId) * The EntityId class now has as fields entity type (string) and id serialization (string) * New ItemId and PropertyId derivatives, which in the constructor only take the id serialization. The long term goal is to make EntityId abstract * Serialization format compatibility where required. The format of the DataValue will remain the same for now Comments on the work in progress are welcome: https://gerrit.wikimedia.org/r/#/c/80394/ It is possible this change will be done by tomorrow. More likely it'll only be merged on Monday or later. Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Attachments:

attachment.htm (text/html — 1.6 KB)

Show replies by date

Jeroen De Dauw

23 Aug 23 Aug

1:01 a.m.

Hey, Some important implications I forgot to mention: * No more global state is used * No more semi-dependency from DataModel on lib * getPrefixedId is no longer evil and thus not deprecated any more * EntityId and derivatives no longer expose the id format. A FooIdDerivative that has a md5 in it can be created if that'd make sense * One can type hint against ItemId or PropertyId, which are guaranteed to be valid ids for the correct entity type To help understand the usage changes, here are 2 examples: 1. When constructing an id programmatically: Old way: $itemId = new EntityId( 'item', 42 ); New way: $itemId = new ItemId( 'q42' ); 2. When deserializing an item id (for instance in an API module): Old way: $idParser = getParserSomehow(); $itemId = $idParser->parse( $serialization ); New way: $itemId = new ItemId( $serialization ); (Error handling omitted for readability, as its the same in both cases anyway.) Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Daniel Kinzler

11:54 a.m.

Am 23.08.2013 00:01, schrieb Jeroen De Dauw:

...

1. When constructing an id programmatically: Old way: $itemId = new EntityId( 'item', 42 ); New way: $itemId = new ItemId( 'q42' );

In the database, we (generally) have the numeric ID and entity type. How do we construct an ItemId from that? I suggest to have some static methods as pseudo-constructors to make this easy. Also, I'm afraid this change means that we'll start using the serialized form internally, because it's convenient. I still think that this is a bad idea, even if the prefix is no longer configurable. -- daniel

Jeroen De Dauw

5:19 p.m.

Hey,

...

In the database, we (generally) have the numeric ID and entity type. How

do we construct an ItemId from that? See the code ;) Serialization was already encapsulated, so only some local changes are needed for this. Users in general do not know about the db serialization format.

...

Also, I'm afraid this change means that we'll start using the serialized

form internally, because it's convenient. I still think that this is a bad idea, even if the prefix is no longer configurable. We did originally use the serialized form, and thus have many of those in the db. The code that handled this legacy format is now once again the main code handling the current one. (I'm rather amused by that.) This certainly is convenient, thus we need good reason to not go with it. Why do you think it is a bad idea? What problems will it cause? And why are we not seeing those problems already with the current legacy serializations? Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Daniel Kinzler

25 Aug 25 Aug

4:18 p.m.

Am 23.08.2013 16:19, schrieb Jeroen De Dauw:

...

Hey,

In the database, we (generally) have the numeric ID and entity type. How

The serialization format is an external, canonical representation. The database form is internal and may change. Isn't that the reason we also have two JSON representations for entities?

...

Also, I'm afraid this change means that we'll start using the serialized

As far as I know, the only place where we have the serialized form in a DB field is wb_changes.change_object_id. Everything else uses two separate fields. This is quite annoying, since it makes it impossible to join the changes table against one of the other wb tables.

...

This certainly is convenient, thus we need good reason to not go with it. Why do you think it is a bad idea? What problems will it cause? And why are we not seeing those problems already with the current legacy serializations?

The only problem we are seeing is caused by the inconsistency described above. I think it is generally a bad idea to tie the database schema to a specific serialization. Encoding multiple things into a single field in the database is generally smelly, even though it's useful in some cases. I don't see a huge problem with using the prefixed ID in the database - but considering that we use separate fields almost everywhere, we should stick to that approach. Also, configurable or not, the knowledge of the prefixes should be in a single piece of code, so it can be easily changed (and make configurable again). I really don't want to see code sticking a "q" to a number all over the place. -- daniel

Jeroen De Dauw

7:45 p.m.

Hey,

...

... database ... schema ... serialization ...

This change has zero impact on our database schema or on how we index things based on entity ids. That is an entirely different topic. In the database, we (generally) have the numeric ID and entity type. How do

...

we construct an ItemId from that?

See the code ;) Serialization was already encapsulated, so only some local changes are needed for this. Users in general do not know about the db serialization format.

The serialization format is an external, canonical representation. The database form is internal and may change. Isn't that the reason we also have two JSON representations for entities?

What are you asking or stating? Is your second question here related to your first? Are you simply trying to state some things by asking rhetorical questions? Something else entirely? Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Daniel Kinzler

26 Aug 26 Aug

12:52 p.m.

Hm, this discussion is getting off track, perhaps we should have it in person tomorrow. I'll try to give some replies below, but I'm getting the feeling we are talking past each other. Regardless of the below, I want to make sure that the knowledge about prefixes and the formatting of IDs in general is encapsulated in a way that allows us to easily change or extend it later. That code should be the only place where the question whether the prefixes are configurable is relevant. Essentially, I fear that it's a bad idea to hard code the assumption that prefixes are not configurable. This can be knowledge in a few pieces of code, but should not be an assumption throughout the code base. Am 25.08.2013 18:45, schrieb Jeroen De Dauw:

...

Hey,

... database ... schema ... serialization ...

This change has zero impact on our database schema or on how we index things based on entity ids. That is an entirely different topic.

To me it sounded like you suggested to use the prefixed form in the database too. But that's not my main point. My point was that if we use two separate field in the database, it's probably more sensible to use this form in PHP too, rather than to convert back and forth all the time.

...

See the code ;) Serialization was already encapsulated, so only some local changes are needed for this. Users in general do not know about the db serialization format.

The serialization format is an external, canonical representation. The database form is internal and may change. Isn't that the reason we also have two JSON representations for entities?

What are you asking or stating? Is your second question here related to your first? Are you simply trying to state some things by asking rhetorical questions? Something else entirely?

It was a rhetorical question making the point that a format that is good as an external, canonical form is not automatically the right choice for the internal representation. -- daniel

Jeroen De Dauw

25 Aug 25 Aug

4:17 a.m.

Hey, I've been working on making the needed changes in Wikibase [0]. (During this I also updated a lot of usages of deprecated methods, cleaned up some messes and fixed some issues uncovered by the now stricter validation in the domain model.) While this is probably mostly done, there are still some failing tests in lib. I'm however having difficulties running the lib tests, as PHPUnit is aborting at some unknown location with a 255 error. I'd be great if someone else could try running the tests with the new code and see if they get the same issue. And if not, send me a list of the remaining errors. If anyone has a suggestion on how to find where the 255 is coming from, that'd be even better. [0] https://gerrit.wikimedia.org/r/#/c/80506/ Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Tobi Gritschacher

26 Aug 26 Aug

3:15 p.m.

Hey, I ran the unit tests on the new code and I'm having the same issue, PHPUnit is just aborting when running --group wikibaseLib. But I figured out where most of the failures come from, so if you run --group WikibaseSerialization PHPUnit will not abort and you can see the reports of at least 8 failues.. cheers, tobi On Sun, Aug 25, 2013 at 3:17 AM, Jeroen De Dauw <jeroendedauw(a)gmail.com>wrote;wrote:

...

-- Tobi Gritschacher Software Developer - Wikidata - http://www.wikidata.org Imagine a world, in which every single human being can freely share in the sum of all knowledge. That‘s our commitment. Wikimedia Deutschland e.V. | Obentrautstraße 72 | 10963 Berlin Phone +49 (0)30 219 158 260 http://wikimedia.de <http://www.wikimedia.de/> Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e.V. Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer 27/681/51985.

Tobi Gritschacher

3:53 p.m.

Some more groups which will give you errors/failures: --group WikibaseEntityLookup --group SnakFormatterTest --group EntityLinkFinder --filter UpdateRepoOnMoveJobTest (this does not even have a single group, so I'm not sure that was ever executed..) --filter WikibaseDataTypeBuildersTest HTH, cheers, tobi On Mon, Aug 26, 2013 at 2:15 PM, Tobi Gritschacher < tobias.gritschacher(a)wikimedia.de> wrote:

...

Jeroen De Dauw

7:44 p.m.

Daniel Kinzler

10:46 p.m.

You can run phpunit.php with --debug-tests, and send the debug output to a file with something like $wgDebugLogGroups['PHPUnitCommand'] = "/tmp/tests.log"; This will give you a (very verbose) log of test executions, so you can find the one at which phpunit crashes. I wrote that feature exactly for situations like this, so use it :) -- daniel Am 26.08.2013 18:44, schrieb Jeroen De Dauw:

...

Hey Tobi, Thanks for listing those groups, this is very helpful. Lets hope the 255 goes away after the errors reported for those groups have been fixed :) Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 -- _______________________________________________ Wikidata-tech mailing list Wikidata-tech(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-tech

Jeroen De Dauw

11:10 p.m.

Hey, You can run phpunit.php with --debug-tests, and send the debug output to a

...

file with something like $wgDebugLogGroups['PHPUnitComm and'] = "/tmp/tests.log"; This will give you a (very verbose) log of test executions, so you can find the one at which phpunit crashes. I wrote that feature exactly for situations like this, so use it :)

Thanks - that's very useful! :) Now I know one of the 255 was caused by MockRepositoryTest::testGetSiteLinksForItem passing in a property id into a function expecting an item id. Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

Jeroen De Dauw

11:49 p.m.

Hey, I now got all the tests to pass. That means the EntityId rewrite is up for review https://gerrit.wikimedia.org/r/#/c/80394/ And the follow up in Wikibase is as well https://gerrit.wikimedia.org/r/#/c/80506/ Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --

3933

days inactive

3937

days old

wikidata-tech@lists.wikimedia.org

Manage subscription

13 comments

3 participants

tags (0)

participants (3)

Daniel Kinzler
Jeroen De Dauw
Tobi Gritschacher