Sitemap Events: registration response buggy

[maggu2810]

If a new resource is created the response should use status code 201 and the HTTP response headers should contain the location of the created resource.
See e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201

The current implementation in the SitemapResource looks like

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

Lines 288 to 311 in d23fb59

	/**
	* Creates a subscription for the stream of sitemap events.
	*
	* @return a subscription id
	*/
	@POST
	@Path(SEGMENT_EVENTS + "/subscribe")
	@ApiOperation(value = "Creates a sitemap event subscription.")
	@ApiResponses(value = { @ApiResponse(code = 201, message = "Subscription created."),
	@ApiResponse(code = 503, message = "Subscriptions limit reached.") })
	public Object createEventSubscription() {
	String subscriptionId = subscriptions.createSubscription(this);
	if (subscriptionId == null) {
	return JSONResponse.createResponse(Status.SERVICE_UNAVAILABLE, null,
	"Max number of subscriptions is reached.");
	}
	final EventOutput eventOutput = new SitemapEventOutput(subscriptions, subscriptionId);
	broadcaster.add(eventOutput);
	eventOutputs.put(subscriptionId, eventOutput);
	URI uri = uriInfo.getBaseUriBuilder().path(PATH_SITEMAPS).path(SEGMENT_EVENTS).path(subscriptionId).build();
	logger.debug("Client from IP {} requested new subscription => got id {}.", request.getRemoteAddr(),
	subscriptionId);
	return Response.created(uri);
	}

Response.created(uri); is already and it will create a status code 201 and the URI as location header...

BUT Response.created(uri); does not create a Response it creates a ResponseBuilder.
The call to build() is missing.
Other methods in this class already uses the correct construct return Response.ok(responseObject).build();

As you return a ResponseBuilder object instead of Response the resulting response looks very different.

• The status code will be 200.
• The location header is missing.
• The ResponseBuilder object is (as every object that is not a Response) deserialized by the Gson as a JSON string.
• ResponseBuilder is an interface so the JSON string looks like the current response builder implementation that is used (and could change if another implementation or an updated implementation will be used).

The solution would be to add the missing build() method call.

It seems the clients just eat what they get and needs to be fixed, too.
For example that way (I fixed the Classic UI Basic UI already):

https://github.com/openhab/openhab-webui/compare/master...maggu2810:jax-rs-whiteboard?expand=1

Current reply:

HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 360
Server: Jetty(9.4.18.v20190429)

{"status":"CREATED","context":{"headers":{"Location":["http://localhost:8080/rest/sitemaps/events/a0371b51-c7b3-42e4-a1cc-71b7fb0933f4"]},"committingOutputStream":{"bufferSize":0,"directWrite":true,"isCommitted":false,"isClosed":false},"entityAnnotations":[],"entityStream":{"bufferSize":0,"directWrite":true,"isCommitted":false,"isClosed":false}}}

Should be:

HTTP/1.1 201 Created
Date: Fri, 15 Nov 2019 08:57:20 GMT
Location: http://localhost:8080/rest/sitemaps/events/a0371b51-c7b3-42e4-a1cc-71b7fb0933f4
Content-Length: 0
Server: Jetty(9.4.11.v20180605)

[lolodomo]

Basic UI, not Classic UI.
Other UIs like Android app will certainly be impacted.

[maggu2810]

To sync the changes the clients could be changed first accept the old reply and the new reply.

For example:

• if status code is 200 and the entity is not empty, parse the JSON content and try to extract the location
• if status code is 201 try to use the location header
• else failure

If all clients has been updated the Sitemap Events endpoint can be fixed.

[maggu2810]

What's the plan here?

I see two options:

1. Fix the UIs, so the core component can be fixed.
2. Create a DTO that serialization results into the same response as before and use this in the response entity. That way, you still use the old (but IMHO non correct response) but be independent of the specific ResponseBuilder implementation.

[J-N-K]

@openhab/webui-maintainers @openhab/android-maintainers @openhab/ios-maintainers Can you please comment here? I would prefer to correct the response of SitemapResource.

[florian-h05]

I would prefer to correct the response of SitemapResource.

I agree.
I am sure we can coordinate the required changes for the WebUIs with the core PR, and since the WebUIs are not shipped separately from Core, there shouldn‘t be compatibility problems.
(Unfortunately I am very busy for the next 4 weeks and I am not sure whether I find the time to contribute the MainUI part.)

The more critical part of this IMO are the mobile apps, since they are shipped independently from openHAB Distro. They need to be compatible to both „ways“.

[maniac103]

The more critical part of this IMO are the mobile apps, since they are shipped independently from openHAB Distro

For the Android app, it should be easy to implement both (-> openhab/openhab-android#3335); the most interesting part is getting it tested before the code is merged.

[lolodomo]

For BasicUI, it should be possible to implement both.

[florian-h05]

For MainUI, no change is needed since MainUI is not using the Sitemap events (/sitemaps/events), but instead the normal events (/events).

[mueller-ma]

the most interesting part is getting it tested before the code is merged.

Merge the code on the server first and when it's in a snapshot release, test the Android app. The app can be released faster than the server.

[lolodomo]

First à PR must be submitted in core.
Another PR must be submitted for Basic UI to support new response.
Merge of Basic UI PR should be done the same day as the core PR.

[J-N-K]

@openhab/ios-maintainers Can you comment here?

[lolodomo]

I updated my previous message, there is no need to support old and new response in Basic UI.
We just need to sync change in core framework and Basic UI.