Corrects duplicate error messages

Duplicate error message triggered by scalar evaluation when part od collection or user-defined types. Also fixes grammar identifiers.

Change-Id: Id862a40ae6855ff9f72176a1bdf3815d3b31e5a4
Issues-Id: MODELING-15
Signed-off-by: Serban Jora <jora@research.att.com>

4 files changed, 102 insertions, 51 deletions

diff --git a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Checker.java b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Checker.java
index 9991c86..5cc8233 100644
--- a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Checker.java
+++ b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Checker.java
@@ -131,7 +131,7 @@ public class Checker {
   private Target   						target = null; //what we're validating at the moment
 	private	Map<String, Target>	grammars = new HashMap<String, Target>(); //grammars for the different tosca versions
 
-	private CheckerConfiguration config = new CheckerConfiguration();	
+	private CheckerConfiguration config;
 	private Catalog   		catalog;
 	private	TargetLocator	locator = new CommonLocator();
 
@@ -139,22 +139,26 @@ public class Checker {
 	private Messages											messages;
 	private Logger    log = Logger.getLogger(Checker.class.getName());
 
+
 	private static String[] EMPTY_STRING_ARRAY = new String[0];
+	private static final	String[] grammarFiles = new String[] {"tosca/tosca_simple_yaml_1_0.grammar",
+																															"tosca/tosca_simple_yaml_1_1.grammar"};	
 
 
-  public Checker() throws CheckerException {
+  public Checker(CheckerConfiguration theConfig) throws CheckerException {
+		this.config = theConfig;
     loadGrammars();
 		loadAnnotations();
 		messages = new Messages();
   }
 
-	/* Need a proper way to indicate where the grammars are and how they should be identified
-	 */
-	private final	String[] grammarFiles = new String[] {"tosca/tosca_simple_yaml_1_0.grammar",
-																											"tosca/tosca_simple_yaml_1_1.grammar"};	
+  public Checker() throws CheckerException {
+		this(new CheckerConfiguration());
+	}
+
 	private void loadGrammars() throws CheckerException {
 
-		for (String grammarFile: grammarFiles) {
+		for (String grammarFile: this.config.grammars()) {
 			Target grammarTarget = this.locator.resolve(grammarFile);
 			if (grammarTarget == null) {
 				log.warning("Failed to locate grammar " + grammarFile);
@@ -181,7 +185,7 @@ public class Checker {
 				log.warning("Invalid grammar " + grammarFile + ": cannot locate tosca_definitions_versions");
 			}
 			if (versions == null || versions.isEmpty()) {
-				log.warning("Invalid grammar " + grammarFile + ": no tosca_definitions_versions specified");
+				log.warning("Invalid grammar " + grammarFile + ": no tosca_definitions_version specified");
 				continue;
 			}
 
@@ -1593,8 +1597,8 @@ available and the available targets won't be re-processed.
   
 	//topology_template_definition and sub-rules
 	/* */
-	@Checks(path="/topology_template")
-	protected void check_topology_template(
+	@Catalogs(path="/topology_template")
+	protected void catalog_topology_template(
 										Map<String,Map> theDef, final CheckContext theContext) {
     
 		theContext.enter("topology_template");
@@ -1602,22 +1606,32 @@ available and the available targets won't be re-processed.
 		try {		
 			theDef.entrySet().stream()
 				.forEach(e ->	catalogs(e.getKey(), e.getValue(), theContext));
+		}
+		finally {
+			theContext.exit();
+		}
+  }
+
+	/** */
+	@Checks(path="/topology_template")
+	protected void check_topology_template(
+										Map<String,Map> theDef, final CheckContext theContext) {
+    
+		theContext.enter("topology_template");
+
+		try {		
+		//	theDef.entrySet().stream()
+		//		.forEach(e ->	catalogs(e.getKey(), e.getValue(), theContext));
 		
 			theDef.entrySet().stream()
 				.forEach(e ->	checks(e.getKey(), e.getValue(), theContext));
-/*		
-		for (Iterator<Map.Entry<String,Object>> ri = theDef.entrySet().iterator();
-		     ri.hasNext(); ) {
-			Map.Entry<String,Object> e = ri.next();
-			checks(e.getKey(), e.getValue(), theContext);
-		}
-*/
 		}
 		finally {
 			theContext.exit();
 		}
   }
 
+
   /*
    * Once the syntax of the imports section is validated parse/validate/catalog    * all the imported template information
    */
@@ -1806,7 +1820,7 @@ available and the available targets won't be re-processed.
 		}
 	}
   
-	@Checks(path="topology_template/outputs")
+	@Checks(path="/topology_template/outputs")
 	protected void check_outputs(Map<String, Map> theOutputs,
 	                            CheckContext theContext) {
     theContext.enter("outputs");
@@ -1834,7 +1848,12 @@ available and the available targets won't be re-processed.
 			    !checkDefinition(theName, theDef, theContext)) {
 			  return;
 			}
-			//check the expression
+      
+      //check value
+			Object value = theDef.get("value");
+			if (value != null) {
+				checkDataValuation(value, theDef, theContext);
+			}
 		}
 		finally {
 		  theContext.exit();
@@ -3444,7 +3463,7 @@ System.out.println(" **** unassigned -> " + unassignedEntry.getKey() + " : " + u
 	 * @return true if any handler returned true (if they returned something at all), false otherwise (even when no
    * handlers were found)
 	 */
-	protected boolean handles(String theHandlerKey, Object... theArgs) {
+	protected boolean handles(String theHandlerKey,	Object... theArgs) {
 
 		boolean handled = false;
 		Map<Method, Object> entries = handlers.row(theHandlerKey);
@@ -3455,7 +3474,7 @@ System.out.println(" **** unassigned -> " + unassignedEntry.getKey() + " : " + u
 					res = entry.getKey().invoke(entry.getValue(), theArgs);
 				}
 				catch (Exception x) {
-        	log.log(Level.WARNING, theHandlerKey + " by " + entry.getKey() + " failed", x);
+       		log.log(Level.WARNING, theHandlerKey + " by " + entry.getKey() + " failed", x);
 				}
 				handled |= res == null ? false : (res instanceof Boolean && ((Boolean)res).booleanValue());
 			}	
@@ -3630,11 +3649,12 @@ System.out.println(" **** unassigned -> " + unassignedEntry.getKey() + " : " + u
 
 	public static class CheckerConfiguration {
 
-		private boolean 	allowAugmentation = true;
-		private String		defaultImportsPath = null;
-		private String		defaultCheckerRoots = null;
+		private boolean 			allowAugmentation = true;
+		private String				defaultImportsPath = null;
+		private List<String>	grammars = new LinkedList<String>();
 
 		protected CheckerConfiguration() {
+			withGrammars(Checker.grammarFiles);
 		}
 
 		public CheckerConfiguration allowAugmentation(boolean doAllow) {
@@ -3646,7 +3666,7 @@ System.out.println(" **** unassigned -> " + unassignedEntry.getKey() + " : " + u
 			return this.allowAugmentation;
 		}
 
-		public CheckerConfiguration defaultImportsPath(String thePath) {
+		public CheckerConfiguration withDefaultImportsPath(String thePath) {
 			this.defaultImportsPath = thePath;
 			return this;
 		}
@@ -3655,6 +3675,16 @@ System.out.println(" **** unassigned -> " + unassignedEntry.getKey() + " : " + u
 			return this.defaultImportsPath;
 		}
 
+		public CheckerConfiguration withGrammars(String... theGrammars) {
+			for (String grammar: theGrammars) {
+				this.grammars.add(grammar);
+			}
+			return this;
+		}
+
+		public String[] grammars() {
+			return this.grammars.toArray(new String[0]);
+		}
 	}
 
 }
diff --git a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Data.java b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Data.java
index fc29dcf..9aab6ca 100644
--- a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Data.java
+++ b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/Data.java
@@ -79,6 +79,8 @@ public class Data {
 		
 		public String name();
 
+		public boolean isScalar();
+
 		public Evaluator evaluator();
 
 		public Evaluator constraintsEvaluator();
@@ -92,6 +94,10 @@ public class Data {
 			return null;
 		}
 
+		public boolean isScalar() {
+			return false;
+		}
+
 		public Evaluator evaluator() {
 			return Data::evalUser;
 		}
@@ -99,56 +105,65 @@ public class Data {
 		public Evaluator constraintsEvaluator() {
 			return Data::evalUserConstraints;
 		}
+		
+		public String toString() {
+			return "UserType(scalar=false)";
+		}
 	};
 
 
 	public static enum CoreType implements Type {
 
-    String("string",
+    String("string", true,
 					(expr,def,ctx) -> expr != null && expr instanceof String,
 					Data::evalScalarConstraints),
-    Integer("integer",
+    Integer("integer", true,
 					(expr,def,ctx) -> Data.valueOf(ctx, expr, Integer.class),
 					Data::evalScalarConstraints),
-		Float("float",
+		Float("float", true,
 					(expr,def,ctx) -> Data.valueOf(ctx, expr, Double.class, Integer.class),
 					Data::evalScalarConstraints),
- 	  Boolean("boolean",
+ 	  Boolean("boolean", true,
 					(expr,def,ctx) -> Data.valueOf(ctx, expr, Boolean.class),
 					Data::evalScalarConstraints),
- 	  Null("null",
+ 	  Null("null", true,
 					(expr,def,ctx) -> expr.equals("null"),
 					null),
-   	Timestamp("timestamp",
+   	Timestamp("timestamp", true,
 					(expr,def,ctx) -> timestampRegex.matcher(expr.toString()).matches(),
 					null),
-    List("list", Data::evalList, Data::evalListConstraints),
-    Map("map", Data::evalMap, Data::evalMapConstraints),
-		Version("version",
+    List("list", false, Data::evalList, Data::evalListConstraints),
+    Map("map", false, Data::evalMap, Data::evalMapConstraints),
+		Version("version", true,
 					(expr,def,ctx) -> versionRegex.matcher(expr.toString()).matches(),
 					null),
 		/* use a scanner and check that the upper bound is indeed greater than
-		 * the lower bound */
-    Range("range",
+		 * the lower bound. Marked as a scalar because its evaluator does not record a context error */
+    Range("range", true,
 					(expr,def,ctx) -> { return rangeRegex.matcher(expr.toString()).matches();},
 					null ),
-    Size("scalar-unit.size",
+    Size("scalar-unit.size", true,
 				 (expr,def,ctx) -> sizeRegex.matcher(expr.toString()).matches(),
 					null),
-    Time("scalar-unit.time",
+    Time("scalar-unit.time", true,
 				 (expr,def,ctx) -> timeRegex.matcher(expr.toString()).matches(),
 					null),
-    Frequency("scalar-unit.frequency",
+    Frequency("scalar-unit.frequency", true,
 				 (expr,def,ctx) -> frequencyRegex.matcher(expr.toString()).matches(),
 					null);
 
 
 		private String 		toscaName;
+		private boolean		isScalar;
 		private Evaluator	valueEvaluator,
 											constraintsEvaluator;
 
-		private CoreType(String theName, Evaluator theValueEvaluator, Evaluator theConstraintsEvaluator) {
+		private CoreType(String theName,
+										 boolean theIsScalar,
+										 Evaluator theValueEvaluator,
+										 Evaluator theConstraintsEvaluator) {
 			this.toscaName = theName;
+			this.isScalar = theIsScalar;
 			this.valueEvaluator = theValueEvaluator;
 			this.constraintsEvaluator = theConstraintsEvaluator;
 
@@ -162,6 +177,10 @@ public class Data {
 			return this.toscaName;
 		}
 
+		public boolean isScalar() {
+			return this.isScalar;
+		}
+
 		public Evaluator evaluator() {
 			return this.valueEvaluator;
 		}
@@ -169,6 +188,7 @@ public class Data {
 		public Evaluator constraintsEvaluator() {
 			return this.constraintsEvaluator;
 		}
+
 	}
 
 	private static Pattern timestampRegex = null,
@@ -338,8 +358,9 @@ public class Data {
 			if (entryEvaluator != null &&
 					!entryEvaluator.eval(val, entryTypeDef, theCtx)) {
 				res= false;
-				//the constraints evaluator should have already added an error, but it also adds some context 
-				//theCtx.addError("Value " + val + " failed evaluation", null);
+				if (entryType.isScalar()) {
+					theCtx.addError("Value " + val + " failed constraints evaluation", null);
+				}
 			}
 		}
 		return res;
@@ -366,15 +387,16 @@ public class Data {
 			Map propDef = (Map)propEntry.getValue();
 			Object propVal = val.get(propEntry.getKey());
 
-//System.out.println("evalUser: " + propVal);
-
 			if (propVal != null) {
 				Data.Type propType = typeByName((String)propDef.get("type"));
 
+//System.out.println("evalUser: " + propVal + " of type " + propType + "/" + propType.isScalar());
+
 				if (!propType.evaluator().eval(propVal, propDef, theCtx)) {
 					res= false;
-					//the constraints evaluator should have already added an error
-					//theCtx.addError("Property " + propEntry.getKey() + " failed evaluation for " + propVal, null);
+					if (propType.isScalar()) {
+						theCtx.addError("Property " + propEntry.getKey() + " failed evaluation for " + propVal + " as " + propType.name(), null);
+					}
 				}
 			}
 		}
diff --git a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/annotations/Checks.java b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/annotations/Checks.java
index 856ac1b..54e5685 100644
--- a/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/annotations/Checks.java
+++ b/javatoscachecker/checker/src/main/java/org/onap/tosca/checker/annotations/Checks.java
@@ -38,5 +38,6 @@ import java.lang.annotation.Target;
  */
 public @interface Checks {
   String path() default "/";
-	String[] version() default { "1.0", "1.0.0", "1.1", "1.1.0" };
+	String[] version() default { "tosca_simple_yaml_1_0", "tosca_simple_yaml_1_0_0",
+															 "tosca_simple_yaml_1_1", "tosca_simple_yaml_1_1_0" };
 }
diff --git a/javatoscachecker/checker/src/main/resources/tosca/tosca_simple_yaml_1_1.grammar b/javatoscachecker/checker/src/main/resources/tosca/tosca_simple_yaml_1_1.grammar
index e199680..39b11d9 100644
--- a/javatoscachecker/checker/src/main/resources/tosca/tosca_simple_yaml_1_1.grammar
+++ b/javatoscachecker/checker/src/main/resources/tosca/tosca_simple_yaml_1_1.grammar
@@ -286,8 +286,6 @@ _property_definition: &property_definition
 #      type: str
 #      required: no
 
-#see section A.5.8
-#_property_assignment_definition: &property_assignment_definition
 
 #see 3.5.10
 _attribute_definition: &attribute_definition
@@ -1407,7 +1405,7 @@ _topology_template_definition: &topology_template_definition
       type: map
       mapping:
         =:
-          *property_definition
+          *parameter_definition
     node_templates:
       desc: "definition of the node templates of the topology"
       name: node_templates
@@ -1431,7 +1429,7 @@ _topology_template_definition: &topology_template_definition
       type: map
       mapping:
         =:
-          *attribute_assignment_definition
+          *parameter_definition
     groups:
       desc: "An optional list of Group definitions whose members are node templates defined within this same Topology Template"
       name: groups