diff options
author | Jon Bratseth <bratseth@gmail.com> | 2021-12-14 18:27:52 +0100 |
---|---|---|
committer | Jon Bratseth <bratseth@gmail.com> | 2021-12-14 18:27:52 +0100 |
commit | 8a085f018f881670bb6f0907c431e37f12d77492 (patch) | |
tree | 9c155e88e4c14c1b2a187e5796449315828ba337 | |
parent | 3a2f9ddf6b337673aeffca83b6801f9e4fa96aee (diff) |
Match document types in document selections exactly only
Routing all child types to a cluster a parent is added to
may be convenient for some users, but if it's not what you want
it is then harder to prevent it from happening.
16 files changed, 170 insertions, 58 deletions
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/DistributorCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/DistributorCluster.java index 518c3f73a8f..7a45689901f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/DistributorCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/DistributorCluster.java @@ -25,7 +25,6 @@ public class DistributorCluster extends AbstractConfigProducer<Distributor> impl public static final Logger log = Logger.getLogger(DistributorCluster.class.getPackage().toString()); - private static class GcOptions { public final int interval; @@ -55,8 +54,8 @@ public class DistributorCluster extends AbstractConfigProducer<Distributor> impl this.parent = parent; } - private String prepareGCSelection(ModelElement documentNode, String selStr) throws ParseException { - DocumentSelector s = new DocumentSelector(selStr); + private String prepareGCSelection(ModelElement documentNode, String selectionString) throws ParseException { + DocumentSelector s = new DocumentSelector(selectionString); boolean enableGC = false; if (documentNode != null) { enableGC = documentNode.booleanAttribute("garbage-collection", false); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/DocumentTypeVisitor.java b/config-model/src/main/java/com/yahoo/vespa/model/content/DocumentTypeVisitor.java index eeb0eac8fcc..7e839f230b7 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/DocumentTypeVisitor.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/DocumentTypeVisitor.java @@ -14,6 +14,7 @@ import com.yahoo.document.select.rule.NowNode; import com.yahoo.document.select.rule.VariableNode; public abstract class DocumentTypeVisitor implements Visitor { + @Override public void visit(ArithmeticNode arithmeticNode) { for (ArithmeticNode.NodeItem item : arithmeticNode.getItems()) { @@ -64,4 +65,5 @@ public abstract class DocumentTypeVisitor implements Visitor { @Override public void visit(VariableNode variableNode) { } + } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DocumentSelectionBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DocumentSelectionBuilder.java index ea444a6c7f8..ee60d161888 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DocumentSelectionBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DocumentSelectionBuilder.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.model.content.cluster; import com.yahoo.document.select.DocumentSelector; import com.yahoo.document.select.parser.ParseException; import com.yahoo.document.select.rule.DocumentNode; +import com.yahoo.document.select.rule.DocumentTypeNode; import com.yahoo.vespa.model.content.DocumentTypeVisitor; import com.yahoo.vespa.model.builder.xml.dom.ModelElement; @@ -13,6 +14,7 @@ import com.yahoo.vespa.model.builder.xml.dom.ModelElement; public class DocumentSelectionBuilder { private static class AllowedDocumentTypesChecker extends DocumentTypeVisitor { + String allowedType; private AllowedDocumentTypesChecker(String allowedType) { @@ -21,26 +23,35 @@ public class DocumentSelectionBuilder { @Override public void visit(DocumentNode documentNode) { - if (!documentNode.getType().equals(this.allowedType)) { + validateType(documentNode.getType()); + } + + @Override + public void visit(DocumentTypeNode documentTypeNode) { + validateType(documentTypeNode.getType()); + } + + private void validateType(String type) { + if (!type.equals(this.allowedType)) { if (this.allowedType == null) { throw new IllegalArgumentException("Document type references are not allowed " + - "in global <documents> tag selection attribute (found reference to type '" + - documentNode.getType() + "')"); + "in global <documents> tag selection attribute " + + "(found reference to type '" + type + "')"); } else { - throw new IllegalArgumentException("Selection for document type '" + - this.allowedType + "' can not contain references to other " + - "document types (found reference to type '" + documentNode.getType() + "')"); + throw new IllegalArgumentException("Selection for document type '" + this.allowedType + + "' can not contain references to other document types " + + "(found reference to type '" + type + "')"); } } } } - private void validateSelectionExpression(String sel, String allowedType) { + private void validateSelectionExpression(String selectionString, String allowedType) { DocumentSelector s; try { - s = new DocumentSelector(sel); + s = new DocumentSelector(selectionString); } catch (ParseException e) { - throw new IllegalArgumentException("Could not parse document routing selection: " + sel, e); + throw new IllegalArgumentException("Could not parse document routing selection: " + selectionString, e); } AllowedDocumentTypesChecker checker = new AllowedDocumentTypesChecker(allowedType); s.visit(checker); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/DocumentSelectionConverter.java b/config-model/src/main/java/com/yahoo/vespa/model/search/DocumentSelectionConverter.java index 84a55184629..9ea7c531b9a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/search/DocumentSelectionConverter.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/search/DocumentSelectionConverter.java @@ -6,11 +6,11 @@ import com.yahoo.document.select.convert.SelectionExpressionConverter; import com.yahoo.document.select.parser.ParseException; import java.util.Map; - /** * @author Ulf Lilleengen */ -// TODO: Public methods only used in tests. What is this class for? +// TODO: This should be renamed to reflect it is only a validator of expressions +// (in DocumentSelector and SelectionExpressionConverter) public class DocumentSelectionConverter { private final DocumentSelector selector; diff --git a/document/src/main/java/com/yahoo/document/select/Context.java b/document/src/main/java/com/yahoo/document/select/Context.java index 9853a4d6cfd..f36daf47c23 100644 --- a/document/src/main/java/com/yahoo/document/select/Context.java +++ b/document/src/main/java/com/yahoo/document/select/Context.java @@ -7,6 +7,7 @@ import java.util.HashMap; import java.util.Map; public class Context { + private DocumentOperation documentOperation; private Map<String, Object> variables = new HashMap<>(); diff --git a/document/src/main/java/com/yahoo/document/select/NowCheckVisitor.java b/document/src/main/java/com/yahoo/document/select/NowCheckVisitor.java index 76d9272903a..9aa87601d92 100644 --- a/document/src/main/java/com/yahoo/document/select/NowCheckVisitor.java +++ b/document/src/main/java/com/yahoo/document/select/NowCheckVisitor.java @@ -5,6 +5,7 @@ import com.yahoo.document.select.rule.ArithmeticNode; import com.yahoo.document.select.rule.AttributeNode; import com.yahoo.document.select.rule.ComparisonNode; import com.yahoo.document.select.rule.DocumentNode; +import com.yahoo.document.select.rule.DocumentTypeNode; import com.yahoo.document.select.rule.EmbracedNode; import com.yahoo.document.select.rule.IdNode; import com.yahoo.document.select.rule.LiteralNode; @@ -18,8 +19,8 @@ import com.yahoo.document.select.rule.VariableNode; * * @author Ulf Lilleengen */ - public class NowCheckVisitor implements Visitor { + private int nowNodeCount = 0; public boolean requiresConversion() { @@ -41,18 +42,17 @@ public class NowCheckVisitor implements Visitor { node.getRHS().accept(this); } - public void visit(DocumentNode node) { - } + public void visit(DocumentNode node) {} + + public void visit(DocumentTypeNode node) {} public void visit(EmbracedNode node) { node.getNode().accept(this); } - public void visit(IdNode node) { - } + public void visit(IdNode node) {} - public void visit(LiteralNode node) { - } + public void visit(LiteralNode node) {} public void visit(LogicNode node) { for (LogicNode.NodeItem item : node.getItems()) { diff --git a/document/src/main/java/com/yahoo/document/select/Visitor.java b/document/src/main/java/com/yahoo/document/select/Visitor.java index 4c198c9cf0d..6a9bf151ffb 100644 --- a/document/src/main/java/com/yahoo/document/select/Visitor.java +++ b/document/src/main/java/com/yahoo/document/select/Visitor.java @@ -1,11 +1,11 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.document.select; - import com.yahoo.document.select.rule.ArithmeticNode; import com.yahoo.document.select.rule.AttributeNode; import com.yahoo.document.select.rule.ComparisonNode; import com.yahoo.document.select.rule.DocumentNode; +import com.yahoo.document.select.rule.DocumentTypeNode; import com.yahoo.document.select.rule.EmbracedNode; import com.yahoo.document.select.rule.IdNode; import com.yahoo.document.select.rule.LiteralNode; @@ -15,16 +15,17 @@ import com.yahoo.document.select.rule.NowNode; import com.yahoo.document.select.rule.VariableNode; /** - * This interface can be used to create custom visitors for the selection tree. + * A visitor of the document selection tree. * * @author Ulf Lilleengen */ - public interface Visitor { + void visit(ArithmeticNode node); void visit(AttributeNode node); void visit(ComparisonNode node); void visit(DocumentNode node); + void visit(DocumentTypeNode node); void visit(EmbracedNode node); void visit(IdNode node); void visit(LiteralNode node); @@ -32,4 +33,5 @@ public interface Visitor { void visit(NegationNode node); void visit(NowNode node); void visit(VariableNode node); + } diff --git a/document/src/main/java/com/yahoo/document/select/convert/NowQueryExpression.java b/document/src/main/java/com/yahoo/document/select/convert/NowQueryExpression.java index ee4222fc0fb..c4a0065ac6f 100644 --- a/document/src/main/java/com/yahoo/document/select/convert/NowQueryExpression.java +++ b/document/src/main/java/com/yahoo/document/select/convert/NowQueryExpression.java @@ -12,6 +12,7 @@ import com.yahoo.document.select.rule.ComparisonNode; * @author Ulf Lilleengen */ public class NowQueryExpression { + private final AttributeNode attribute; private final ComparisonNode comparison; private final NowQueryNode now; @@ -33,7 +34,7 @@ public class NowQueryExpression { sb.append(item.getName()).append("."); } sb.deleteCharAt(sb.length() - 1); - return sb.toString() + ":" + comparison.getOperator() + now; + return sb + ":" + comparison.getOperator() + now; } } diff --git a/document/src/main/java/com/yahoo/document/select/convert/SelectionExpressionConverter.java b/document/src/main/java/com/yahoo/document/select/convert/SelectionExpressionConverter.java index aa4866ffa37..3ada0c298e4 100644 --- a/document/src/main/java/com/yahoo/document/select/convert/SelectionExpressionConverter.java +++ b/document/src/main/java/com/yahoo/document/select/convert/SelectionExpressionConverter.java @@ -7,6 +7,7 @@ import com.yahoo.document.select.rule.ArithmeticNode; import com.yahoo.document.select.rule.AttributeNode; import com.yahoo.document.select.rule.ComparisonNode; import com.yahoo.document.select.rule.DocumentNode; +import com.yahoo.document.select.rule.DocumentTypeNode; import com.yahoo.document.select.rule.EmbracedNode; import com.yahoo.document.select.rule.ExpressionNode; import com.yahoo.document.select.rule.IdNode; @@ -27,13 +28,15 @@ import java.util.Map; */ public class SelectionExpressionConverter implements Visitor { - private Map<String, NowQueryExpression> expressionMap = new HashMap<>(); + private final Map<String, NowQueryExpression> expressionMap = new HashMap<>(); + + private static class BuildState { - private class BuildState { public AttributeNode attribute; public ComparisonNode comparison; public ArithmeticNode arithmetic; public NowNode now; + } private BuildState state; @@ -56,7 +59,6 @@ public class SelectionExpressionConverter implements Visitor { return ret; } - public void visit(ArithmeticNode node) { if (state == null ) return; if (node.getItems().size() > 2) { @@ -114,6 +116,10 @@ public class SelectionExpressionConverter implements Visitor { // Silently ignore } + public void visit(DocumentTypeNode node) { + // Silently ignore + } + public void visit(EmbracedNode node) { if (state == null ) return; throw new UnsupportedOperationException("Grouping is not supported yet."); diff --git a/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java b/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java index bd7b7106809..bd7661ac818 100644 --- a/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java +++ b/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java @@ -27,18 +27,11 @@ import java.util.List; public class AttributeNode implements ExpressionNode { private ExpressionNode value; - private final List<Item> items = new ArrayList<>(); + private final List<Item> items; - public AttributeNode(ExpressionNode value, List items) { + public AttributeNode(ExpressionNode value, List<Item> items) { this.value = value; - for (Object obj : items) { - if (obj instanceof Item) { - this.items.add((Item)obj); - } else { - throw new IllegalStateException("Can not add an instance of " + obj.getClass().getName() + - " as a function item."); - } - } + this.items = new ArrayList<>(items); } public ExpressionNode getValue() { @@ -99,12 +92,14 @@ public class AttributeNode implements ExpressionNode { } static class IteratorHandler extends FieldPathIteratorHandler { + VariableValueList values = new VariableValueList(); @Override public void onPrimitive(FieldValue fv) { values.add(new ResultList.VariableValue((VariableMap)getVariables().clone(), fv)); } + } private static Object applyFunction(String function, Object value) { @@ -153,7 +148,7 @@ public class AttributeNode implements ExpressionNode { private static Object evaluateFieldPath(String fieldPathStr, Object value) { if (value instanceof DocumentPut) { - final Document doc = ((DocumentPut) value).getDocument(); + Document doc = ((DocumentPut) value).getDocument(); if (isSimpleImportedField(fieldPathStr, doc.getDataType())) { // Imported fields can only be meaningfully evaluated in the backend, so we // explicitly treat them as if they are valid fields with missing values. This @@ -211,6 +206,7 @@ public class AttributeNode implements ExpressionNode { } public static class Item { + public static final int ATTRIBUTE = 0; public static final int FUNCTION = 1; @@ -242,5 +238,7 @@ public class AttributeNode implements ExpressionNode { @Override public String toString() { return name + (type == FUNCTION ? "()" : ""); } + } + } diff --git a/document/src/main/java/com/yahoo/document/select/rule/DocumentNode.java b/document/src/main/java/com/yahoo/document/select/rule/DocumentNode.java index 8022adf3fbe..c42e632cc88 100644 --- a/document/src/main/java/com/yahoo/document/select/rule/DocumentNode.java +++ b/document/src/main/java/com/yahoo/document/select/rule/DocumentNode.java @@ -13,7 +13,12 @@ import com.yahoo.document.select.Context; import com.yahoo.document.select.Visitor; /** + * A document node which returns a document: For accessing document field data in AttributeNode, + * where it should be possible to access fields both by the concrete type ("concreteType.fieldName") + * and by parent type ("inheritedType.inheritedField"). + * * @author Simon Thoresen Hult + * @author bratseth */ public class DocumentNode implements ExpressionNode { @@ -42,8 +47,25 @@ public class DocumentNode implements ExpressionNode { return evaluate(context.getDocumentOperation()); } - public Object evaluate(DocumentOperation op) { - return op.getId().getDocType().equals(type) ? op : false; + private Object evaluate(DocumentOperation op) { + if (hasData(op)) + return evaluateForDataLookup(op); + else // Simplify to just false since we can't progress here? + return op.getId().getDocType().equals(type) ? op : false; + } + + private Object evaluateForDataLookup(DocumentOperation op) { + if (op instanceof DocumentPut) + return ((DocumentPut)op).getDocument().getDataType().isA(this.type) ? op : false; + else if (op instanceof DocumentUpdate) + return ((DocumentUpdate)op).getDocumentType().isA(this.type) ? op : false; + else + throw new IllegalStateException("Programming error"); + } + + /** Returns whether this operation is of a type that may contain data */ + private boolean hasData(DocumentOperation op) { + return op instanceof DocumentPut || op instanceof DocumentUpdate; } public void accept(Visitor visitor) { diff --git a/document/src/main/java/com/yahoo/document/select/rule/DocumentTypeNode.java b/document/src/main/java/com/yahoo/document/select/rule/DocumentTypeNode.java new file mode 100644 index 00000000000..7e04ec6fa1b --- /dev/null +++ b/document/src/main/java/com/yahoo/document/select/rule/DocumentTypeNode.java @@ -0,0 +1,58 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.document.select.rule; + +import com.yahoo.document.BucketIdFactory; +import com.yahoo.document.DocumentOperation; +import com.yahoo.document.DocumentPut; +import com.yahoo.document.DocumentUpdate; +import com.yahoo.document.select.BucketSet; +import com.yahoo.document.select.Context; +import com.yahoo.document.select.Visitor; + +/** + * A document type node which returns the document type if exactly the type sopecified or false otherwise: + * For using the exact document type as a condition. + * + * @author bratseth + */ +public class DocumentTypeNode implements ExpressionNode { + + private String type; + + public DocumentTypeNode(String type) { + this.type = type; + } + + public String getType() { + return type; + } + + public DocumentTypeNode setType(String type) { + this.type = type; + return this; + } + + @Override + public BucketSet getBucketSet(BucketIdFactory factory) { + return null; + } + + @Override + public Object evaluate(Context context) { + return evaluate(context.getDocumentOperation()); + } + + private Object evaluate(DocumentOperation op) { + return op.getId().getDocType().equals(type) ? op : false; + } + + public void accept(Visitor visitor) { + visitor.visit(this); + } + + @Override + public String toString() { + return type; + } + +} diff --git a/document/src/main/java/com/yahoo/document/select/rule/ExpressionNode.java b/document/src/main/java/com/yahoo/document/select/rule/ExpressionNode.java index 2da82abc614..1c0b009396d 100644 --- a/document/src/main/java/com/yahoo/document/select/rule/ExpressionNode.java +++ b/document/src/main/java/com/yahoo/document/select/rule/ExpressionNode.java @@ -7,7 +7,7 @@ import com.yahoo.document.select.Context; import com.yahoo.document.select.Visitor; /** - * This is the interface of all expression nodes. It declares the methods requires by all expression nodes to maintain + * The interface of all expression nodes. It declares the methods requires by all expression nodes to maintain * a working document selector language. * * @author Simon Thoresen Hult @@ -17,22 +17,22 @@ public interface ExpressionNode { /** * Evaluate the content of this node based on document object, and return that value. * - * @param doc the document to evaluate over. - * @return the value of this. + * @param doc the document to evaluate over + * @return the value of this */ Object evaluate(Context doc); /** * Returns the set of bucket ids covered by this node. * - * @param factory the factory used by the current application. + * @param factory the factory used by the current application */ BucketSet getBucketSet(BucketIdFactory factory); /** * Perform visitation of this node. * - * @param visitor the visitor that wishes to visit the node. + * @param visitor the visitor that wishes to visit the node */ void accept(Visitor visitor); diff --git a/document/src/main/javacc/SelectParser.jj b/document/src/main/javacc/SelectParser.jj index 4df6fd99020..b6a7e21133c 100755 --- a/document/src/main/javacc/SelectParser.jj +++ b/document/src/main/javacc/SelectParser.jj @@ -159,15 +159,23 @@ VariableNode variable() : ExpressionNode attribute() : { - ExpressionNode val; + ExpressionNode value; AttributeNode.Item item; - List lst = new ArrayList(); + List items = new ArrayList(); } { - ( val = value() ( <DOT> identifier() { item = new AttributeNode.Item(token.image); } - [ <LBRACE> <RBRACE> { item.setType(AttributeNode.Item.FUNCTION); } ] - { lst.add(item); } )* ) - { return lst.size() > 0 ? new AttributeNode(val, lst) : val; } + ( value = value() ( <DOT> identifier() { item = new AttributeNode.Item(token.image); } + [ <LBRACE> <RBRACE> { item.setType(AttributeNode.Item.FUNCTION); } ] + { items.add(item); } )* ) + { + if ( ! items.isEmpty()) // A value followed by dotted access/method call + return new AttributeNode(value, items); + + if (value instanceof DocumentNode) // A standalone document type access: Convert to exact type matching + return new DocumentTypeNode(((DocumentNode)value).getType()); + + return value; + } } ExpressionNode value() : diff --git a/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java b/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java index 74b603cead5..690b216e5e2 100644 --- a/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java +++ b/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java @@ -746,8 +746,14 @@ public class DocumentSelectorTestCase { @Test public void testInheritance() throws ParseException { + var s=new DocumentSelector("parent.parentField = \"parentValue\""); List<DocumentPut> documents = createDocs(); + assertEquals(Result.TRUE, evaluate("test", documents.get(0))); + assertEquals("Matching on type is exact", + Result.FALSE, evaluate("parent", documents.get(0))); assertEquals(Result.TRUE, evaluate("test.parentField = \"parentValue\"", documents.get(0))); + assertEquals("Fields may be accessed by parent type", + Result.TRUE, evaluate("parent.parentField = \"parentValue\"", documents.get(0))); } @Test diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentRouteSelectorPolicy.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentRouteSelectorPolicy.java index 7422a303ce9..67ebe974e4e 100755 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentRouteSelectorPolicy.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentRouteSelectorPolicy.java @@ -39,10 +39,8 @@ public class DocumentRouteSelectorPolicy selectors.put(name, new DocumentSelector(cluster.selector())); } catch (ParseException e) { - throw new IllegalArgumentException("Error parsing selector '" + - cluster.selector() + - "' for route '" + name +"'", - e); + throw new IllegalArgumentException("Error parsing selector '" + cluster.selector() + + "' for route '" + name +"'", e); } }); this.config = Map.copyOf(selectors); @@ -87,9 +85,9 @@ public class DocumentRouteSelectorPolicy DocumentSelector selector; try { selector = new DocumentSelector(route.selector()); - log.log(Level.CONFIG, "Selector for route '" + route.name() + "' is '" + selector + "'."); + log.log(Level.CONFIG, "Selector for route '" + route.name() + "' is '" + selector + "'"); } catch (com.yahoo.document.select.parser.ParseException e) { - error = "Error parsing selector '" + route.selector() + "' for route '" + route.name() + "; " + + error = "Error parsing selector '" + route.selector() + "' for route '" + route.name() + ": " + e.getMessage(); break; } |