Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfigDocument API #280

Merged
merged 19 commits into from
Mar 24, 2015
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions config/src/main/java/com/typesafe/config/ConfigNode.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config;

/**
* An immutable node that makes up the ConfigDocument AST, and which can be
* used to reproduce part or all of the original text of an input.
*
* <p>
* Because this object is immutable, it is safe to use from multiple threads and
* there's no need for "defensive copies."
*
* <p>
* <em>Do not implement interface {@code ConfigNode}</em>; it should only be
* implemented by the config library. Arbitrary implementations will not work
* because the library internals assume a specific concrete implementation.
* Also, this interface is likely to grow new methods over time, so third-party
* implementations will break.
*/
public interface ConfigNode {
/**
* The original text of the input which was used to form this particular node.
* @return the original text used to form this node as a String
*/
public String render();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import com.typesafe.config.ConfigNode;
import java.util.Collection;

abstract class AbstractConfigNode implements ConfigNode {
abstract Collection<Token> tokens();
final public String render() {
StringBuilder origText = new StringBuilder();
Iterable<Token> tokens = tokens();
for (Token t : tokens) {
origText.append(t.tokenText());
}
return origText.toString();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

// This is required if we want
// to be referencing the AbstractConfigNode class in implementation rather than the
// ConfigNode interface, as we can't cast an AbstractConfigNode to an interface
abstract class AbstractConfigNodeValue extends AbstractConfigNode {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import com.typesafe.config.ConfigNode;

import java.util.*;

final class ConfigNodeComplexValue extends AbstractConfigNodeValue {
final private ArrayList<AbstractConfigNode> children;

ConfigNodeComplexValue(Collection<AbstractConfigNode> children) {
this.children = new ArrayList(children);
}

public Iterable<AbstractConfigNode> children() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be final I guess

return children;
}

protected Collection<Token> tokens() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should have @Override

ArrayList<Token> tokens = new ArrayList();
for (AbstractConfigNode child : children) {
tokens.addAll(child.tokens());
}
return tokens;
}

protected ConfigNodeComplexValue changeValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
boolean replaced = value == null;
ConfigNodeKeyValue node;
Path key;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two vars could be inside the loop right?

for (int i = children.size() - 1; i >= 0; i--) {
if (!(children.get(i) instanceof ConfigNodeKeyValue)) {
continue;
}
node = (ConfigNodeKeyValue)children.get(i);
key = node.key().value();
if (key.equals(desiredPath)) {
if (!replaced) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a nice practice to avoid combining negation and if/else (do if (replaced) { remove(i) } else {})

childrenCopy.set(i, node.replaceValue(value));
replaced = true;
}
else
childrenCopy.remove(i);
} else if (desiredPath.startsWith(key)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below feels like it should be using recursion somehow - maybe it's as simple as childrenCopy.set(i, node.changeValueOnPath(remainingPath, value)) ?

if (node.value() instanceof ConfigNodeComplexValue) {
Path remainingPath = desiredPath.subPath(key.length());
if (!replaced) {
node = node.replaceValue(((ConfigNodeComplexValue) node.value()).changeValueOnPath(remainingPath, value));
if (node.render() != children.get(i).render())
replaced = true;
childrenCopy.set(i, node);
} else {
node = node.replaceValue(((ConfigNodeComplexValue) node.value()).removeValueOnPath(remainingPath));
childrenCopy.set(i, node);
}
}
}
}
return new ConfigNodeComplexValue(childrenCopy);
}

public ConfigNodeComplexValue setValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
ConfigNodeComplexValue node = changeValueOnPath(desiredPath, value);

// If the desired Path did not exist, add it
if (node.render().equals(render())) {
ArrayList<AbstractConfigNode> childrenCopy = new ArrayList<AbstractConfigNode>(children);
ArrayList<AbstractConfigNode> newNodes = new ArrayList();
newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
newNodes.add(new ConfigNodeKey(desiredPath));
newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
newNodes.add(new ConfigNodeSingleToken(Tokens.COLON));
newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
newNodes.add(value);
newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
childrenCopy.add(new ConfigNodeKeyValue(newNodes));
node = new ConfigNodeComplexValue(childrenCopy);
}
return node;
}

