[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
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 getUserHistory()
+public UserHistoryResource getUserHistory()
throws GuacamoleException {
-// Retrieve the requested user's history
-List apiRecords = new
ArrayList();
-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 getUserHistory()
+public UserHistoryResource getUserHistory()
throws GuacamoleException {
-// Retrieve the requested user's history
-List apiRecords = new
ArrayList();
-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 getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-for (ConnectionRecord record : connection.getHistory())
-apiRecords.add(new APIConnectionRecord(record));
-
-// Retu
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r514562005
##
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionService.java
##
@@ -411,28 +411,16 @@ protected ConnectionRecord
getObjectInstance(ConnectionRecordModel model) {
ModeledConnection connection) throws GuacamoleException {
String identifier = connection.getIdentifier();
-
-// Retrieve history only if READ permission is granted
-if (hasObjectPermission(user, identifier, ObjectPermission.Type.READ))
{
Review comment:
Right - `getActiveConnections()` is required to only return connections
visible to the current 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:
[email protected]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r514036834
##
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/ActivityRecordSet.java
##
@@ -56,6 +58,19 @@
* If an error occurs while retrieving the records within this set.
*/
Collection asCollection() throws GuacamoleException;
+
+/**
+ * Returns all records within this set as a list
+ *
+ * @return
+ * A list containing all records in this set.
+ *
+ * @throws GuacamoleException
+ * If an error occurs while retrieving the records within this set.
+ */
+default List asList() throws GuacamoleException {
+return new ArrayList<>(asCollection());
+}
Review comment:
I'm not sure about the benefit of an `asList()` function outside our own
need for a convenient way to implement the now-deprecated functions. Perhaps
this shouldn't be part of the API? Will the benefit of this function survive
the eventual removal of the deprecated functions?
##
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionService.java
##
@@ -411,28 +411,16 @@ protected ConnectionRecord
getObjectInstance(ConnectionRecordModel model) {
ModeledConnection connection) throws GuacamoleException {
String identifier = connection.getIdentifier();
-
-// Retrieve history only if READ permission is granted
-if (hasObjectPermission(user, identifier, ObjectPermission.Type.READ))
{
Review comment:
Can you clarify why the explicit check for "READ" permission is no
longer needed?
##
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/resources/org/apache/guacamole/auth/jdbc/user/UserRecordMapper.xml
##
@@ -105,25 +105,32 @@
FROM guacamole_user_history
-
-(
-
-guacamole_user_history.user_id IN (
-SELECT user_id
-FROM guacamole_user
-JOIN guacamole_entity ON guacamole_user.entity_id =
guacamole_entity.entity_id
-WHERE
-POSITION(#{term.term,jdbcType=VARCHAR} IN
guacamole_entity.name) > 0
-AND guacamole_entity.type = 'USER'),
-)
-
-
-OR start_date BETWEEN #{term.startDate,jdbcType=TIMESTAMP}
AND #{term.endDate,jdbcType=TIMESTAMP}
-
+
+
+
+guacamole_entity.name = #{username,jdbcType=VARCHAR}
Review comment:
I don't see where `guacamole_entity` is joined in here. Should this be
against `guacamole_user_history.username`?
##
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleActivityRecordSet.java
##
@@ -35,10 +36,30 @@
public class SimpleActivityRecordSet
implements ActivityRecordSet {
+private final List records;
+
+/**
+ * Create a new SimpleActivityRecordSet that contains an empty set of
+ * records.
+ */
+public SimpleActivityRecordSet() {
+records = Collections.emptyList();
+}
+
+/**
+ * Create a new SimpleActivityRecordSet that contains the provided records.
+ *
+ * @param records
+ * The records that this SimpleActivityRecordSet should contain.
+ */
+public SimpleActivityRecordSet(List records) {
+this.records = records;
Review comment:
If the provided `List` will back the resulting `SimpleActivityRecordSet`
(such that changes to the `List` affect the content of the set), that should be
noted here.
##
File path:
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleActivityRecordSet.java
##
@@ -35,10 +36,30 @@
public class SimpleActivityRecordSet
implements ActivityRecordSet {
+private final List records;
Review comment:
Please document.
##
File path:
guacamole/src/main/java/org/apache/guacamole/rest/user/UserResource.java
##
@@ -110,18 +112,26 @@ public UserResource(@Assisted UserContext userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the user history.
*/
-@GET
+@SuppressWarnings("deprecation")
@Path("history")
-public List getUserHistory()
+public UserHistoryResource getUserHistory()
throws GuacamoleException {
-// Retrieve the requested user's history
-List apiRecords = new
ArrayList();
-for (ActivityRecord record : user.getHistory())
-apiRecords.add(new APIActivityRecord(record));
-
-// Return the converted history
-return apiRecords;
+try {
+return new UserHistoryResource(user.getUser
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r514013516
##
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 getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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:
I agree that it would make sense to be consistent in the handling of
`GuacamoleUnsupportedException` and provide the same functionality there, as
well. It could make things easier for developers that may well use the same
code to drive both.
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r513950716
##
File path:
guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java
##
@@ -150,18 +153,26 @@ public ConnectionResource(@Assisted UserContext
userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the connection history.
*/
-@GET
+@SuppressWarnings("deprecation")
@Path("history")
-public List getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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) {
+try {
+List history = Collections.emptyList();
+for (ConnectionRecord record : connection.getHistory()) {
+history.add(record);
+}
+return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>(history));
+}
+catch (GuacamoleUnsupportedException ex) {
+return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>());
+}
+}
Review comment:
...unless the intent is that `SimpleActivityRecordSet` will be backed by
the provided `List`, in which case `List` **is** correct. This is
what is done with `SimpleDirectory`:
https://guacamole.apache.org/doc/guacamole-ext/org/apache/guacamole/net/auth/simple/SimpleDirectory.html#SimpleDirectory-java.util.Collection-
I would suggest that the type be `Collection`, though, for API's
sake.
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r513946820
##
File path:
guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java
##
@@ -150,18 +153,26 @@ public ConnectionResource(@Assisted UserContext
userContext,
* @throws GuacamoleException
* If an error occurs while retrieving the connection history.
*/
-@GET
+@SuppressWarnings("deprecation")
@Path("history")
-public List getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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) {
+try {
+List history = Collections.emptyList();
+for (ConnectionRecord record : connection.getHistory()) {
+history.add(record);
+}
+return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>(history));
+}
+catch (GuacamoleUnsupportedException ex) {
+return new ConnectionHistoryResource(new
SimpleActivityRecordSet<>());
+}
+}
Review comment:
Ah, I see what you're saying. It's a rather confusing aspect of Java
generics. Consider if this was possible:
```java
List a = new ArrayList<>();
List b = a; // This is completely OK
List c = b; // This shouldn't be allowed because ...
c.add(new Thing()); // ... this breaks the contract of List
```
Suddenly, that `List` that's expected to contain only fancy things contains
a non-fancy thing, breaking the contract of the type. Declaring something as
`List` is saying "a list of `Thing` or any subclass of `Thing`".
Declaring something as `List` is saying "a list of **an
unspecified subclass of `Thing`**".
For the approach here with `SimpleActivityRecordSet`:
* There is a convenient way to turn `List` into `List`,
[`Collections.unmodifiableList()`](https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#unmodifiableList-java.util.List-).
* If you're adding a convenience constructor to `SimpleActivityRecordSet()`
that accepts a list of records, you should probably use `List` for the parameter so that it can accept any `List` having usable
elements. This is what `ArrayList` does, presumably for the same reason:
https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#ArrayList-java.util.Collection-
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r513699439
##
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 getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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:
> Okay, I've worked through this - not entirely sure how well I've done.
I hit some issues with the `List` and converting
that to `List`. I ended up with the loops you see, here, now
- not sure if there's a more elegant way to do that translation from `? extends
RecordType` to just `RecordType`? ...
Can you point to the specific part of the code where you're encountering
trouble? I don't think there should be any need to convert.
> Also, the `HistoryResource` class in `rest/history` pulls history from
`UserContext`, whereas the `ConnectionResource` and `UserResource` classes in
`rest/connection` and `rest/user` pull history from `Connection` and `User`,
and I'm not sure if there are any modifications needed to the `HistoryResource`
class or anything in the `UserContext` interfaces or classes that need to be
tweaked to handle these changes?
Are you referring to whether the code that pulls directly from `UserContext`
needs to add special handling for `GuacamoleUnsupportedException`?
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
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 getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
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 getConnectionHistory()
+public ConnectionHistoryResource getConnectionHistory()
throws GuacamoleException {
-// Retrieve the requested connection's history
-List apiRecords = new
ArrayList();
-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()` -> `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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r512960190
##
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##
@@ -103,7 +103,26 @@
* If an error occurs while reading the history of this user, or if
* permission is denied.
*/
+@Deprecated
Review comment:
> Okay, so I could either do that, or just write `default`
implementations in `User` that return `List` (`getHistory()`)
and `ActivityRecordSet` (`getUserHistory()`). Is there a
situations where the default implementation returning empty sets would cause
issues?
For something like:
```java
public interface User {
...
@Deprecated
default List getHistory() throws
GuacamoleException {
return Collections.emptyList();
}
ActivityRecordSet getUserHistory() throws
GuacamoleException {
return new SimpleActivityRecordSet<>();
}
...
}
```
We end up with:
| Extension implements... | Result |
| -- | -- |
| Only `getHistory()` (older extension) | Webapp invokes `getUserHistory()`
which returns an empty set, despite `getHistory()` doing otherwise. |
| Only `getUserHistory()` (newer extension) | Webapp invokes
`getUserHistory()` which works as expected. If an older extension decorates
`User` objects and attempts to do something with `getHistory()`, it will not
have any effect and may behave as if history is unimplemented. |
| Both (masochistic extension) | All calls to history-related functions work
as expected. |
| Neither | Calls to either `getUserHistory()` or `getHistory()` have the
same result: empty set of records |
That ends up not working well because older extensions would cease being
compatible (they would function differently with 1.3.0 than the version of
Guacamole they were written for).
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r512937584
##
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##
@@ -103,7 +103,26 @@
* If an error occurs while reading the history of this user, or if
* permission is denied.
*/
+@Deprecated
Review comment:
If we have something like:
```java
public interface User {
...
@Deprecated
default List getHistory() throws
GuacamoleException {
return Collections.emptyList();
}
ActivityRecordSet getUserHistory() throws
GuacamoleException {
return new SomeSortOfSimpleActivityRecordSet<>(getHistory());
}
...
}
```
Then we end up with:
| Extension implements... | Result |
| -- | -- |
| Only `getHistory()` (older extension) | Webapp invokes `getUserHistory()`
which calls `getHistory()` by default and works as expected. |
| Only `getUserHistory()` (newer extension) | Webapp invokes
`getUserHistory()` which works as expected. If an older extension decorates
`User` objects and attempts to do something with `getHistory()`, it will not
have any effect and may behave as if history is unimplemented. |
| Both (masochistic extension) | All calls to history-related functions work
as expected. |
| Neither | Calls to either `getUserHistory()` or `getHistory()` have the
same result: empty set of records |
That seems OK except for the case of inter-extension calling of the older
`getHistory()` function.
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546: URL: https://github.com/apache/guacamole-client/pull/546#discussion_r512928085 ## File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java ## @@ -103,7 +103,26 @@ * If an error occurs while reading the history of this user, or if * permission is denied. */ +@Deprecated Review comment: At first, my impression was that there are a couple issues with that approach: 1. If an implementation of `User` or `Connection` does not support history tracking, it should return an empty `List` or empty `ActivityRecordSet` (depending on which history function we're talking about). Throwing an exception will cause any associated REST API call to fail with an error. 2. If the `default` version of the _new_ function (`getUserHistory()`) does not point back to the old function (`getHistory()`), then an extension for an older version of Guacamole that implemented the old function will mysteriously cease to function. *BUT:* Perhaps this can work well if the REST API itself handles `GuacamoleUnsupportedException`, relying on that to indicate whether the older function should be invoked? 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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546:
URL: https://github.com/apache/guacamole-client/pull/546#discussion_r511771968
##
File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java
##
@@ -103,7 +103,26 @@
* If an error occurs while reading the history of this user, or if
* permission is denied.
*/
+@Deprecated
Review comment:
OK - I haven't reviewed the meat of this, but the issue with the
interface is still there. I think I can explain a bit better and in more depth.
The API issue is that we're adding a new function to a Java interface, in
this case `User`. Consider a Java class defined within a Guacamole extension
like:
```java
public class ExampleUser implements User {
@Override
public ActivityRecordSet getUserHistory() {
// STUB
}
}
```
The above class will not compile because it does not implement the
deprecated `getHistory()` function. This is problematic, because it seems
unreasonable to require developers to implement deprecated functions that
should no longer be used. Ideally, they should implement only the current
functions and be OK.
Java 8 provides for this with default implementations. If the interface
provides a default implementation for `getHistory()`, then implementations need
not provide that function:
```java
public interface User {
...
default List getHistory() throws
GuacamoleException {
return new ArrayList<>(getUserHistory().toCollection());
}
...
}
```
So ... problem solved? Nope, because we need to maintain backward
compatibility, as well. Unless additional steps are taken, an attempt to use an
extension built for 1.2.0, 1.1.0, etc. may fail against 1.3.0 because a class
implementing `User` does not provide `getUserHistory()` - the new function.
Ah! But then we can just provide defaults for both, right?
```java
public interface User {
...
default List getHistory() throws
GuacamoleException {
return new ArrayList<>(getUserHistory().toCollection());
}
ActivityRecordSet getUserHistory() throws
GuacamoleException {
return new SomeSortOfSimpleActivityRecordSet<>(getHistory());
}
...
}
```
_Now_ problem solved? Almost. The above would work, and would solve all
compatibility issues, but we lose the benefit of having a compiler yell at us
if a needed function is unimplemented. Worse still: failing to implement at
least one of those functions would result in a stack overflow when the webapp
invokes `getUserHistory()`.
It is possible to solve this, but it involves:
* Providing a default only for the deprecated function within guacamole-ext.
(See [the current `Connectable` interface within
guacamole-ext](https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Connectable.java))
* Within the webapp, copying `User` entirely and providing a default _for
both_. (See [the current, internal `Connectable` interface within the
webapp](https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole/src/main/java/org/apache/guacamole/net/auth/Connectable.java))
That would allow old extensions to continue to work, with new extensions
intending to build against the new API getting the expected compile-time
errors, but makes things difficult to maintain until the deprecated things are
removed.
Overall, the type of change that results in this is replacing one function
with another. For `Connectable`, there was no choice at all. If a sensible set
of changes can accomplish what we need here (without replacing one function of
an interface with another), then the complexity can be avoided.
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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546: URL: https://github.com/apache/guacamole-client/pull/546#discussion_r511687968 ## File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java ## @@ -103,7 +103,26 @@ * If an error occurs while reading the history of this user, or if * permission is denied. */ +@Deprecated Review comment: Sure thing. 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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546: URL: https://github.com/apache/guacamole-client/pull/546#discussion_r483275361 ## File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java ## @@ -103,7 +103,26 @@ * If an error occurs while reading the history of this user, or if * permission is denied. */ +@Deprecated Review comment: The context of this deprecation needs to be documented within the JavaDoc as well. ## File path: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java ## @@ -103,7 +103,26 @@ * If an error occurs while reading the history of this user, or if * permission is denied. */ +@Deprecated Review comment: We'll have to be careful here: * As written, deprecated or not, an implementation of `User` will still be required to implement the older `getHistory()` function. * Providing a default implementation of `getHistory()` which leverages `getUserHistory()` would allow things to behave as expected, but then older extensions which implement only `getHistory()` will fail. * Providing default implementations of _both_ which each point at the other would would technically work, but would make implementing `User` dangerous. An extension that fails to implement either function would compile without any warning or error, yet invoking either function would result in infinite recursion. The way we worked around this for similar changes to `Connectable` was to provide a default implementation only for the deprecated function at the library level, but override that internally with an identical interface specific to the web application that provides default implementations of both. Finding some way to achieve this purely through enhancements to `ActivityRecordSet` could avoid the above. 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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
mike-jumper commented on a change in pull request #546: URL: https://github.com/apache/guacamole-client/pull/546#discussion_r449823359 ## 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 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 asCollection(String identifier) +throws GuacamoleException; Review comment: Another possibility could be to add something like `getConnectionHistory()` and `getUserHistory()` (returning `ActivityRecordSet` like their `UserContext` versions) to `Connection` and `User` respectively, deprecating the old `getHistory()`. That seems clean but would also lend itself well to default implementations (could just wrap `getHistory()` in some sort of basic `ActivityRecordSet` implementation). 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]
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #546: GUACAMOLE-1123: Standardize REST endpoints for historical data
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 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 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 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:
[email protected]
