Page MenuHomePhabricator

Make Wikibase type dispatching services more uniform
Open, Needs TriagePublic

Description

There are a lot of services within Wikibase that can have one implementation per entity type. Wikibase uses type dispatching services to delegate to the service corresponding to the object (an entity or an entity id) that is passed in as an argument. See e.g. TypeDispatchingEntityTitleStoreLookup.

Many of the services are very similar and repeat the same couple of lines of code, but very few of them share code. Only recently some of the shared functionality was extracted from services introduced within the federated properties hike. This extracted ServiceByTypeDispatcher only fit the few use cases that it was created for initially, but can likely be extended to cover many more. For some of the existing type dispatching services it is obvious what needs to be changed either in ServiceByTypeDispatcher or in the dispatching service itself to make the two fit together, others use completely different patterns.

These services are used all over the code base and they all do pretty much the same thing, but implemented in sometimes subtly, sometimes completely different ways. This makes them difficult to maintain. Chances are that they could be much more uniform.

A few patterns that emerged when looking through the list of existing type dispatching services:

  • dispatchers that themselves implement the interface of the services they delegate to, and create and cache services per entity type at the time one of the interface methods are called e.g. TypeDispatchingEntityTitleStoreLookup, TypeDispatchingArticleIdLookup, ...
  • factories that create the required service based on the entity type e.g. DispatchingEntityMetaTagsCreatorFactory, EntityLinkFormatterFactory, ...
  • registering different strategies to dispatch to, e.g. EntityPatcher, EntityDiffer, ...

The following is a list of dispatching services (based on their EntityTypeDefinitions constants) and whether they can be changed to also use the new type dispatcher:
Already use ServiceByTypeDispatcher

  • ARTICLE_ID_LOOKUP_CALLBACK
  • TITLE_TEXT_LOOKUP_CALLBACK
  • URL_LOOKUP_CALLBACK
  • EXISTENCE_CHECKER_CALLBACK
  • REDIRECT_CHECKER_CALLBACK

Can probably use ServiceByTypeDispatcher

Can probably use ServiceByTypeDispatcher once it supports arguments on the factory callbacks

  • ENTITY_TITLE_STORE_LOOKUP_FACTORY_CALLBACK
  • ENTITY_STORE_FACTORY_CALLBACK
  • ENTITY_REVISION_LOOKUP_FACTORY_CALLBACK
  • ENTITY_DIFF_VISUALIZER_CALLBACK (EntityDiffVisualizerFactory)
  • CHANGEOP_DESERIALIZER_CALLBACK (EntityChangeOpProvider)
  • LINK_FORMATTER_CALLBACK (EntityLinkFormatterFactory)

Can probably use ServiceByTypeDispatcher once it can be used without a default

  • VIEW_FACTORY_CALLBACK

Cannot use ServiceByTypeDispatcher without major changes

  • ENTITY_METADATA_ACCESSOR_CALLBACK
    • the way it's currently implemented, it must not cache the implementation because it depends on the language at construction time
    • can probably be made to use ServiceByTypeDispatcher by passing the language into the method instead
  • META_TAGS_CREATOR_CALLBACK
    • same as above
  • PREFETCHING_TERM_LOOKUP_CALLBACK
    • ByTypeDispatchingPrefetchingTermLookup dispatching via instances of PrefetchingTL instead of factory callbacks
  • SERIALIZER_FACTORY_CALLBACK
    • use isSerializerFor to determine the right serializer for entities
  • STORAGE_SERIALIZER_FACTORY_CALLBACK
    • same as above but can probably be changed into the usual pattern
  • DESERIALIZER_FACTORY_CALLBACK
    • use a similar pattern to SERIALIZER_FACTORY_CALLBACK
  • ENTITY_DIFFER_STRATEGY_BUILDER
    • same as above, but can likely be changed
  • ENTITY_PATCHER_STRATEGY_BUILDER
    • samve as above
  • ENTITY_SEARCH_CALLBACK
    • similarly to ENTITY_METADATA_ACCESSOR_CALLBACK it is constructed in a way that does not allow for it to be cached
  • ENTITY_ID_HTML_LINK_FORMATTER_CALLBACK
    • are built all at once and passed into DispatchingEntityIdHtmlLinkFormatter as instances instead of callbacks but that's probably unnecessary
  • ENTITY_REFERENCE_EXTRACTOR_CALLBACK
    • has some extra default behavior in the dispatcher, could be converted

Unclear

  • CONTENT_HANDLER_FACTORY_CALLBACK
    • ContentHandlerFactory could possible use ServiceByTypeDispatcher
  • RDF_BUILDER_FACTORY_CALLBACK
    • all created at once in EntityRdfBuilderFactory - not sure if by design
  • SEARCH_FIELD_DEFINITIONS
    • the way it's used makes me question whether it's useful at all

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 599056 had a related patch set uploaded (by Jakob; owner: Jakob):
[mediawiki/extensions/Wikibase@master] ByTypeDispatchingEntityIdLookup: use ServiceByTypeDispatcher

https://gerrit.wikimedia.org/r/599056

Change 599286 had a related patch set uploaded (by Jakob; owner: Jakob):
[mediawiki/extensions/Wikibase@master] Use ServiceByTypeDispatcher in TypeDispatchingEntityTitleStoreLookup

https://gerrit.wikimedia.org/r/599286

Change 599286 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] TypeDispatchingEntityTitleStoreLookup: use ServiceByTypeDispatcher

https://gerrit.wikimedia.org/r/599286

Change 599056 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] ByTypeDispatchingEntityIdLookup: use ServiceByTypeDispatcher

https://gerrit.wikimedia.org/r/599056

@Jakob_WMDE could you update this ticket with what you see still needs to happen? (If anything) Or perhaps those bits live in other tickets?