public ConfigNodeComplexValue removeValueOnPath(Path desiredPath) {
return changeValueOnPath(desiredPath, null);
}

}
21 changes: 21 additions & 0 deletions config/src/main/java/com/typesafe/config/impl/ConfigNodeKey.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import java.util.Collection;

final class ConfigNodeKey extends AbstractConfigNode {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a convention throughout the lib, or at least in public API, that we use "key" to mean one element of a path; paths have to be parsed, keys are the raw strings in each Path element. ConfigNodeKey should probably be ConfigNodePath to be sure we don't get confused on this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I think it would also be worth changing ConfigNodeKeyValue to ConfigNodePathValue so I'll go ahead and do that too.

final private Path key;
ConfigNodeKey(Path key) {
this.key = key;
}

protected Collection<Token> tokens() {
return key.tokens();
}

protected Path value() {
return key;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import com.typesafe.config.ConfigException;
import com.typesafe.config.ConfigNode;

import java.util.ArrayList;
import java.util.Collection;

final class ConfigNodeKeyValue extends AbstractConfigNode {
final private ArrayList<AbstractConfigNode> children;

public ConfigNodeKeyValue(Collection<AbstractConfigNode> children) {
this.children = new ArrayList(children);
}

protected Collection<Token> tokens() {
ArrayList<Token> tokens = new ArrayList();
for (AbstractConfigNode child : children) {
tokens.addAll(child.tokens());
}
return tokens;
}

public ConfigNodeKeyValue replaceValue(AbstractConfigNodeValue newValue) {
ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
for (int i = 0; i < childrenCopy.size(); i++) {
if (childrenCopy.get(i) instanceof AbstractConfigNodeValue) {
childrenCopy.set(i, newValue);
return new ConfigNodeKeyValue(childrenCopy);
}
}
throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
}

public AbstractConfigNodeValue value() {
for (int i = 0; i < children.size(); i++) {
if (children.get(i) instanceof AbstractConfigNodeValue) {
return (AbstractConfigNodeValue)children.get(i);
}
}
throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
}

public ConfigNodeKey key() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be called path() and the KeyValue class could either be called PathValue or perhaps simply Field would be easier.

for (int i = 0; i < children.size(); i++) {
if (children.get(i) instanceof ConfigNodeKey) {
return (ConfigNodeKey)children.get(i);
}
}
throw new ConfigException.BugOrBroken("KeyValue node doesn't have a key");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import java.util.Collection;
import java.util.Collections;

class ConfigNodeSimpleValue extends AbstractConfigNodeValue {
final Token token;
ConfigNodeSimpleValue(Token value) {
token = value;
}

protected Collection<Token> tokens() {
return Collections.singletonList(token);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
*/
package com.typesafe.config.impl;

import java.util.Collection;
import java.util.Collections;

final class ConfigNodeSingleToken extends AbstractConfigNode{
final Token token;
ConfigNodeSingleToken(Token t) {
token = t;
}

protected Collection<Token> tokens() {
return Collections.singletonList(token);
}
}
8 changes: 6 additions & 2 deletions config/src/main/java/com/typesafe/config/impl/Parser.java
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,7 @@ private static Path parsePathExpression(Iterator<Token> expression,
private static Path parsePathExpression(Iterator<Token> expression,
ConfigOrigin origin, String originalText) {
// each builder in "buf" is an element in the path.
ArrayList<Token> pathTokens = new ArrayList<Token>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a somewhat messy mixing of concerns to make Path and PathBuilder keep the tokens around; we do use Path in a performance-sensitive context when we parse paths passed to the getters on Config, like Config.getInt. That's why there's all this fast-path path-parsing stuff. But, even ignoring performance I think it's a little messy. What I'd suggest is to factor this code so there's a version that returns a Path and a version that returns a ConfigNodeKey, and then a private function which maybe takes the pathTokens array list which can be null. So:

private static Path parsePathExpression(Iterator<Token> expression, ConfigOrigin origin, String originalText, ArrayList<Token> pathTokens) {
   ...
   if (pathTokens != null) pathTokens.add(t)
   ...
}

Then you can have the functions which call that either provide the ArrayList or provide null, depending on whether they want to end up with a Path or ConfigNodeKey

It would be fine to skip the speculativeFastParsePath codepath for ConfigNodeKey, and always use the slow parsePathExpression when we need the tokens. The fast path matters only for that Config.getInt case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's doable, but I think in that case, a ConfigNodePath is going to need to be passed into the setValueOnPath() method in ConfigNodeComplexValue, since we'll need the tokens to add in a new field.

List<Element> buf = new ArrayList<Element>();
buf.add(new Element("", false));

Expand All @@ -1073,6 +1074,7 @@ private static Path parsePathExpression(Iterator<Token> expression,

while (expression.hasNext()) {
Token t = expression.next();
pathTokens.add(t);

// Ignore all IgnoredWhitespace tokens
if (Tokens.isIgnoredWhitespace(t))
Expand Down Expand Up @@ -1119,7 +1121,7 @@ private static Path parsePathExpression(Iterator<Token> expression,
}
}

PathBuilder pb = new PathBuilder();
PathBuilder pb = new PathBuilder(pathTokens);
for (Element e : buf) {
if (e.sb.length() == 0 && !e.canBeEmpty) {
throw new ConfigException.BadPath(
Expand Down Expand Up @@ -1192,8 +1194,10 @@ private static boolean looksUnsafeForFastParser(String s) {
private static Path fastPathBuild(Path tail, String s, int end) {
// lastIndexOf takes last index it should look at, end - 1 not end
int splitAt = s.lastIndexOf('.', end - 1);
ArrayList<Token> tokens = new ArrayList<Token>();
tokens.add(Tokens.newUnquotedText(null, s));
// this works even if splitAt is -1; then we start the substring at 0
Path withOneMoreElement = new Path(s.substring(splitAt + 1, end), tail);
Path withOneMoreElement = new Path(s.substring(splitAt + 1, end), tail, tokens);
if (splitAt < 0) {
return withOneMoreElement;
} else {
Expand Down
32 changes: 32 additions & 0 deletions config/src/main/java/com/typesafe/config/impl/Path.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
*/
package com.typesafe.config.impl;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;

Expand All @@ -13,12 +15,22 @@ final class Path {
final private String first;
final private Path remainder;

// We only need to keep track of this for top-level paths created with
// parsePath, so this will be empty or null for all other cases
final private ArrayList<Token> tokens;

Path(String first, Path remainder) {
this(first, remainder, new ArrayList<Token>());
}

Path(String first, Path remainder, Collection<Token> tokens) {
this.first = first;
this.remainder = remainder;
this.tokens = new ArrayList<Token>(tokens);
}

Path(String... elements) {
this.tokens = null;
if (elements.length == 0)
throw new ConfigException.BugOrBroken("empty path");
this.first = elements[0];
Expand All @@ -40,6 +52,7 @@ final class Path {

// append all the paths in the iterator together into one path
Path(Iterator<Path> i) {
this.tokens = null;
if (!i.hasNext())
throw new ConfigException.BugOrBroken("empty path");

Expand Down Expand Up @@ -141,6 +154,21 @@ Path subPath(int firstIndex, int lastIndex) {
return pb.result();
}

boolean startsWith(Path other) {
Path myRemainder = this;
Path otherRemainder = other;
if (otherRemainder.length() <= myRemainder.length()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since length is O(n) it would be a bit nicer to instead check myRemainder for null, below, perhaps. Since paths are generally short it probably doesn't really matter that much.

while(otherRemainder != null) {
if (!otherRemainder.first().equals(myRemainder.first()))
return false;
myRemainder = myRemainder.remainder();
otherRemainder = otherRemainder.remainder();
}
return true;
}
return false;
}

@Override
public boolean equals(Object other) {
if (other instanceof Path) {
Expand Down Expand Up @@ -189,6 +217,10 @@ private void appendToStringBuilder(StringBuilder sb) {
}
}

protected Collection<Token> tokens() {
return tokens;
}

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
Expand Down
Loading