You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by mike-jumper <gi...@git.apache.org> on 2018/04/12 04:33:20 UTC
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
GitHub user mike-jumper opened a pull request:
https://github.com/apache/guacamole-client/pull/271
GUACAMOLE-542: Add AbstractUserContext and AbstractAuthenticationProvider base classes.
Note that these changes also involved improving `SimpleDirectory` and, as a result, deprecating the `SimpleConnectionDirectory`, `SimpleUserDirectory`, etc. classes (as the `SimpleDirectory` now accomplishes everything necessary while being cleaner and more generic).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mike-jumper/guacamole-client base-api
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/271.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #271
----
commit 9b7ef0dfcf62626857f54a704e7e28acb574d92b
Author: Michael Jumper <mj...@...>
Date: 2018-04-12T00:03:39Z
GUACAMOLE-542: Migrate to simpler AbstractAuthenticationProvider / AbstractUserContext base classes.
commit 57ff8b84e6bd7f02f70999b4a77853bdc5279e8a
Author: Michael Jumper <mj...@...>
Date: 2018-04-12T00:04:07Z
GUACAMOLE-542: Deprecate SimpleConnectionDirectory, etc., relying instead on SimpleDirectory.
commit 9c90f75c161303c0a20a70a08d5b0af6b8356c5b
Author: Michael Jumper <mj...@...>
Date: 2018-04-12T00:55:39Z
GUACAMOLE-542: Explicitly document the behavior of the default implementations provided by AbstractUserContext and AbstractAuthenticationProvider.
----
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/271#discussion_r181044995
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticationProvider.java ---
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.auth;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Base implementation of AuthenticationProvider which provides default
+ * implementations of most functions. Implementations must provide their
+ * own {@link #getIdentifier()}, but otherwise need only override an implemented
+ * function if they wish to actually implement the functionality defined for
+ * that function by the AuthenticationProvider interface.
+ */
+public abstract class AbstractAuthenticationProvider implements AuthenticationProvider {
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This implementation simply returns <tt>null</tt>. Implementations that
--- End diff --
I see some of these HTML tags in a few other Java classes, but definitely does not seem to be widely used throughout JavaDoc comments in the Guacamole code. Did you intend to put these here? Is that something you want to start using, or something that crept in from an IDE?
Similarly, the `{@inheritDoc}` directive doesn't seem to show up anywhere else - again, something/new intentional, or something that slipped by?
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/271
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/271#discussion_r181473524
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticationProvider.java ---
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.auth;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Base implementation of AuthenticationProvider which provides default
+ * implementations of most functions. Implementations must provide their
+ * own {@link #getIdentifier()}, but otherwise need only override an implemented
+ * function if they wish to actually implement the functionality defined for
+ * that function by the AuthenticationProvider interface.
+ */
+public abstract class AbstractAuthenticationProvider implements AuthenticationProvider {
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This implementation simply returns <tt>null</tt>. Implementations that
--- End diff --
Sounds good. Just wanted to make sure. We should probably also update the style guidelines on the web page...
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/271#discussion_r181179697
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticationProvider.java ---
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.auth;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Base implementation of AuthenticationProvider which provides default
+ * implementations of most functions. Implementations must provide their
+ * own {@link #getIdentifier()}, but otherwise need only override an implemented
+ * function if they wish to actually implement the functionality defined for
+ * that function by the AuthenticationProvider interface.
+ */
+public abstract class AbstractAuthenticationProvider implements AuthenticationProvider {
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This implementation simply returns <tt>null</tt>. Implementations that
--- End diff --
Use of `<tt>` might be legacy, though... It's used for `putAll()` above, but the actual docs for javadoc specify `<code>` or `{@code ...}`:
https://docs.oracle.com/javase/1.5.0/docs/tooldocs/windows/javadoc.html#%7B@code%7D
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/271#discussion_r181227238
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticationProvider.java ---
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.auth;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Base implementation of AuthenticationProvider which provides default
+ * implementations of most functions. Implementations must provide their
+ * own {@link #getIdentifier()}, but otherwise need only override an implemented
+ * function if they wish to actually implement the functionality defined for
+ * that function by the AuthenticationProvider interface.
+ */
+public abstract class AbstractAuthenticationProvider implements AuthenticationProvider {
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This implementation simply returns <tt>null</tt>. Implementations that
--- End diff --
I'll at least migrate things to match the modern recommendations in JavaDoc's docs.
---
[GitHub] guacamole-client pull request #271: GUACAMOLE-542: Add AbstractUserContext a...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/271#discussion_r181179068
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticationProvider.java ---
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.auth;
+
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Base implementation of AuthenticationProvider which provides default
+ * implementations of most functions. Implementations must provide their
+ * own {@link #getIdentifier()}, but otherwise need only override an implemented
+ * function if they wish to actually implement the functionality defined for
+ * that function by the AuthenticationProvider interface.
+ */
+public abstract class AbstractAuthenticationProvider implements AuthenticationProvider {
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This implementation simply returns <tt>null</tt>. Implementations that
--- End diff --
> I see some of these HTML tags in a few other Java classes, but definitely does not seem to be widely used throughout JavaDoc comments in the Guacamole code. Did you intend to put these here?
Yep. This is intentional. JavaDoc uses a subset of HTML for formatting.
> Is that something you want to start using, or something that crept in from an IDE?
We probably should start using such formatting as applicable. The additional formatting makes the documentation significantly more readable, and there are more than a few comments in older code which rely on my erroneous impression that blank lines would imply a new paragraph (JavaDoc actually just ignores those blanks).
> Similarly, the `{@inheritDoc}` directive doesn't seem to show up anywhere else - again, something/new intentional, or something that slipped by?
Definitely intentional.
It is normally not necessary, as JavaDoc automatically pulls in the documentation for overridden methods, but it's necessary here where I'm intending to add a note regarding implementation without replacing (or manually duplicating) the existing documentation.
Including `{@inheritDoc}` like this pulls in the original documentation for the function, all parameters, the return value, throws, etc., while also appending the text below, which in this case are implementation details.
I looked to the main Java source for examples on how such default implementations should be documented, as I recalled that `AbstractMap` had similar needs for its `putAll()` implementation ([noting that it relies on `put()`](https://docs.oracle.com/javase/7/docs/api/java/util/AbstractMap.html#putAll(java.util.Map))):
/**
* {@inheritDoc}
*
* <p>This implementation iterates over the specified map's
* <tt>entrySet()</tt> collection, and calls this map's <tt>put</tt>
* operation once for each entry returned by the iteration.
*
* <p>Note that this implementation throws an
* <tt>UnsupportedOperationException</tt> if this map does not support
* the <tt>put</tt> operation and the specified map is nonempty.
*
* @throws UnsupportedOperationException {@inheritDoc}
* @throws ClassCastException {@inheritDoc}
* @throws NullPointerException {@inheritDoc}
* @throws IllegalArgumentException {@inheritDoc}
*/
public void putAll(Map<? extends K, ? extends V> m) {
for (Map.Entry<? extends K, ? extends V> e : m.entrySet())
put(e.getKey(), e.getValue());
}
---