Merge pull request #329 from dawagner/xmlgeneration-check-conflicting-elements

krocard · krocard · commit 7ab5e9574e3e · 2016-01-12T15:53:56.000+01:00
Xml generation: check conflicting elements

Conflicting elements are elements belonging to more than one domain.
The behaviour in such a case is unspecified.

In the context of XML generation from user input, this denotes a mistake from the user.
We now warn him and fail the generation.
diff --git a/parameter/ConfigurableDomain.cpp b/parameter/ConfigurableDomain.cpp
@@ -686,8 +686,6 @@ bool CConfigurableDomain::deleteConfiguration(const string &strName, string &str
 
 void CConfigurableDomain::listAssociatedToElements(string &strResult) const
 {
-    strResult = "\n";
-
     ConfigurableElementListIterator it;
 
     // Browse all configurable elements
diff --git a/parameter/ConfigurableDomains.cpp b/parameter/ConfigurableDomains.cpp
@@ -317,8 +317,6 @@ bool CConfigurableDomains::split(const string &domainName, CConfigurableElement
 
 void CConfigurableDomains::listAssociatedElements(string &strResult) const
 {
-    strResult = "\n";
-
     std::set<const CConfigurableElement *> configurableElementSet;
 
     // Get all owned configurable elements
@@ -341,8 +339,6 @@ void CConfigurableDomains::listAssociatedElements(string &strResult) const
 
 void CConfigurableDomains::listConflictingElements(string &strResult) const
 {
-    strResult = "\n";
-
     std::set<const CConfigurableElement *> configurableElementSet;
 
     // Get all owned configurable elements
@@ -369,8 +365,6 @@ void CConfigurableDomains::listConflictingElements(string &strResult) const
 
 void CConfigurableDomains::listDomains(string &strResult) const
 {
-    strResult = "\n";
-
     // List domains
     size_t uiNbConfigurableDomains = getNbChildren();
 
diff --git a/parameter/ConfigurableElement.cpp b/parameter/ConfigurableElement.cpp
@@ -468,8 +468,6 @@ void CConfigurableElement::listBelongingDomains(std::string &strResult, bool bVe
 // Elements with no domains
 void CConfigurableElement::listRogueElements(std::string &strResult) const
 {
-    strResult = "\n";
-
     // Get rogue element aggregate list (no associated domain)
     std::list<const CConfigurableElement *> rogueElementList;
 
@@ -568,11 +566,6 @@ void CConfigurableElement::listDomains(
     const std::list<const CConfigurableDomain *> &configurableDomainList, std::string &strResult,
     bool bVertical) const
 {
-    if (bVertical && configurableDomainList.empty()) {
-
-        strResult = "\n";
-    }
-
     // Fill list
     ConfigurableDomainListConstIterator it;
     bool bFirst = true;
diff --git a/parameter/Element.cpp b/parameter/Element.cpp
@@ -122,7 +122,6 @@ string CElement::dumpContent(utility::ErrorContext &errorContext, const size_t d
 // Element properties
 void CElement::showProperties(string &strResult) const
 {
-    strResult = "\n";
     strResult += "Kind: " + getKind() + "\n";
     showDescriptionProperty(strResult);
 }
@@ -330,8 +329,6 @@ bool CElement::removeChild(CElement *pChild)
 
 void CElement::listChildren(string &strChildList) const
 {
-    strChildList = "\n";
-
     // Get list of children names
     for (CElement *pChild : _childArray) {
 
diff --git a/parameter/ParameterMgr.cpp b/parameter/ParameterMgr.cpp
@@ -1370,8 +1370,6 @@ CParameterMgr::CCommandHandler::CommandStatus CParameterMgr::listElementsCommand
         return CCommandHandler::EFailed;
     }
 
-    strResult = string("\n");
-
     if (!pLocatedElement) {
 
         // List from root folder
@@ -1399,8 +1397,6 @@ CParameterMgr::CCommandHandler::CommandStatus CParameterMgr::listParametersComma
         return CCommandHandler::EFailed;
     }
 
-    strResult = string("\n");
-
     if (!pLocatedElement) {
 
         // List from root folder
diff --git a/remote-processor/RemoteCommandHandlerTemplate.h b/remote-processor/RemoteCommandHandlerTemplate.h
@@ -194,8 +194,6 @@ class TRemoteCommandHandlerTemplate : public IRemoteCommandHandler
     {
         initMaxCommandUsageLength();
 
-        strResult = "\n";
-
         // Show usages
         for (const auto *pRemoteCommandParserItem : _remoteCommandParserVector) {
 
diff --git a/test/xml-generator/PFConfig/duplicate_criterion_value.txt b/test/xml-generator/PFConfig/duplicate_criterion_value.txt
@@ -0,0 +1 @@
+InclusiveCriterion Duplicate : Foo Foo
diff --git a/test/xml-generator/PFConfig/structure.xml b/test/xml-generator/PFConfig/structure.xml
@@ -10,6 +10,9 @@
         <InstanceDefinition>
 
             <Component Name="included" Type="Included"/>
+            <ParameterBlock Name="inline">
+                <BooleanParameter Name="bool"/>
+            </ParameterBlock>
 
             <ParameterBlock Name="block" ArrayLength="5">
                 <FixedPointParameter Name="q2.5" Size="8" Integral="2" Fractional="5"/>
diff --git a/test/xml-generator/test.py b/test/xml-generator/test.py
@@ -90,27 +90,42 @@ def check(self, reference=None, expectedErrors=0):
 config_dir = os.path.join(basedir, "PFConfig")
 vector_dir = os.path.join(basedir, "testVector")
 class TestCase(unittest.TestCase):
-    nominal_reference = open(os.path.join(vector_dir, "reference.xml")).read().splitlines()
-    nominal_pfconfig = PfConfig(os.path.join(config_dir, "configuration.xml"),
-                                os.path.join(config_dir, "criteria.txt"),
-                                os.path.join(basedir, "../../schemas"))
-    nominal_vector = TestVector(os.path.join(vector_dir, "initialSettings.xml"),
-                                [os.path.join(vector_dir, "first.pfw"),
-                                 os.path.join(vector_dir, "second.pfw"),
-                                 os.path.join(vector_dir, "complex.pfw")],
-                                [os.path.join(vector_dir, "third.xml"),
-                                 os.path.join(vector_dir, "fourth.xml")])
+    def setUp(self):
+        self.nominal_reference = open(os.path.join(vector_dir, "reference.xml")).read().splitlines()
+        self.nominal_pfconfig = PfConfig(os.path.join(config_dir, "configuration.xml"),
+                                         os.path.join(config_dir, "criteria.txt"),
+                                         os.path.join(basedir, "../../schemas"))
+        self.nominal_vector = TestVector(os.path.join(vector_dir, "initialSettings.xml"),
+                                         [os.path.join(vector_dir, "first.pfw"),
+                                          os.path.join(vector_dir, "second.pfw"),
+                                          os.path.join(vector_dir, "complex.pfw")],
+                                         [os.path.join(vector_dir, "third.xml"),
+                                          os.path.join(vector_dir, "fourth.xml")])
 
     def test_nominal(self):
         tester = Tester(self.nominal_pfconfig, self.nominal_vector)
         tester.check(self.nominal_reference)
 
     def test_nonfatalError(self):
-        vector = copy.copy(self.nominal_vector)
-        vector.edds.append(os.path.join(vector_dir, "duplicate.pfw"))
+        self.nominal_vector.edds.append(os.path.join(vector_dir, "duplicate.pfw"))
 
-        tester = Tester(self.nominal_pfconfig, vector)
+        tester = Tester(self.nominal_pfconfig, self.nominal_vector)
         tester.check(self.nominal_reference, expectedErrors=1)
 
+    def test_conflicting(self):
+        vector = TestVector(edds=[os.path.join(vector_dir, "conflicting.pfw")])
+
+        tester = Tester(self.nominal_pfconfig, vector)
+        tester.check(expectedErrors=1)
+
+    def test_fail_criteria(self):
+        self.nominal_pfconfig.criteria = os.path.join(config_dir, "duplicate_criterion_value.txt")
+        # Empty test vector: we want to make sure that the erroneous criterion
+        # is the only source of error
+        empty_vector = TestVector()
+
+        tester = Tester(self.nominal_pfconfig, empty_vector)
+        tester.check(expectedErrors=1)
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/test/xml-generator/testVector/complex.pfw b/test/xml-generator/testVector/complex.pfw
@@ -33,8 +33,8 @@ domainGroup: Red sequenceAware
 						Colors Includes Blue
 
 						component: /Test/test
-							component: block/3
+							component: block/0
 								q2.5 = 1.18750
 								string = 12 ab @ 	 <![CDATA[ < > \n \0 ✔ "'\
-							component: included
+							component: inline
 								bool = 1
diff --git a/test/xml-generator/testVector/conflicting.pfw b/test/xml-generator/testVector/conflicting.pfw
@@ -0,0 +1,7 @@
+# Causes a conflicting-element error
+domain: First
+	conf: Default
+		/Test/test/included/bool = 1
+domain: Second
+	conf: Default
+		/Test/test/included/bool = 0
diff --git a/test/xml-generator/testVector/reference.xml b/test/xml-generator/testVector/reference.xml
@@ -200,19 +200,19 @@
       </Configuration>
     </Configurations>
     <ConfigurableElements>
-      <ConfigurableElement Path="/Test/test/block/3/q2.5"/>
-      <ConfigurableElement Path="/Test/test/block/3/string"/>
-      <ConfigurableElement Path="/Test/test/included/bool"/>
+      <ConfigurableElement Path="/Test/test/block/0/q2.5"/>
+      <ConfigurableElement Path="/Test/test/block/0/string"/>
+      <ConfigurableElement Path="/Test/test/inline/bool"/>
     </ConfigurableElements>
     <Settings>
       <Configuration Name="green.confGroup..On.Blue.dot">
-        <ConfigurableElement Path="/Test/test/block/3/q2.5">
+        <ConfigurableElement Path="/Test/test/block/0/q2.5">
           <FixedPointParameter Name="q2.5">1.18750</FixedPointParameter>
         </ConfigurableElement>
-        <ConfigurableElement Path="/Test/test/block/3/string">
+        <ConfigurableElement Path="/Test/test/block/0/string">
           <StringParameter Name="string">12 ab @ 	 &lt;![CDATA[ &lt; &gt; \n \0 ✔ "'\</StringParameter>
         </ConfigurableElement>
-        <ConfigurableElement Path="/Test/test/included/bool">
+        <ConfigurableElement Path="/Test/test/inline/bool">
           <BooleanParameter Name="bool">1</BooleanParameter>
         </ConfigurableElement>
       </Configuration>
diff --git a/tools/xmlGenerator/domainGeneratorConnector.cpp b/tools/xmlGenerator/domainGeneratorConnector.cpp
@@ -92,6 +92,14 @@ class XmlGenerator
      */
     size_t parse(std::istream &input);
 
+    /** Check for elements belonging to several domains
+     *
+     * Prints conflicting elements, if any, on the error output.
+     *
+     * @returns true if there are conflicting elements, false otherwise
+     */
+    bool conflictingElements();
+
     /** Prints the Parameter Framework's instance configuration
      *
      * @param[out] output The stream to which output the configuration
@@ -182,6 +190,22 @@ size_t XmlGenerator::parse(std::istream &input)
     return errorNb;
 }
 
+bool XmlGenerator::conflictingElements()
+{
+    string conflicting;
+    if (not mCommandHandler->process("listConflictingElements", {}, conflicting)) {
+        // Should not happen
+        throw Exception("Failed to list conflicting elements");
+    }
+
+    if (not conflicting.empty()) {
+        std::cerr << "There are conflicting elements:" << std::endl << conflicting;
+        return true;
+    }
+
+    return false;
+}
+
 void XmlGenerator::start()
 {
     string error;
@@ -257,7 +281,9 @@ int main(int argc, char *argv[])
     try {
         XmlGenerator xmlGenerator(toplevelConfig, validate, verbose, schemasDir);
         auto errorNb = xmlGenerator.parse(std::cin);
-        // TODO: add a check for conflicting elements
+        if (xmlGenerator.conflictingElements()) {
+            errorNb++;
+        }
         xmlGenerator.exportDomains(std::cout);
 
         return normalizeExitCode(errorNb);

Original file line number	Diff line number	Diff line change
`@@ -686,8 +686,6 @@ bool CConfigurableDomain::deleteConfiguration(const string &strName, string &str`
`686`	`686`
`687`	`687`	`void CConfigurableDomain::listAssociatedToElements(string &strResult) const`
`688`	`688`	`{`
`689`		`- strResult = "\n";`
`690`		`-`
`691`	`689`	`ConfigurableElementListIterator it;`
`692`	`690`
`693`	`691`	`// Browse all configurable elements`
Original file line number	Diff line number	Diff line change
`@@ -317,8 +317,6 @@ bool CConfigurableDomains::split(const string &domainName, CConfigurableElement`
`317`	`317`
`318`	`318`	`void CConfigurableDomains::listAssociatedElements(string &strResult) const`
`319`	`319`	`{`
`320`		`- strResult = "\n";`
`321`		`-`
`322`	`320`	`std::set<const CConfigurableElement *> configurableElementSet;`
`323`	`321`
`324`	`322`	`// Get all owned configurable elements`
`@@ -341,8 +339,6 @@ void CConfigurableDomains::listAssociatedElements(string &strResult) const`
`341`	`339`
`342`	`340`	`void CConfigurableDomains::listConflictingElements(string &strResult) const`
`343`	`341`	`{`
`344`		`- strResult = "\n";`
`345`		`-`
`346`	`342`	`std::set<const CConfigurableElement *> configurableElementSet;`
`347`	`343`
`348`	`344`	`// Get all owned configurable elements`
`@@ -369,8 +365,6 @@ void CConfigurableDomains::listConflictingElements(string &strResult) const`
`369`	`365`
`370`	`366`	`void CConfigurableDomains::listDomains(string &strResult) const`
`371`	`367`	`{`
`372`		`- strResult = "\n";`
`373`		`-`
`374`	`368`	`// List domains`
`375`	`369`	`size_t uiNbConfigurableDomains = getNbChildren();`
`376`	`370`
Original file line number	Diff line number	Diff line change
`@@ -122,7 +122,6 @@ string CElement::dumpContent(utility::ErrorContext &errorContext, const size_t d`
`122`	`122`	`// Element properties`
`123`	`123`	`void CElement::showProperties(string &strResult) const`
`124`	`124`	`{`
`125`		`- strResult = "\n";`
`126`	`125`	`strResult += "Kind: " + getKind() + "\n";`
`127`	`126`	`showDescriptionProperty(strResult);`
`128`	`127`	`}`
`@@ -330,8 +329,6 @@ bool CElement::removeChild(CElement *pChild)`
`330`	`329`
`331`	`330`	`void CElement::listChildren(string &strChildList) const`
`332`	`331`	`{`
`333`		`- strChildList = "\n";`
`334`		`-`
`335`	`332`	`// Get list of children names`
`336`	`333`	`for (CElement *pChild : _childArray) {`
`337`	`334`
Original file line number	Diff line number	Diff line change
`@@ -1370,8 +1370,6 @@ CParameterMgr::CCommandHandler::CommandStatus CParameterMgr::listElementsCommand`
`1370`	`1370`	`return CCommandHandler::EFailed;`
`1371`	`1371`	`}`
`1372`	`1372`
`1373`		`- strResult = string("\n");`
`1374`		`-`
`1375`	`1373`	`if (!pLocatedElement) {`
`1376`	`1374`
`1377`	`1375`	`// List from root folder`
`@@ -1399,8 +1397,6 @@ CParameterMgr::CCommandHandler::CommandStatus CParameterMgr::listParametersComma`
`1399`	`1397`	`return CCommandHandler::EFailed;`
`1400`	`1398`	`}`
`1401`	`1399`
`1402`		`- strResult = string("\n");`
`1403`		`-`
`1404`	`1400`	`if (!pLocatedElement) {`
`1405`	`1401`
`1406`	`1402`	`// List from root folder`
Original file line number	Diff line number	Diff line change
`@@ -194,8 +194,6 @@ class TRemoteCommandHandlerTemplate : public IRemoteCommandHandler`
`194`	`194`	`{`
`195`	`195`	`initMaxCommandUsageLength();`
`196`	`196`
`197`		`- strResult = "\n";`
`198`		`-`
`199`	`197`	`// Show usages`
`200`	`198`	`for (const auto *pRemoteCommandParserItem : _remoteCommandParserVector) {`
`201`	`199`