[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r449822556



##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/ActivityRecordSet.java
##########
@@ -56,6 +56,24 @@
      *      If an error occurs while retrieving the records within this set.
      */
     Collection<RecordType> asCollection() throws GuacamoleException;
+    
+    /**
+     * Returns records within this set for the item having the specified
+     * identifier as a standard Collection.
+     *
+     * @param identifier
+     *     The identifier of the underlying item that the collection
+     *     should be limited to.
+     * 
+     * @return
+     *     A collection containing records within this set for the specified
+     *     identifier.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while retrieving the records within this set.
+     */
+    Collection<RecordType> asCollection(String identifier)
+            throws GuacamoleException;

Review comment:
       A couple things:
   
   * To continue maintaining backward compatibility, default implementations need to be provided for new functions added to existing interfaces within guacamole-ext.
   * As defined, this will be ambiguous as to which type of object the identifier refers to. The records within the collection may point to multiple types of objects. For example, a connection record points to both a user and a connection, and may also point to a sharing profile.
   * The general pattern for `ActivityRecordSet` is a set of functions which can be layered on top of each other by virtue of those functions each returning `ActivityRecordSet`, similar to the various "builder" objects we see in JAX-RS. Keeping with that pattern, I think this would be best implemented not as new version of `asCollection()`, but as some other function that allows the set to be narrowed to records containing a particular object having a particular identifier.
   
      I could imagine a new version of `contains()` which accepts an `Identifiable` rather than a `String`, or perhaps multiple `contains()` for `Connection`, `User`, `SharingProfile`, etc.

##########
File path: guacamole/src/main/java/org/apache/guacamole/rest/history/HistoryResource.java
##########
@@ -86,8 +93,9 @@ public HistoryResource(UserContext userContext) {
      *     If an error occurs while retrieving the connection history.
      */
     @GET
-    @Path("connections")
+    @Path("connections{p:/?}{identifier : ([0-9]+)?}")
     public List<APIConnectionRecord> getConnectionHistory(
+            @PathParam("identifier") String identifier,

Review comment:
       Beware that not all identifiers are numeric. That said, I don't think this is the best place to add this. I think it would be better to continue using the existing per-connection endpoint for connection-specific history searches, but attempt to refactor `HistoryResource` such that it can be used both at the global level and per-connection / per-user.




----------------------------------------------------------------
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:
users@infra.apache.org