Allow subscriptions for complete sitemaps (not limited to a single page)

[TAKeanice]

The changes in this PR allow to leave out the pageId in a request for updates from a subscription. The motivation for this was discussed in #3650

Leaving out the pageId in a subscription request makes the event API send events for updates of all widgets within a sitemap. This is accomplished by walking the sitemap tree and collecting all widgets in it when the subscription is created. Previously, only the widgets on the given page were collected at that occasion.

In the issue above, performance issues were discussed. I do not know how to test for them, and would happily accept help with that matter.

[lolodomo]

I will have a look to your proposal. But as this is something we are not recommending at all, first can you confirm that this will not become the default behaviour for existing UIs ? And I would like something added in the javadoc explaining that this is a possible not a recommended usage.
Any official openHAB should not use that. This could lead to a massive network traffic between server and browser (SSE events).

[TAKeanice]

@lolodomo thank you for giving this PR some support. I split up the methods to create a sitemap-wide subscription and a normal subscription endpoint, and wrote comments on the ones that should not be used. There should be no possibility to unconsciously subscribe to the whole sitemap.

The things I did not change apparently contained some nullability issues with the pageId. I did not try to resolve them to not break legacy behaviour.

[TAKeanice]

@lolodomo the compliance with your request to make sure not the whole sitemap is listened to by default is now also ensured by a regression test. I don´t see any reason why this should not be merged.

[TAKeanice]

@lolodomo I know everyone is busy and stuff, but now that it´s spring and everything is getting back to life, would you consider reviving your interest in this PR, too?

[J-N-K]

I think you can simplify the code very much by allowig pageId to be null (and consider that as "whole sitemap"). You can remove nearly all added methods in this case.

[TAKeanice]

@J-N-K I the request by @lolodomo was specifically not to do that, since it could accidentally be used in some official implementation. My original implementation was like you suggested. If you want, I revert my changes to get that version back. Expecting your final decision :)

[J-N-K]

I can't find that request in this PR, can you point me to the comment?

[TAKeanice]

the comment

#3652 (comment)_

Have I misinterpreted the request to make sure it does not get used accidentally?

[J-N-K]

IMO yes, my understanding is that a comment should state "whole sitemap is not recommended" (maybe also in the API documentation, not only in JavaDoc) and that a subscription with page should never result in a subscription to the whole sitemap.

But maybe @lolodomo can comment on this.

Edit: This only relevant for the SitemapResource, it does is not relevant for internal code. If omitting pageId for the "whole sitemap subscription" would be a good idea is debatable, but as far as I can see that is not possible in any case, because it would collide with other endpoints. Accidentally adding * or wholeSitemap will not happen.

[TAKeanice]

I will make some simplifications to the private methods, that should reduce code duplication.

[lolodomo]

IMO yes, my understanding is that a comment should state "whole sitemap is not recommended"

Yes, exactly ,that was my request.
Also as a consequence that it should not become the default behaviour.

[TAKeanice]

The reason I split the methods is that the method getSitemapEvents

openhab-core/bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java

Lines 346 to 349 in 81f2bd9

	public void getSitemapEvents(@Context final SseEventSink sseEventSink, @Context final HttpServletResponse response,
	@PathParam("subscriptionid") @Parameter(description = "subscription id") String subscriptionId,
	@QueryParam("sitemap") @Parameter(description = "sitemap name") @Nullable String sitemapname,
	@QueryParam("pageid") @Parameter(description = "page id") @Nullable String pageId) {

allows null as pageId. If I unsplit the whole implementation by allowing null pageIds throughout, omitting the pageId when calling that method would result in accidentally using sitemap-wide subscriptions. That was the nullability issue I referred to here: #3652 (comment)

[J-N-K]

There is an issue with the tests, can you fix that? I'll nmerge afterwards. Thanks for your patience.

[lolodomo]

I have not checked but I will not be surprised that tests are failing due to the change I am commenting in my last message.

[TAKeanice]

@J-N-K and @lolodomo I think the tests fail due to my code deduplication, I'll fix that today. It was a pleasure working on finalizing this the last couple of days, thank you for that!

[TAKeanice]

The test fails because the subscriptionService is just a mock, I need to properly instantiate it and mock its itemUIRegistry in the test.

[J-N-K]

You can also keep the mocked subscriptionService and return another mock, depending on the called method (using when).

[TAKeanice]

You can also keep the mocked subscriptionService

but that would defeat the purpose of the test, since an important part of the logic resides in collectWidgets

[J-N-K]

I didn't look at the test, it was more a general comment.

[J-N-K]

Thanks!

[TAKeanice]

@J-N-K the test issues have been resolved.

[lolodomo]

By renaming method setPageId, you broke UIs using it :(
Basic UI is no more compiling, I am going to fix that.

[TAKeanice]

I am going to fix that.

Thank you very much, I wasn't aware that the method was public api. Maybe I should re-add the legacy method but deprecate it and make it just call the new one, just in case other (plugin) implementations use the method, too?

[lolodomo]

I searched in GitHub (including iOS and Android apps) and apparently Basic UI was the unique app calling this method.

[lolodomo]

But you could fix javadoc still referencing it

openhab-core/bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/SitemapSubscriptionService.java

Line 63 in 1a89b9e

* For this to work correctly, the subscriber needs to make sure that setPageId is called whenever it switches to a new