mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r514610644
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/user/UserResource.java
##########
@@ -110,18 +116,29 @@ public UserResource(@Assisted UserContext userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the user history.
*/
- @GET
+ @SuppressWarnings("deprecation")
@Path("history")
- public List<APIActivityRecord> getUserHistory()
+ public UserHistoryResource getUserHistory()
throws GuacamoleException {
- // Retrieve the requested user's history
- List<APIActivityRecord> apiRecords = new
ArrayList<APIActivityRecord>();
- for (ActivityRecord record : user.getHistory())
- apiRecords.add(new APIActivityRecord(record));
-
- // Return the converted history
- return apiRecords;
+ // First try to retrieve history using the current getUserHistory()
method.
+ try {
+ return new UserHistoryResource(user.getUserHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ logger.debug("Unsupported exception when calling
getUserHistory().", e);
+ }
+
+ // Fall back to deprecated getHistory() method.
+ try {
+ return new UserHistoryResource(new
SimpleActivityRecordSet<>(user.getHistory()));
+ }
+ catch (GuacamoleUnsupportedException e) {
+ logger.debug("Unsupport expection when calling getHistory().", e);
Review comment:
Expection?
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/user/UserResource.java
##########
@@ -110,18 +116,29 @@ public UserResource(@Assisted UserContext userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the user history.
*/
- @GET
+ @SuppressWarnings("deprecation")
@Path("history")
- public List<APIActivityRecord> getUserHistory()
+ public UserHistoryResource getUserHistory()
throws GuacamoleException {
- // Retrieve the requested user's history
- List<APIActivityRecord> apiRecords = new
ArrayList<APIActivityRecord>();
- for (ActivityRecord record : user.getHistory())
- apiRecords.add(new APIActivityRecord(record));
-
- // Return the converted history
- return apiRecords;
+ // First try to retrieve history using the current getUserHistory()
method.
+ try {
+ return new UserHistoryResource(user.getUserHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ logger.debug("Unsupported exception when calling
getUserHistory().", e);
Review comment:
This sounds to me like it's the exception itself that's unsupported. I
think it would be better to note that `getUserHistory()` is reported as
unsupported, and that `getHistory()` will be tried instead.
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##########
@@ -93,17 +96,50 @@
* of this user, including any active sessions. ActivityRecords
* in this list will be sorted in descending order of end time (active
* sessions are first), and then in descending order of start time
- * (newer sessions are first).
+ * (newer sessions are first). If user login history is not implemented
+ * this method should throw GuacamoleUnsupportedException.
*
+ * @deprecated
+ * This function is deprecated in favor of {@link getUserHistory},
which
+ * returns the login history as an ActivityRecordSet which supports
+ * various sort and filter functions. While this continues to be
defined
+ * for API compatibility, new implementation should avoid this function
+ * and use getUserHistory(), instead.
+ *
* @return
* A list of ActivityRecords representing the login history of this
* User.
*
* @throws GuacamoleException
- * If an error occurs while reading the history of this user, or if
- * permission is denied.
+ * If history tracking is not implement, if an error occurs while
Review comment:
implemented*
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java
##########
@@ -150,18 +156,29 @@ public ConnectionResource(@Assisted UserContext
userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the connection history.
*/
- @GET
+ @SuppressWarnings("deprecation")
@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 the current getConnectionHistory() method, first, for
connection history.
+ try {
+ return new
ConnectionHistoryResource(connection.getConnectionHistory());
+ }
+ catch (GuacamoleUnsupportedException e) {
+ logger.debug("Unsupported exception retrieving connection
history.", e);
+ }
+
+ // Fall back to the deprecated getHistory() method.
+ try {
+ return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>(connection.getHistory()));
+ }
+ catch (GuacamoleUnsupportedException e) {
+ logger.debug("Unsupported exception retrieving connection
history.", e);
+ }
Review comment:
Same here - I think it would be best to rephrase these log messages such
that it's clear what's not supported and what's being done as a result.
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/ActivityRecordSet.java
##########
@@ -19,7 +19,9 @@
package org.apache.guacamole.net.auth;
+import java.util.ArrayList;
import java.util.Collection;
+import java.util.List;
Review comment:
Same here - unless the build is currently failing, it's unlikely new
`import` statements are needed without other code changes within the same file
requiring them.
##########
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleUser.java
##########
@@ -24,6 +24,8 @@
import java.util.Set;
import org.apache.guacamole.GuacamoleException;
import org.apache.guacamole.net.auth.AbstractUser;
+import org.apache.guacamole.net.auth.ActivityRecord;
+import org.apache.guacamole.net.auth.ActivityRecordSet;
Review comment:
FYI - This file now shows as changed only by the addition of these
`import` statements. Unless things didn't compile before, these are likely
unnecessary.
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##########
@@ -93,17 +96,50 @@
* of this user, including any active sessions. ActivityRecords
* in this list will be sorted in descending order of end time (active
* sessions are first), and then in descending order of start time
- * (newer sessions are first).
+ * (newer sessions are first). If user login history is not implemented
+ * this method should throw GuacamoleUnsupportedException.
*
+ * @deprecated
+ * This function is deprecated in favor of {@link getUserHistory},
which
+ * returns the login history as an ActivityRecordSet which supports
+ * various sort and filter functions. While this continues to be
defined
+ * for API compatibility, new implementation should avoid this function
+ * and use getUserHistory(), instead.
+ *
* @return
* A list of ActivityRecords representing the login history of this
* User.
*
* @throws GuacamoleException
- * If an error occurs while reading the history of this user, or if
- * permission is denied.
+ * If history tracking is not implement, if an error occurs while
+ * reading the history of this user, or if permission is denied.
*/
- List<? extends ActivityRecord> getHistory() throws GuacamoleException;
+ @Deprecated
+ default List<? extends ActivityRecord> getHistory() throws
GuacamoleException {
+ return Collections.unmodifiableList(new
ArrayList<>(getUserHistory().asCollection()));
+ }
+
+ /**
+ * Returns an ActivityRecordSet containing ActivityRecords representing
+ * the login history for this user, including any active sessions.
+ * ActivityRecords in this list will be sorted in descending order of end
+ * time (active sessions are first), and then in descending order of start
+ * time (newer sessions are first). If login history tracking is not
+ * implemented, or is only implemented using the deprecated {@link
getHistory}
+ * method, this method should throw GuacamoleUnsupportedException.
+ *
+ * @return
+ * An ActivityRecordSet containing ActivityRecords representing the
+ * login history for this user.
+ *
+ * @throws GuacamoleException
+ * If history tracking is not implement, if an error occurs while
Review comment:
implemented*
----------------------------------------------------------------
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]
