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<ActivityRecord> 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<? extends ActivityRecord> 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<? extends ActivityRecord> getHistory() throws
GuacamoleException {
return new ArrayList<>(getUserHistory().toCollection());
}
ActivityRecordSet<ActivityRecord> 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]
