Remove API NotificationFilter PreferenceProvider

Hi everyone,

during the analysis of a fix for Loading... I realized that there was a design problem in xwiki-platform-notification-filters modules: right now we have NotificationFilterPreferenceProvider which is a public interface defining a component role, which should never have been public IMO and which is even completely useless in 16.x.

So to give a bit of history: notification filter preferences used to be stored in WatchlistClass xobjects, and then this mechanism has been changed to store the filter preferences in DB. However during a long time (from 10.11.9 to 16.0.0 according to Loading...) there was no migration provided for WatchlistClass xobjects, and instead there was a bridge for the API to retrieve the info either in DB or from xobjects. So this NotificationFilterPreferenceProvider was the common interface used by both the deprecated bridge and the new API that retrieve preferences from DB.

I’ve no idea why it’s been made public, but for me this was a mistake: first, I don’t see a reason why an extension would want a custom storage of the filter preferences, and second right now in DefaultNotificationFilterPreferenceManager we were calling each time all providers to perform the various API calls, which actually led to Loading.... With a new implementation of a provider we would have hit this bug probably way before.

Now, besides being public, this component role is now even completely useless: I did performed the migration of the watchlistclass xobjects as part of Loading..., so in XWiki Standard we now have a single implementation of this interface. And removing it simplify a lot the code as it removes an indirection and make all calls of the API far more easy to understand.

Note that I did perform a check in xwiki-contrib and there’s 0 hit for NotificationFilterPreferenceProvider, also it’s never been documented in our Notification API extension page.
Now I opened a draft PR containing the actual removal of this class with all changes, just to see what it would impact (built with skipping revapi and jacoco): XWIKI-22211: Deprecate NotificationFilterPreferenceProvider by surli · Pull Request #2882 · xwiki/xwiki-platform · GitHub

So I’m opening a vote for actually breaking the API to entirely remove NotificationFilterPreferenceProvider in 16.x. This vote is opened for 2 weeks, until the 27th of february.

Here’s my +1.

To be perfectly clear: it’s completely possible to deprecate only the API and keep it in our code, but I do think it’s maintaining something for no value as I strongly believe it’s never meant to be public.

+1

Hello.

As far as I remember (not having coded anything in regards to notifications for 6 years does not help!),
it has been done this way so we could implement reading the watchlist preferences with a bridge without having to migrate the data - which was important during the period when notifications were unstable and disabled by default.

The interface was public because I wanted extensions to be able to bring their own types of notifications, and be able to store their preferences in a custom way if needed.

It’s probably an advanced use-case that we will never meet, so if you think it’s better to remove it as a public API, I’m OK.

1 Like

Sounds like I forgot to close that vote: so with 3 +1 the vote is accepted and I actually performed the changes recently as part of Loading...