Add an XSLT pre-analyse step #1096

stefanweiser · 2017-03-21T10:27:02Z

Hi folks,

I implemented an XSLT translation. I currently does not support wildcards (don't know how to get the concrete file list out of a wildcard pattern in Java), but multiple input and output files seperated by comma. The translation of files is done before using the report file of the output sensor, so:

`sonar.cxx.xslt.1.stylesheet=MyStylesheets1.xslt
sonar.cxx.xslt.1.source=tool1/reports/xxx-1.xml
sonar.cxx.xslt.1.output=reports/other-tool1-1.xml

sonar.cxx.xslt.2.stylesheet=MyStylesheets2.xslt
sonar.cxx.xslt.2.source=reports/other-tool1-1.xml
sonar.cxx.xslt.2.output=reports/other-tool2-2.xml

sonar.cxx.other.reportPath=reports/other-tool2-2.xml`

will report "reports/other-tool2-2.xml" after translation from tool1/reports/xxx-1.xml to reports/other-tool1-1.xml to reports/other-tool2-2.xml.

I tested it on our own server, but was not able to provide a test case. I think I'm not able to understand the unit test infrastructure...

close #1080

This change is

guwirth

unit test is missing
UI integration is missing
- think we need then hard coded properties?
if we add UI: would start with 5 only (instead 10)

guwirth · 2017-03-22T08:18:25Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

 */
 public class CxxExternalRulesSensor extends CxxReportSensor {
  public static final Logger LOG = Loggers.get(CxxExternalRulesSensor.class);
  public static final String REPORT_PATH_KEY = "sonar.cxx.other.reportPath";
+  public static final String SONAR_CXX_XSLT_KEY = "sonar.cxx.xslt.";


Not sure, but I think should be in an extra sensor and not within CxxReportSensor?

If yes: must be clarified how to define order of sensor execution

I was uncertain, too. I guess, it's a question of: Which other sensor could benefit? I don't see any use in transforming a report, that already fits to the sensor (which is the case elsewhere).

I do agree with Stefan, this kind of transformation seems applicable only to external sensor since it's the only one that forces users to provide a specific format, @guwirth which other sensors do you think would benefit from this?

There is also the XUnit sensor using a XSLT doing the same: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/tests/xunit/CxxXunitSensor.java#L233. Question is if we wanna have two?

in this case, i think so. the XunitSensor is feeding test metrics, this is to feed issues so they have different purposes. what we can do is to use the same transformation code, @stefanweiser you could take a look into the xunit sensor how its done move the logic to own class and then reuse in both external sensor and xunit sensor?

To answer the question of being an extra sensor. This affects sensor execution order: I don't think, that we could enforce a specific order. The documentation do not say something about it. Somebody asked this already, but got not very much information: http://stackoverflow.com/q/42571579/3781684

In case we keep it for 'other' name must be sonar.cxx.other.xslt.xxx.

Think it's not hard to change later one. Let's start as part of 'other'. There is also no problem with the sequence then.

imo this should be like the https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/tests/xunit/CxxXunitSensor.java so no big complexity here

guwirth · 2017-03-22T08:22:44Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

+      String outputKey = SONAR_CXX_XSLT_KEY + i + OUTPUT_KEY;
+
+      String stylesheet = FilenameUtils.normalize(settings.getString(SONAR_CXX_XSLT_KEY + i + STYLESHEET_KEY));
+      String[] sources = settings.getStringArray(SONAR_CXX_XSLT_KEY + i + SOURCE_KEY);


Would not support comma separated list. Would use wildcard pattern instead

I thought, that getStringArray would return an array of strings, that are comma-seperated in the settings file and I don't know how to solve wildcard patterns. Is there any reference implementation for this in sonar-cxx?

We are doing this in CxxReportSensor.getReports. This is a static function, maybe we can move it https://github.com/SonarOpenCommunity/sonar-cxx/tree/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils to make it more general?

sonar-cxx/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java

Line 126 in 056fbb3

public static List<File> getReports(Settings settings, final File moduleBaseDir,

guwirth · 2017-03-22T08:23:47Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

+      } else if ((stylesheet != null) ||
+        ((sources != null) && (sources.length > 0)) ||
+        ((outputs != null) && (outputs.length > 0))) {
+        if (stylesheet == null) {


Think we need always all three. So one message would be sufficient?

We do need always all three, but I appreciate a tool, that reports to me exactly what's wrong. Otherwise you have to do some research first to find the bug.

guwirth · 2017-03-22T08:27:26Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

+  public void execute(SensorContext context) {
+    try {
+      transformFiles();
+    } catch (Exception e) {


Think try/catch is at the wrong place. If one transformation is failing no other is executed?
Would prefer: if one transformation is failing, should try the next one

Doing so would then result in a lot of errors if one transformation fails, because the source of the next transformation step would be missing. I know, that this would be more tolerant.

Its a design decision, which I would leave up to you. So if you want to go on transforming files, even there are errors, then I would implement it that way.

Up to now we have two modes: strict and tolerant

strict: break whole analysis after an error

tolerant: ignore and continue

This is hidden behind CxxUtils.validateRecovery.

In tolerant mode:

should continue if one of the XSLT transformation fails and there are more source files in the comma separated list

should continue with next XSLT transformation if one before was failing

So you suggest different behavior for both modes? ;-)

Reasons to catch not only on highest level:

Could be tool A needs transforming A and tool B needs transformation B. If A is failing B should nevertheless work (tolerant mode)

Same if more then one file has to be transformed. If transformation A handles more then one file it should continue if one transformation is failing.

So you suggest different behavior for both modes? ;-)

Yes behavior of all sensors is different depending on setting of 'sonar.cxx.errorRecoveryEnabled'. But all what you have to do is to call:
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxUtils.java#L88

guwirth · 2017-03-22T08:34:40Z

@stefanweiser thanks a lot for providing this solution. I started a review with some general questions.

stefanweiser · 2017-03-23T06:27:10Z

I want to write a unit test for this, but I was not able get it to work. Seems, that I have to put more effort in this.

What do you mean with UI-integration? How do I get that?

guwirth · 2017-03-23T07:44:15Z

@stefanweiser

unit test: maybe you can use this as an sample: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/tests/xunit/CxxXunitSensorTest.java#L79

guwirth · 2017-03-23T07:49:28Z

What do you mean with UI-integration? How do I get that?

@stefanweiser on server side you can set the properties normally also in the UI <Administration/C++ (Community)>. These properties are defined here: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/CxxPlugin.java#L79

Question is if XSLT would be a new tab: (6) XSLT Transformation?

stefanweiser · 2017-03-23T07:56:40Z

OK, whether it is an own tab or not is a question of "Would it be an own sensor or is it part of a sensor", because in the first case the information is associated to the plugin and is of common nature. In the second case it belongs to the sensor and would be different for every sensor.

Currently I believe, that it should belong as a utility class to each sensor.

guwirth · 2017-03-26T07:17:17Z

@stefanweiser point should be changed before we merge it:

we keep it as part of other => change keys accordingly
support 'tolerant' mode in the right way
support wildcards
add an unit test

jmecosta · 2017-03-26T08:02:16Z

@stefanweiser i would also recommend that the xslt in https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/tests/xunit/CxxXunitSensor.java to be moved to a utility library. and be reused in both xunit and external sensor.

jmecosta

still in early stages to be merged, tests are missing and we have duplicated functionality in 2 places. so can be made a utility class.

jmecosta · 2017-03-26T08:03:16Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

 */
 public class CxxExternalRulesSensor extends CxxReportSensor {
  public static final Logger LOG = Loggers.get(CxxExternalRulesSensor.class);
  public static final String REPORT_PATH_KEY = "sonar.cxx.other.reportPath";
+  public static final String SONAR_CXX_XSLT_KEY = "sonar.cxx.xslt.";


imo this should be like the https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/tests/xunit/CxxXunitSensor.java so no big complexity here

jmecosta · 2017-03-26T08:04:41Z

sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java

+    }
+  }
+
+  private void transformFile(File stylesheetFile, String source, String output) throws TransformerException {


transformation can be done in memory, as the xunit sensor. it should speed up the analysis

stefanweiser · 2017-04-13T07:41:27Z

Sorry, for being quiet for a long time. I was a bit busy. I integrated most of the change requests and got stuck with testing now. How can I debug these tests (propably just with printf). I added some test files, but it seems that my test will not generate any output files in the test scenario.

jmecosta · 2017-04-13T07:45:08Z

@stefanweiser try intarget\surefire-reports usually all test data ouput is found under those files

stefanweiser · 2017-04-13T14:27:53Z

Thanks @jmecosta. This worked pretty well.

I've pushed all changes and I hope, that I forgot nothing.

The keys start now with: sonar.cxx.other.xslt.*
Added two unit tests (one for invalid files and one for successful transformation)
I use getReports now, so wildcards are supported on the inputs (on the outputs, there is no way, because it would not be possible to guess the output names)
Support for the tolerant and strict way

I hope, that you won't find some easter eggs in there.

jmecosta · 2017-04-13T14:54:16Z

I would still try to do the transformation on memory, so the output properties are really not required. I think the unit test is done like that

…

On Thu, 13 Apr 2017, 17:27 Stefan Weiser, ***@***.***> wrote: Thanks @jmecosta <https://github.com/jmecosta>. This worked pretty well. I've pushed all changes and I hope, that I forgot nothing. - The keys start now with: sonar.cxx.other.xslt.* - Added two unit tests (one for invalid files and one for successful transformation) - I use getReports now, so wildcards are supported on the inputs (on the outputs, there is no way, because it would not be possible to guess the output names) - Support for the tolerant and strict way I hope, that you won't find some easter eggs in there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1096 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_jyGU4MFSugenmYtN_iXAsYm16HJEnks5rvjDpgaJpZM4MjmE0> .

stefanweiser · 2017-04-24T07:59:23Z

But if we would skip the temporaries and output just the last XMLs, you'd have much more to debug if one tranlation script in the midst is buggy. How realistic is the case, where multiple transformation steps are necessary?

jmecosta · 2017-04-24T08:54:17Z

ok, but then the default behaviour should be without the outputs. in normal cases a end user would want to set only the xslt?

stefanweiser · 2017-04-24T11:26:09Z

I think the usual way is to set the XSLT, provide one or more input XML(s) and go. The configuration scheme should also change into something like this, if we don't want the intermediate output:

sonar.cxx.xslt.stylesheets=MyStylesheet1.xslt,MyStylesheet2.xslt,MyStylesheet3.xslt sonar.cxx.xslt.source=tool1/reports/xxx-1.xml,tool1/reports/xxx-2.xml sonar.cxx.xslt.output=reports/other-tool1-1.xml,reports/other-tool1-2.xml

Then there is no need for the different blocks sonar.cxx.xslt.1, 2, 3...

The way it is at the moment, we will have to configure more and get more build artifacts but IMHO it is better for debugging and more straight forward.

jmecosta · 2017-04-24T12:32:05Z

Well it's not a big deal, should be ok then, try to rebase and squash to get the build green

…

On Mon, 24 Apr 2017, 14:26 Stefan Weiser, ***@***.***> wrote: I think the usual way is to set the XSLT, provide one or more input XML(s) and go. The configuration scheme should also change into something like this, if we don't want the intermediate output: sonar.cxx.xslt.stylesheets=MyStylesheet1.xslt,MyStylesheet2.xslt,MyStylesheet3.xslt sonar.cxx.xslt.source=tool1/reports/xxx-1.xml,tool1/reports/xxx-2.xml sonar.cxx.xslt.output=reports/other-tool1-1.xml,reports/other-tool1-2.xml Then there is no need for the different blocks sonar.cxx.xslt.1, 2, 3... The way it is at the moment, we will have to configure more and get more build artifacts but IMHO it is better for debugging and more straight forward. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1096 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_jyO1-1h3t-7R0N3X1FF7dZZ2CZ0L7ks5rzIbSgaJpZM4MjmE0> .

stefanweiser · 2017-04-24T13:48:39Z

Now all checks are green.

stefanweiser · 2017-04-25T13:09:54Z

I fixed an issue on key naming (sonar.cxx is getting now prefixed by CxxLanguage), that exists since I merged the upstream branch into my fork.

guwirth · 2017-04-25T14:50:52Z

@stefanweiser thanks for providing this. It's also ok from my side besides minor issues:

because source and output are lists now should be sources and outputs (with s)

sonar.cxx.xslt.1.sources=tool1/reports/xxx-1.xml
sonar.cxx.xslt.1.outputs=reports/other-tool1-1.xml

after seeing the solution wondering if we should not better rename sources to inputs? Think the pairs are source/destination and input/output?

stefanweiser · 2017-04-25T15:51:21Z

ACK, renamed it in inputs and outputs.

guwirth

@stefanweiser Sorry, call me pedantic. But please add also variable and parameter names from <source(s)> to <input(s)>

guwirth · 2017-04-26T15:47:25Z

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java

+  public static final String KEY = "other";
+  public static final String OTHER_XSLT_KEY = KEY + ".xslt.";
+  public static final String STYLESHEET_KEY = ".stylesheet";
+  public static final String SOURCE_KEY = ".inputs";


name should be INPUT_KEY

guwirth · 2017-04-26T15:47:59Z

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java

+    Boolean goOn = true;
+    for (int i = 1; (i < 10) && goOn; i++) {
+      String stylesheetKey = OTHER_XSLT_KEY + i + STYLESHEET_KEY;
+      String sourceKey = OTHER_XSLT_KEY + i + SOURCE_KEY;


name should be inputKey

guwirth · 2017-04-26T15:48:31Z

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java

+      String outputKey = OTHER_XSLT_KEY + i + OUTPUT_KEY;
+
+      String stylesheet = resolveFilename(baseDir.getAbsolutePath(), cxxLanguage.getStringOption(stylesheetKey));
+      List<File> sources = getReports(cxxLanguage, baseDir, sourceKey);


name should be inputs

guwirth · 2017-04-26T15:49:05Z

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java

+    }
+  }
+
+  private void transformFileList(final String baseDir, File stylesheetFile, List<File> sources, List<String> outputs) {


sources => inputs

guwirth · 2017-04-26T15:49:45Z

cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxUtils.java

-
+
+
+  public static void transformFile(Source stylesheetFile, File source, File output) throws TransformerException {


source => input

stefanweiser · 2017-04-27T08:25:37Z

Shame on me. I've just fixed that.

guwirth · 2017-04-27T12:54:15Z

Hi @stefanweiser thanks a lot. Maybe you can do a final rebase & squash before merge. AppVeyor problem seems to be a tool problem.

stefanweiser · 2017-04-27T13:46:53Z

@guwirth: The branch is already rebased. Do you want me to squash all my commits into one?

guwirth · 2017-04-27T16:24:15Z

@stefanweiser yes if possible. This will trigger a new build.

guwirth · 2017-04-29T08:33:11Z

@jmecosta can you restart AppVeyor to see if everything is green please.

jmecosta · 2017-04-29T08:48:25Z

Yep, bit later in the day. Tought

…

On Sat, 29 Apr 2017, 11:33 Günter Wirth, ***@***.***> wrote: @jmecosta <https://github.com/jmecosta> can you restart AppVeyor to see if everything is green please. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1096 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_jyHu2pjzGGlxxtCm9BZnGX86dY4KPks5r0vXIgaJpZM4MjmE0> .

stefanweiser · 2017-04-29T09:50:09Z

Sorry for letting you wait. Squashed the feature into one commit.

guwirth · 2017-04-29T11:47:50Z

@stefanweiser thanks a lot for your contribution and the patience with us 😄.

guwirth changed the title ~~Implementation for #1080~~ Add an XSLT pre-analyse step Mar 22, 2017

guwirth requested review from jmecosta and guwirth March 22, 2017 08:16

guwirth assigned stefanweiser Mar 22, 2017

guwirth added the enhancement label Mar 22, 2017

guwirth added this to the 0.9.8 milestone Mar 22, 2017

guwirth requested changes Mar 22, 2017

View reviewed changes

jmecosta requested changes Mar 26, 2017

View reviewed changes

jmecosta approved these changes Apr 24, 2017

View reviewed changes

stefanweiser force-pushed the master branch from be979a6 to 6f709b0 Compare April 26, 2017 14:01

guwirth requested changes Apr 26, 2017

View reviewed changes

guwirth approved these changes Apr 27, 2017

View reviewed changes

Implement SonarOpenCommunity#1080.

3a626ff

stefanweiser force-pushed the master branch from c81d913 to 3a626ff Compare April 29, 2017 09:48

guwirth added the docu request label Apr 29, 2017

guwirth merged commit 8ff30b9 into SonarOpenCommunity:master Apr 29, 2017

guwirth mentioned this pull request Oct 29, 2017

RFF V0.9.8 with SQ6.x #1196

Closed

guwirth mentioned this pull request Dec 4, 2017

Fix Issue1277: ERROR: sonar.cxx.other.xslt.1.inputs file is not defined. #1348

Closed




		public static void transformFile(Source stylesheetFile, File source, File output) throws TransformerException {

Add an XSLT pre-analyse step #1096

Add an XSLT pre-analyse step #1096

Conversation

stefanweiser commented Mar 21, 2017 • edited by SonarOpenCommunityAdmin Loading

guwirth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guwirth commented Mar 22, 2017

stefanweiser commented Mar 23, 2017

guwirth commented Mar 23, 2017

guwirth commented Mar 23, 2017 • edited Loading

stefanweiser commented Mar 23, 2017

guwirth commented Mar 26, 2017

jmecosta commented Mar 26, 2017

jmecosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanweiser commented Apr 13, 2017 • edited Loading

jmecosta commented Apr 13, 2017

stefanweiser commented Apr 13, 2017

jmecosta commented Apr 13, 2017 via email

stefanweiser commented Apr 24, 2017 • edited Loading

jmecosta commented Apr 24, 2017

stefanweiser commented Apr 24, 2017

jmecosta commented Apr 24, 2017 via email

stefanweiser commented Apr 24, 2017

stefanweiser commented Apr 25, 2017

guwirth commented Apr 25, 2017

stefanweiser commented Apr 25, 2017

guwirth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanweiser commented Apr 27, 2017

guwirth commented Apr 27, 2017

stefanweiser commented Apr 27, 2017 • edited Loading

guwirth commented Apr 27, 2017

guwirth commented Apr 29, 2017

jmecosta commented Apr 29, 2017 via email

stefanweiser commented Apr 29, 2017

guwirth commented Apr 29, 2017

stefanweiser commented Mar 21, 2017 •

edited by SonarOpenCommunityAdmin

Loading

guwirth commented Mar 23, 2017 •

edited

Loading

stefanweiser commented Apr 13, 2017 •

edited

Loading

stefanweiser commented Apr 24, 2017 •

edited

Loading

guwirth left a comment •

edited

Loading

stefanweiser commented Apr 27, 2017 •

edited

Loading