SONAR-5056 Refactor the way characteristics and rules are imported from XML

10 files changed, 117 insertions, 59 deletions

diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
index d10c8056add..1c47ffbc6a0 100644
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
@@ -20,38 +20,40 @@
 
 package org.sonar.server.debt;
 
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
 import org.apache.commons.lang.StringUtils;
 import org.codehaus.stax2.XMLInputFactory2;
 import org.codehaus.staxmate.SMInputFactory;
 import org.codehaus.staxmate.in.SMHierarchicCursor;
 import org.codehaus.staxmate.in.SMInputCursor;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.sonar.api.ServerExtension;
 import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
-import org.sonar.api.utils.ValidationMessages;
-import org.sonar.core.technicaldebt.DefaultTechnicalDebtModel;
 
+import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
 import javax.xml.stream.XMLInputFactory;
 import javax.xml.stream.XMLStreamException;
 
 import java.io.Reader;
 import java.io.StringReader;
+import java.util.Collection;
+import java.util.List;
 
-public class DebtCharacteristicsXMLImporter implements ServerExtension {
+import static com.google.common.collect.Lists.newArrayList;
 
-  private static final Logger LOG = LoggerFactory.getLogger(DebtCharacteristicsXMLImporter.class);
+public class DebtCharacteristicsXMLImporter implements ServerExtension {
 
   public static final String CHARACTERISTIC = "chc";
   public static final String CHARACTERISTIC_KEY = "key";
   public static final String CHARACTERISTIC_NAME = "name";
 
-  public DefaultTechnicalDebtModel importXML(String xml, ValidationMessages messages) {
-    return importXML(new StringReader(xml), messages);
+  public DebtModel importXML(String xml) {
+    return importXML(new StringReader(xml));
   }
 
-  public DefaultTechnicalDebtModel importXML(Reader xml, ValidationMessages messages) {
-    DefaultTechnicalDebtModel model = new DefaultTechnicalDebtModel();
+  public DebtModel importXML(Reader xml) {
+    DebtModel model = new DebtModel();
     try {
       SMInputFactory inputFactory = initStax();
       SMHierarchicCursor cursor = inputFactory.rootElementCursor(xml);
@@ -61,14 +63,13 @@ public class DebtCharacteristicsXMLImporter implements ServerExtension {
       SMInputCursor chcCursor = cursor.childElementCursor(CHARACTERISTIC);
 
       while (chcCursor.getNext() != null) {
-        processCharacteristic(model, null, chcCursor, messages);
+        processCharacteristic(model, null, chcCursor);
       }
 
       cursor.getStreamReader().closeCompletely();
 
     } catch (XMLStreamException e) {
-      LOG.error("XML is not valid", e);
-      messages.addErrorText("XML is not valid: " + e.getMessage());
+      throw new IllegalStateException("XML is not valid", e);
     }
     return model;
   }
@@ -82,8 +83,8 @@ public class DebtCharacteristicsXMLImporter implements ServerExtension {
     return new SMInputFactory(xmlFactory);
   }
 
-  private DefaultCharacteristic processCharacteristic(DefaultTechnicalDebtModel model, DefaultCharacteristic parent, SMInputCursor chcCursor,
-                                                      ValidationMessages messages) throws XMLStreamException {
+  @CheckForNull
+  private DefaultCharacteristic processCharacteristic(DebtModel model, @Nullable DefaultCharacteristic parent, SMInputCursor chcCursor) throws XMLStreamException {
     DefaultCharacteristic characteristic = new DefaultCharacteristic();
     SMInputCursor cursor = chcCursor.childElementCursor();
     while (cursor.getNext() != null) {
@@ -98,7 +99,7 @@ public class DebtCharacteristicsXMLImporter implements ServerExtension {
 
         // <chc> can contain characteristics or requirements
       } else if (StringUtils.equals(node, CHARACTERISTIC)) {
-        processCharacteristic(model, characteristic, cursor, messages);
+        processCharacteristic(model, characteristic, cursor);
       }
     }
 
@@ -110,4 +111,53 @@ public class DebtCharacteristicsXMLImporter implements ServerExtension {
     return null;
   }
 
+  static class DebtModel {
+
+    private Collection<DefaultCharacteristic> rootCharacteristics;
+
+    public DebtModel() {
+      rootCharacteristics = newArrayList();
+    }
+
+    public DebtModel addRootCharacteristic(DefaultCharacteristic characteristic) {
+      rootCharacteristics.add(characteristic);
+      return this;
+    }
+
+    public List<DefaultCharacteristic> rootCharacteristics() {
+      return newArrayList(Iterables.filter(rootCharacteristics, new Predicate<DefaultCharacteristic>() {
+        @Override
+        public boolean apply(DefaultCharacteristic input) {
+          return input.isRoot();
+        }
+      }));
+    }
+
+    @CheckForNull
+    public DefaultCharacteristic characteristicByKey(final String key) {
+      return Iterables.find(characteristics(), new Predicate<DefaultCharacteristic>() {
+        @Override
+        public boolean apply(DefaultCharacteristic input) {
+          return input.key().equals(key);
+        }
+      }, null);
+    }
+
+    public List<DefaultCharacteristic> characteristics() {
+      List<DefaultCharacteristic> flatCharacteristics = newArrayList();
+      for (DefaultCharacteristic rootCharacteristic : rootCharacteristics) {
+        flatCharacteristics.add(rootCharacteristic);
+        for (DefaultCharacteristic characteristic : rootCharacteristic.children()) {
+          flatCharacteristics.add(characteristic);
+        }
+      }
+      return flatCharacteristics;
+    }
+
+    public boolean isEmpty() {
+      return rootCharacteristics.isEmpty();
+    }
+
+  }
+
 }
diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelSynchronizer.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelSynchronizer.java
index 9ef036dc864..5fa4ff5bd68 100644
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelSynchronizer.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelSynchronizer.java
@@ -26,9 +26,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.ServerExtension;
 import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
-import org.sonar.api.utils.ValidationMessages;
 import org.sonar.core.persistence.MyBatis;
-import org.sonar.core.technicaldebt.DefaultTechnicalDebtModel;
 import org.sonar.core.technicaldebt.TechnicalDebtModelRepository;
 import org.sonar.core.technicaldebt.db.CharacteristicDao;
 import org.sonar.core.technicaldebt.db.CharacteristicDto;
@@ -54,12 +52,12 @@ public class DebtModelSynchronizer implements ServerExtension {
     this.importer = importer;
   }
 
-  public List<CharacteristicDto> synchronize(ValidationMessages messages) {
+  public List<CharacteristicDto> synchronize() {
     SqlSession session = mybatis.openSession();
 
     List<CharacteristicDto> model = newArrayList();
     try {
-      model = synchronize(messages, session);
+      model = synchronize(session);
       session.commit();
     } finally {
       MyBatis.closeQuietly(session);
@@ -67,14 +65,13 @@ public class DebtModelSynchronizer implements ServerExtension {
     return model;
   }
 
-  public List<CharacteristicDto> synchronize(ValidationMessages messages, SqlSession session) {
-    DefaultTechnicalDebtModel defaultModel = loadModelFromXml(TechnicalDebtModelRepository.DEFAULT_MODEL, messages);
+  public List<CharacteristicDto> synchronize(SqlSession session) {
+    DebtCharacteristicsXMLImporter.DebtModel defaultModel = loadModelFromXml(TechnicalDebtModelRepository.DEFAULT_MODEL);
     List<CharacteristicDto> model = loadOrCreateModelFromDb(defaultModel, session);
-    messages.log(LOG);
     return model;
   }
 
-  private List<CharacteristicDto> loadOrCreateModelFromDb(DefaultTechnicalDebtModel defaultModel, SqlSession session) {
+  private List<CharacteristicDto> loadOrCreateModelFromDb(DebtCharacteristicsXMLImporter.DebtModel defaultModel, SqlSession session) {
     List<CharacteristicDto> characteristicDtos = loadModel();
     if (characteristicDtos.isEmpty()) {
       return createTechnicalDebtModel(defaultModel, session);
@@ -86,7 +83,7 @@ public class DebtModelSynchronizer implements ServerExtension {
     return dao.selectEnabledCharacteristics();
   }
 
-  private List<CharacteristicDto> createTechnicalDebtModel(DefaultTechnicalDebtModel defaultModel, SqlSession session) {
+  private List<CharacteristicDto> createTechnicalDebtModel(DebtCharacteristicsXMLImporter.DebtModel defaultModel, SqlSession session) {
     List<CharacteristicDto> characteristics = newArrayList();
     for (DefaultCharacteristic rootCharacteristic : defaultModel.rootCharacteristics()) {
       CharacteristicDto rootCharacteristicDto = CharacteristicDto.toDto(rootCharacteristic, null);
@@ -101,11 +98,11 @@ public class DebtModelSynchronizer implements ServerExtension {
     return characteristics;
   }
 
-  public DefaultTechnicalDebtModel loadModelFromXml(String pluginKey, ValidationMessages messages) {
+  public DebtCharacteristicsXMLImporter.DebtModel loadModelFromXml(String pluginKey) {
     Reader xmlFileReader = null;
     try {
       xmlFileReader = languageModelFinder.createReaderForXMLFile(pluginKey);
-      return importer.importXML(xmlFileReader, messages);
+      return importer.importXML(xmlFileReader);
     } finally {
       IOUtils.closeQuietly(xmlFileReader);
     }
diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtRulesXMLImporter.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtRulesXMLImporter.java
index 3beb4daa1e4..5f76c882bb9 100644
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtRulesXMLImporter.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtRulesXMLImporter.java
@@ -81,14 +81,13 @@ public class DebtRulesXMLImporter implements ServerExtension {
       cursor.advance();
       SMInputCursor rootCursor = cursor.childElementCursor(CHARACTERISTIC);
       while (rootCursor.getNext() != null) {
-        processCharacteristic(ruleDebts, null, null, rootCursor);
+        process(ruleDebts, null, null, rootCursor);
       }
 
       cursor.getStreamReader().closeCompletely();
     } catch (XMLStreamException e) {
-      LOG.error("XML is not valid", e);
+      throw new IllegalStateException("XML is not valid", e);
     }
-
     return ruleDebts;
   }
 
@@ -101,7 +100,7 @@ public class DebtRulesXMLImporter implements ServerExtension {
     return new SMInputFactory(xmlFactory);
   }
 
-  private void processCharacteristic(List<RuleDebt> ruleDebts, @Nullable String rootKey, @Nullable String parentKey, SMInputCursor chcCursor) throws XMLStreamException {
+  private void process(List<RuleDebt> ruleDebts, @Nullable String rootKey, @Nullable String parentKey, SMInputCursor chcCursor) throws XMLStreamException {
     String currentCharacteristicKey = null;
     SMInputCursor cursor = chcCursor.childElementCursor();
     while (cursor.getNext() != null) {
@@ -109,7 +108,7 @@ public class DebtRulesXMLImporter implements ServerExtension {
       if (StringUtils.equals(node, CHARACTERISTIC_KEY)) {
         currentCharacteristicKey = cursor.collectDescendantText().trim();
       } else if (StringUtils.equals(node, CHARACTERISTIC)) {
-        processCharacteristic(ruleDebts, parentKey, currentCharacteristicKey, cursor);
+        process(ruleDebts, parentKey, currentCharacteristicKey, cursor);
       } else if (StringUtils.equals(node, REPOSITORY_KEY)) {
         RuleDebt ruleDebt = processRule(cursor);
         if (ruleDebt != null) {
diff --git a/sonar-server/src/main/java/org/sonar/server/startup/RegisterDebtModel.java b/sonar-server/src/main/java/org/sonar/server/startup/RegisterDebtModel.java
index f0baaf7fdf5..59a4ecdf035 100644
--- a/sonar-server/src/main/java/org/sonar/server/startup/RegisterDebtModel.java
+++ b/sonar-server/src/main/java/org/sonar/server/startup/RegisterDebtModel.java
@@ -23,7 +23,6 @@ package org.sonar.server.startup;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.utils.TimeProfiler;
-import org.sonar.api.utils.ValidationMessages;
 import org.sonar.server.debt.DebtModelSynchronizer;
 
 public class RegisterDebtModel {
@@ -38,7 +37,7 @@ public class RegisterDebtModel {
 
   public void start() {
     TimeProfiler profiler = new TimeProfiler(LOGGER).start("Register technical debt model");
-    manager.synchronize(ValidationMessages.create());
+    manager.synchronize();
     profiler.stop();
   }
 
diff --git a/sonar-server/src/test/java/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest.java b/sonar-server/src/test/java/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest.java
index 4a6b947c03d..1180215e3a2 100644
--- a/sonar-server/src/test/java/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest.java
@@ -24,12 +24,11 @@ import com.google.common.base.Charsets;
 import com.google.common.io.Resources;
 import org.junit.Test;
 import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
-import org.sonar.api.utils.ValidationMessages;
-import org.sonar.core.technicaldebt.DefaultTechnicalDebtModel;
 
 import java.io.IOException;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 
 public class DebtCharacteristicsXMLImporterTest {
 
@@ -37,8 +36,7 @@ public class DebtCharacteristicsXMLImporterTest {
   public void import_characteristics() {
     String xml = getFileContent("import_characteristics.xml");
 
-    ValidationMessages messages = ValidationMessages.create();
-    DefaultTechnicalDebtModel debtModel = new DebtCharacteristicsXMLImporter().importXML(xml, messages);
+    DebtCharacteristicsXMLImporter.DebtModel debtModel = new DebtCharacteristicsXMLImporter().importXML(xml);
 
     assertThat(debtModel.rootCharacteristics()).hasSize(2);
     assertThat(debtModel.rootCharacteristics().get(0).key()).isEqualTo("PORTABILITY");
@@ -63,26 +61,31 @@ public class DebtCharacteristicsXMLImporterTest {
   public void import_badly_formatted_xml() {
     String xml = getFileContent("import_badly_formatted_xml.xml");
 
-    ValidationMessages messages = ValidationMessages.create();
-    DefaultTechnicalDebtModel debtModel = new DebtCharacteristicsXMLImporter().importXML(xml, messages);
-
-    checkXmlCorrectlyImported(debtModel, messages);
-  }
-
-  private void checkXmlCorrectlyImported(DefaultTechnicalDebtModel sqale, ValidationMessages messages) {
-    assertThat(messages.getErrors()).isEmpty();
+    DebtCharacteristicsXMLImporter.DebtModel debtModel = new DebtCharacteristicsXMLImporter().importXML(xml);
 
     // characteristics
-    assertThat(sqale.rootCharacteristics()).hasSize(2);
-    DefaultCharacteristic efficiency = sqale.characteristicByKey("EFFICIENCY");
+    assertThat(debtModel.rootCharacteristics()).hasSize(2);
+    DefaultCharacteristic efficiency = debtModel.characteristicByKey("EFFICIENCY");
     assertThat(efficiency.name()).isEqualTo("Efficiency");
 
     // sub-characteristics
     assertThat(efficiency.children()).hasSize(1);
-    DefaultCharacteristic memoryEfficiency = sqale.characteristicByKey("MEMORY_EFFICIENCY");
+    DefaultCharacteristic memoryEfficiency = debtModel.characteristicByKey("MEMORY_EFFICIENCY");
     assertThat(memoryEfficiency.name()).isEqualTo("Memory use");
   }
 
+  @Test
+  public void fail_on_bad_xml() {
+    String xml = getFileContent("fail_on_bad_xml.xml");
+
+    try {
+      new DebtCharacteristicsXMLImporter().importXML(xml);
+      fail();
+    } catch (Exception e){
+      assertThat(e).isInstanceOf(IllegalStateException.class);
+    }
+  }
+
   private String getFileContent(String file) {
     try {
       return Resources.toString(Resources.getResource(DebtCharacteristicsXMLImporterTest.class, "DebtCharacteristicsXMLImporterTest/" + file), Charsets.UTF_8);
diff --git a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelSynchronizerTest.java b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelSynchronizerTest.java
index 9b7accc80b2..0ab0f351732 100644
--- a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelSynchronizerTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelSynchronizerTest.java
@@ -31,9 +31,7 @@ import org.mockito.invocation.InvocationOnMock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.mockito.stubbing.Answer;
 import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
-import org.sonar.api.utils.ValidationMessages;
 import org.sonar.core.persistence.MyBatis;
-import org.sonar.core.technicaldebt.DefaultTechnicalDebtModel;
 import org.sonar.core.technicaldebt.TechnicalDebtModelRepository;
 import org.sonar.core.technicaldebt.db.CharacteristicDao;
 import org.sonar.core.technicaldebt.db.CharacteristicDto;
@@ -67,7 +65,7 @@ public class DebtModelSynchronizerTest {
 
   Integer currentId = 1;
 
-  private DefaultTechnicalDebtModel defaultModel;
+  private DebtCharacteristicsXMLImporter.DebtModel defaultModel;
 
   private DebtModelSynchronizer manager;
 
@@ -75,10 +73,10 @@ public class DebtModelSynchronizerTest {
   public void initAndMerge() throws Exception {
     when(myBatis.openSession()).thenReturn(session);
 
-    defaultModel = new DefaultTechnicalDebtModel();
+    defaultModel = new DebtCharacteristicsXMLImporter.DebtModel();
     Reader defaultModelReader = mock(Reader.class);
     when(technicalDebtModelRepository.createReaderForXMLFile("technical-debt")).thenReturn(defaultModelReader);
-    when(xmlImporter.importXML(eq(defaultModelReader), any(ValidationMessages.class))).thenReturn(defaultModel);
+    when(xmlImporter.importXML(eq(defaultModelReader))).thenReturn(defaultModel);
 
     doAnswer(new Answer() {
       public Object answer(InvocationOnMock invocation) {
@@ -102,7 +100,7 @@ public class DebtModelSynchronizerTest {
     when(technicalDebtModelRepository.getContributingPluginList()).thenReturn(Collections.<String>emptyList());
     when(dao.selectEnabledCharacteristics()).thenReturn(Lists.<CharacteristicDto>newArrayList());
 
-    manager.synchronize(ValidationMessages.create());
+    manager.synchronize();
 
     verify(dao).selectEnabledCharacteristics();
     ArgumentCaptor<CharacteristicDto> characteristicCaptor = ArgumentCaptor.forClass(CharacteristicDto.class);
@@ -123,7 +121,7 @@ public class DebtModelSynchronizerTest {
     when(technicalDebtModelRepository.getContributingPluginList()).thenReturn(Collections.<String>emptyList());
     when(dao.selectEnabledCharacteristics()).thenReturn(Lists.newArrayList(new CharacteristicDto()));
 
-    manager.synchronize(ValidationMessages.create());
+    manager.synchronize();
 
     verify(dao, never()).insert(any(CharacteristicDto.class), eq(session));
   }
diff --git a/sonar-server/src/test/java/org/sonar/server/debt/DebtRulesXMLImporterTest.java b/sonar-server/src/test/java/org/sonar/server/debt/DebtRulesXMLImporterTest.java
index 08ed971be0d..9fdaefb428f 100644
--- a/sonar-server/src/test/java/org/sonar/server/debt/DebtRulesXMLImporterTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/debt/DebtRulesXMLImporterTest.java
@@ -190,6 +190,18 @@ public class DebtRulesXMLImporterTest {
     }
   }
 
+  @Test
+  public void fail_on_bad_xml() {
+    String xml = getFileContent("fail_on_bad_xml.xml");
+
+    try {
+      new DebtCharacteristicsXMLImporter().importXML(xml);
+      fail();
+    } catch (Exception e){
+      assertThat(e).isInstanceOf(IllegalStateException.class);
+    }
+  }
+
   private String getFileContent(String file) {
     try {
       return Resources.toString(Resources.getResource(DebtRulesXMLImporterTest.class, "DebtRulesXMLImporterTest/" + file), Charsets.UTF_8);
diff --git a/sonar-server/src/test/java/org/sonar/server/startup/RegisterDebtModelTest.java b/sonar-server/src/test/java/org/sonar/server/startup/RegisterDebtModelTest.java
index 5014226a950..b6d24cbcc94 100644
--- a/sonar-server/src/test/java/org/sonar/server/startup/RegisterDebtModelTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/startup/RegisterDebtModelTest.java
@@ -21,10 +21,8 @@
 package org.sonar.server.startup;
 
 import org.junit.Test;
-import org.sonar.api.utils.ValidationMessages;
 import org.sonar.server.debt.DebtModelSynchronizer;
 
-import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.*;
 
 public class RegisterDebtModelTest {
@@ -36,6 +34,6 @@ public class RegisterDebtModelTest {
 
     sqaleDefinition.start();
 
-    verify(synchronizer, times(1)).synchronize(any(ValidationMessages.class));
+    verify(synchronizer, times(1)).synchronize();
   }
 }
diff --git a/sonar-server/src/test/resources/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest/fail_on_bad_xml.xml b/sonar-server/src/test/resources/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest/fail_on_bad_xml.xml
new file mode 100644
index 00000000000..3b15eae11d6
--- /dev/null
+++ b/sonar-server/src/test/resources/org/sonar/server/debt/DebtCharacteristicsXMLImporterTest/fail_on_bad_xml.xml
@@ -0,0 +1 @@
+Not a valid xml
diff --git a/sonar-server/src/test/resources/org/sonar/server/debt/DebtRulesXMLImporterTest/fail_on_bad_xml.xml b/sonar-server/src/test/resources/org/sonar/server/debt/DebtRulesXMLImporterTest/fail_on_bad_xml.xml
new file mode 100644
index 00000000000..3b15eae11d6
--- /dev/null
+++ b/sonar-server/src/test/resources/org/sonar/server/debt/DebtRulesXMLImporterTest/fail_on_bad_xml.xml
@@ -0,0 +1 @@
+Not a valid xml