mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r513147038
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java
##########
@@ -150,18 +149,16 @@ public ConnectionResource(@Assisted UserContext
userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the connection history.
*/
- @GET
@Path("history")
- public List<APIConnectionRecord> getConnectionHistory()
+ public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
- // Retrieve the requested connection's history
- List<APIConnectionRecord> apiRecords = new
ArrayList<APIConnectionRecord>();
- for (ConnectionRecord record : connection.getHistory())
- apiRecords.add(new APIConnectionRecord(record));
-
- // Return the converted history
- return apiRecords;
+ try {
+ return new
ConnectionHistoryResource(connection.getConnectionHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>());
+ }
Review comment:
Unfortunately, this will break backward compatibility with extensions
written for 1.2.0 and older, as those extensions will not implement
`getConnectionHistory()`. Though they may implement `getHistory()`, that will
not be called here, and older extensions will instead behave as if history is
not supported.
I think I see what you're aiming at here, though, and I hadn't considered
the `getHistory()` wrapping `getConnectionHistory()` approach.
If:
* The older `getHistory()` defaults to calling the newer `getUserHistory()`
/ `getConnectionHistory()`
* The newer `getUserHistory()` / `getConnectionHistory()` defaults to
throwing `GuacamoleUnsupportedException`
* The behavior of newer `getUserHistory()` / `getConnectionHistory()` is
specifically documented as throwing `GuacamoleUnsupportedException` if
unimplemented, and that the older, deprecated `getHistory()` function should be
tried instead if this occurs.
Then the webapp could handle things by:
1. Invoking the newer function.
2. If that fails with `GuacamoleUnsupportedException`, assume older API and
invoke the older function.
3. If _that_ fails, assume no history support and use empty set.
And decorating extensions would also behave correctly, as invocations of the
older `getHistory()` would function as expected, while newer invocations of
`getUserHistory()` / `getConnectionHistory()` would know to handle
`GuacamoleUnsupportedException`.
Am I missing something? Does this solve everything?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
