Page MenuHomePhabricator

Remove EntityDeserializer
Closed, ResolvedPublic

Description

This class contains a bunch of deprecated methods and OCP violations. We should rather have deserializers for StatementListProvider and similar interfaces.

Related Objects

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
DuplicateNone
InvalidLydia_Pintscher
OpenNone
OpenNone
StalledNone
OpenNone
ResolvedAddshore
Resolvedthiemowmde
ResolvedAddshore
ResolvedBene

Event Timeline

Bene created this task.Apr 30 2015, 8:58 PM
Bene raised the priority of this task from to Needs Triage.
Bene updated the task description. (Show Details)
Bene added a subscriber: Bene.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 30 2015, 8:58 PM
Bene claimed this task.May 1 2015, 9:53 AM
Bene set Security to None.
Bene moved this task from Backlog to Doing on the Wikidata-Sprint-2015-04-21 board.

You are right about there being OCP violations that need fixing.

Currently there is this EntityDeserializer, with ItemDeserializer and PropertyDeserializer derivatives. That is very similar to what we had in DataModel with the EntityDiffer code. And I think the solution that should be taken here is the same. Keep having an EntityDeserializer (since people will still want to be able to deserialize entities that are not all of a known same type), but have it delegate to specific deserializers per entity type (ie ItemDeserializer and PropertyDeserializer). This way you avoid the OCP violations and get rid of the code reuse by inheritance.

You can try going with the "instanceof StatementListProvider" approach, though I think it's problematic for serialization and not possible for deserialization. For serialization, the addition of a new entity type that does not have a Fingerprint is not going to break anything. Likewise, if it has a new type of attribute, nothing will break (in this code). Instead, you'll just end up getting a partial serialization of the entity, with no warning or hint about that.

Bene added a comment.EditedMay 1 2015, 11:14 AM

We already have the DispatchableDeserializer and DispatchableSerializer which do exactly that: You can pass an Entity object and it detects the requried Serializers/Deserializers for that type. We can thus just drop the EntityDeserializer imo as it isn't even exposed publicly in the factories. DeserializerFactory::newEntityDeserializer actually returns a DispatchableDeserializer.

So EntityDeserializer is indeed only used for code sharing and should be removed completely.

JeroenDeDauw added a comment.EditedMay 1 2015, 11:30 AM

You are correct - since we have DispatchableSerializer, we do not need to have an EntityDeserializer class to be able to have an "entity deserializer". I did not realize this when writing my previous comment.

JanZerebecki closed this task as Resolved.May 5 2015, 1:28 PM