Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/569#discussion_r107668046
--- Diff:
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GraphManager.java
---
@@ -21,139 +21,98 @@
import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Transaction;
-import org.apache.tinkerpop.gremlin.structure.util.GraphFactory;
-import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
import javax.script.Bindings;
-import javax.script.SimpleBindings;
-import java.util.HashSet;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
-
-/**
- * Holder for {@link Graph} and {@link TraversalSource} instances
configured for the server to be passed to script
- * engine bindings. The {@link Graph} instances are read from the {@link
Settings} for Gremlin Server as defined in
- * the configuration file. The {@link TraversalSource} instances are
rebound to the {@code GraphManager} once
- * initialization scripts construct them.
- */
-public final class GraphManager {
- private static final Logger logger =
LoggerFactory.getLogger(GremlinServer.class);
-
- private final Map<String, Graph> graphs = new ConcurrentHashMap<>();
- private final Map<String, TraversalSource> traversalSources = new
ConcurrentHashMap<>();
+import java.util.function.Supplier;
+public interface GraphManager {
/**
- * Create a new instance using the {@link Settings} from Gremlin
Server.
+ * Get a list of the {@link Graph} instances and their binding names
+ *
+ * @return a {@link Map} where the key is the name of the {@link
Graph} and the value is the {@link Graph} itself
*/
- public GraphManager(final Settings settings) {
- settings.graphs.entrySet().forEach(e -> {
- try {
- final Graph newGraph = GraphFactory.open(e.getValue());
- graphs.put(e.getKey(), newGraph);
- logger.info("Graph [{}] was successfully configured via
[{}].", e.getKey(), e.getValue());
- } catch (RuntimeException re) {
- logger.warn(String.format("Graph [%s] configured at [%s]
could not be instantiated and will not be available in Gremlin Server.
GraphFactory message: %s",
- e.getKey(), e.getValue(), re.getMessage()), re);
- if (re.getCause() != null) logger.debug("GraphFactory
exception", re.getCause());
- }
- });
- }
+ public Map<String, Graph> getGraphs();
--- End diff --
So, now we have `getGraphs()` which returns the `Map` of graph names and
`Graph` instances but then we also expose get/put/remove operations that just
manipulate that same `Map`. That seems like it needs more thought. In other
words, why do:
```java
manager.addGraph("graph", new TinkerGraph());
manager.getGraph("graph");
```
if I can also do:
```java
manager.getGraphs().put("graph",new TinkerGraph());
manager.getGraphs().get("graph");
```
By offering two approaches to do the same thing we open the possibility to
error. If a `GraphManager` implementation adds logic to `addGraph()` (or
related method) then that logic can simply be bypassed by directly manipulating
the `Map` via `getGraphs()`.
Seems like we would want to use `GraphManager` to enforce the logic of the
implementation, thus `getGraphs()` should go away? Perhaps to lessen the
breaking change for 3.2.x it is simply deprecated with some additional javadoc
that states that the method is expected to return an umodifiable `Map`? replace
that method with `Set<String> getGraphNames()` so you could iterate over that
to get all the `Graph` instances or maybe have `GraphManager` implement
`Iterable` so you could `foreach` and get an `iterator()`? and then finally
replace all internal usage of `getGraphs()` so that it can safely be removed on
the master branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
