Cole-Greer commented on code in PR #3287:
URL: https://github.com/apache/tinkerpop/pull/3287#discussion_r2700523832
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java:
##########
@@ -340,13 +336,270 @@ public long getEvaluationTimeout() {
/**
* Read configuration from a file into a new {@link Settings} object.
+ * <p>
+ * This method supports recursive includes of other YAML files over the
"includes" property that contains
+ * a list of strings that can be:
+ * <ol>
+ * <li>Relative paths to other YAML files</li>
+ * <li>Absolute paths to other YAML files</li>
+ * <li>Classpath resources</li>
+ * </ol>
+ * <p>
+ * If properties names of the included files or root file (file that
contains "includes" property) are the same,
+ * they are overwritten forming a single property set. In any other cases
just appended to the root file.
+ * "includes" can be nested and included files can also contain "includes"
property.
+ * Included files can override properties of other included files, the
root file in turn overriding properties of included files.
+ * <p>
+ * This is quite permissive strategy that works because we then map
resulting YAML to <code>Settings</code> object
+ * preventing any configuration inconsistencies.
+ * <p>
+ * Abstract example:
+ * <code>base.yaml</code>
+ * <pre>
+ * server:
+ * connector: { port: 8080, protocol: 'http' }
+ * logging: { level: 'INFO' }
+ * </pre>
+ * <code>root.yaml</code>
+ * <pre>
+ * includes: ['base.yaml']
+ * server:
+ * connector: { port: 9090 } # Overwrite the port only
+ * logging: { file: '/var/log/app.log' } # Add the file, keep level
+ * </pre>
+ * <p>
+ * Resulting configuration:
+ * <pre>
+ * server:
+ * connector: { port: 9090, protocol: 'http' }
+ * logging: { level: 'INFO', file: '/var/log/app.log' }
+ * </pre>
*
* @param file the location of a Gremlin Server YAML configuration file
* @return a new {@link Optional} object wrapping the created {@link
Settings}
*/
- public static Settings read(final String file) throws Exception {
- final InputStream input = new FileInputStream(new File(file));
- return read(input);
+ public static Settings read(final String file) {
+ final NodeMapper constructor = createDefaultYamlConstructor();
+ final Yaml yaml = new Yaml();
+
+ HashSet<String> loadStack = new HashSet<>();
+
+ // Normalize the initial path
+ String normalizedPath = normalizeInitialPath(file);
+ Node finalNode = loadNodeRecursive(yaml, normalizedPath, loadStack);
+ if (finalNode == null) {
+ return new Settings();
+ }
+
+ finalNode.setTag(new Tag(Settings.class));
+ return (Settings) constructor.map(finalNode);
+ }
+
+
+ private static Node loadNodeRecursive(Yaml yaml, String currentPath,
HashSet<String> loadStack) {
+ try {
+ if (loadStack.contains(currentPath)) {
+ throw new IllegalStateException("Circular dependency detected:
" + currentPath);
+ }
+
+ loadStack.add(currentPath);
+ try (InputStream inputStream = getInputStream(currentPath)) {
+ Node rootNode = yaml.compose(new
InputStreamReader(inputStream, StandardCharsets.UTF_8));
+ if (!(rootNode instanceof MappingNode)) {
+ return rootNode;
+ }
+ MappingNode rootMappingNode = (MappingNode) rootNode;
+ //Extract and remove "includes"
+ List<String> includes =
extractAndRemoveIncludes(rootMappingNode);
+ //Base Accumulator
+ MappingNode accumulatedNode = new MappingNode(Tag.MAP, new
ArrayList<>(), rootMappingNode.getFlowStyle());
+ //Process Includes
+ if (!includes.isEmpty()) {
+ for (String includeRaw : includes) {
+ //Resolve the include path relative to the current
file (or absolute)
+ String resolvedIncludePath = resolvePath(currentPath,
includeRaw);
+ Node includedNode = loadNodeRecursive(yaml,
resolvedIncludePath, loadStack);
+
+ if (includedNode instanceof MappingNode) {
+ mergeMappingNodes(accumulatedNode, (MappingNode)
includedNode);
+ } else {
+ // Non-map include replaces everything
Review Comment:
I'm curious what is the expected case which would trigger this else?
Directly returning `includedNode` and ignoring all other configs seems like a
very strong action.
I noticed while debugging through
`org.apache.tinkerpop.gremlin.server.SettingsTest#testIncludeBranches`, the
processing of the empty `base1.yaml` results in `includedNode = null`, which
enters this else statement and entirely prevents the test from ever touching
`branch2.yaml` and `base2.yaml`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
