Fix REST API response of methods with String return type

[ivanayov]

Using the REST API from the GUI used to return "error": "no response from server" and return code 0 for methods that return String.
The problem was that the received response was not in json format, while the expected response content type is application/json.

[geomacy]

Can you add some explanation of what the problem is and how this change fixes it?

[geomacy]

why the qs? it's not actually a valid parameter on Content-Type, it's some sort of Apache webserver hack.

[ivanayov]

retest this please

[geomacy]

I got the following:

Tests run: 181, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 21.038 sec <<< FAILURE! - in TestSuite
testGetConfigValue(org.apache.brooklyn.rest.resources.PolicyResourceTest)  Time elapsed: 0.016 sec  <<< FAILURE!
java.lang.AssertionError: expected [newval] but found ["newval"]
	at org.apache.brooklyn.rest.resources.PolicyResourceTest.testGetConfigValue(PolicyResourceTest.java:141)


Results :

Failed tests: 
  PolicyResourceTest.testGetConfigValue:141 expected [newval] but found ["newval"]

looks like it is caused by the changed response format.

[ahgittin]

@iyovcheva I think this breaks backwards compatibility so we need to be clear where and what it is changing and what it might impact.

Agree it makes sense to be clear are we returning JSON (e.g. wrap string in quotes) or PLAIN.

However we may also have the problem that in some places a string we return will itself be wrapped. And in other places the client might want an unwrapped string (ie not JSON).

[ivanayov]

Thanks @geomacy , I updated the test. "retest this please" triggers a rebuild.

[ivanayov]

@ahgittin This is a list of the GET requests the change impacts:

/activities/{task}/stream/{streamId}
/applications/{application}/entities/{entity}/policies/{policy}/config
/applications/{application}/entities/{entity}/policies/{policy}/config/current-state
/server/user

The one that was broken in the UI was /server/user. In this case we always expect a single username, which makes the change not harmful (if you have an example that shows the opposite, it's well appreciated). The other option here is to change it to text/plain.

For /activities/{task}/stream/{streamId} text/plain is always the logical option and we ensure that json will never be expected in this case.

The controversial options IMO are the two policies requests. In this case the returned string is already in json, so we may not need any changes.

What do you think about:

• Keep the change for /activities/{task}/stream/{streamId}
• Change /server/user to text/plain
• Remove policies changes, as this was working properly before and can be risky now?

[geomacy]

👍 +1 to your suggestions @iyovcheva

If I understand correctly, the changes to add @Produces(MediaType.TEXT_PLAIN don't change the responses, but do ensure that the Content-Type is correctly declared; and removing the policies changes avoids backward compatibility issues.

[neykov]

Suggest we keep the changes for PolicyConfigApi.get(...), making it work analogously to EntityConfigApi.get(...). The value there can be any object, so it needs to be encoded as JSON.

@ahgittin Backwards compatibility is debatable. If we used to return plain strings as application/json then obviously this was broken and clients should've complained. This probably got covered by the libraries used to fetch the resource, suggesting that making it proper json will actually improve compatibility with existing clients.

[geomacy]

per @neykov's suggestion, merging this with the policy changes included.

[neykov]

This is not needed, fixes only needed for methods returning String. This one will get converted to json by the framework.