Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JAVA] [EC10] Image vectors #154

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
3 changes: 2 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ services:
- "extensions:/opt/sonarqube/extensions"
- "logs:/opt/sonarqube/logs"
- "data:/opt/sonarqube/data"

restart: unless-stopped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the use case for this need ?
for me, if container crashes, the developer must check why on logs et make a correction or an issue for correction.
if we add this option, we could miss the real pb

db:
image: postgres:12
container_name: postgresql_ecocode
Expand All @@ -43,6 +43,7 @@ services:
POSTGRES_PASSWORD: sonar
POSTGRES_DB: sonarqube
PGDATA: pg_data:/var/lib/postgresql/data/pgdata
restart: unless-stopped

networks:
sonarnet:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
package fr.greencodeinitiative.java.checks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, this new rule implementation isn't included in ecoCode plugin because there is no declaration in RulesList class : this is mandatory to include it.


import org.sonar.check.Priority;
import org.sonar.check.Rule;
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
import org.sonar.plugins.java.api.tree.*;
import org.w3c.dom.*;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerConfigurationException;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;
import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathConstants;
import javax.xml.xpath.XPathExpressionException;
import javax.xml.xpath.XPathFactory;
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
import java.lang.reflect.Field;
import java.rmi.UnmarshalException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Stream;

@Rule(
key = "EC53",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, check other rule implementation to make the same :

  • create a constant with id value (to make it easier to use it in other classes)
  • tags : no bug, but ecocode, eco-design and performance (maybe other tag, please check available native tags on SonarQube

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong rule id ==> EC10

name = "Developpement",
description = OptimizeImageVectorsSize.MESSAGERULE,
priority = Priority.MINOR,
tags = {"bug"})
public class OptimizeImageVectorsSize extends IssuableSubscriptionVisitor {
protected final String SVG_BEGIN = "<svg";
protected final String[] SVG_END = new String[]{"</svg", "</ svg>"};
protected final List<String> SUPERFLUOUS_ATTRIBUTES = Arrays.asList("xmlns:dc", "xmlns:cc", "xmlns:rdv", "xmlns:svg", "xmlns", "xmlns:sodipodi", "xmlns:inkscape", "inkscape:version", "inkscape:label", "inkscape:groupmode", "sodipodi:docname");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please could you tell us where do you find this list of superfluous attributes ? I'm curious :p
If we are ok with you, we could report it to other language rule implementation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @dedece35
I extracted the superfluous attributes from SVGGO demo page, which provides an original & compressed svg file that I compared:
compressed: blob:https://jakearchibald.github.io/09bc7d0d-2148-470d-9a25-86355304127f
original: blob:https://jakearchibald.github.io/b11f1eec-fd24-4bf2-95f2-127555093714
It's not a perfect list !

You can also define custom attributes in svg, e.g: version='1.2'
These attributes are superfluous if they're not used in the rest of the svg.

protected static final String MESSAGERULE = "Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner";
// COMPLETE MESSAGE:
// "Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner. The following should be avoided: The attribute value length can be reduced, e.g: id=\"filter4210\" to id=\"a\" (the same goes for xlink:href attribute). The attributes' value should be approximated: stdDeviation=\"0.57264819\" to stdDeviation=\"0.573\". Duplicated tags. Superfluous tags, e.g: <sodipodi:namedview>, <metadata>. Redundant tags. Superfluous attributes (xmlns:dc, xmlns:cc, xmlns:rdv, xmlns: svg, xmlns, xmlns:sodipodi, xmlns:inkscape, most of id attributes, version, inkscape:version, inkscape:label, inkscape:groupmode, sodipodi:docname)";

@Override
public List<Tree.Kind> nodesToVisit() {
return List.of(Tree.Kind.STRING_LITERAL, Tree.Kind.TEXT_BLOCK);
}

@Override
public void visitNode(Tree tree) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, explode this method in several smaller private functional methods. this is for readability and ease to maintain it.

String value = ((LiteralTree) tree).value();

// convert to lower case (not case sensitve anymore)
value = value.toLowerCase();

// trim a beginning and ending double quotes (") and single quotes (')
if (tree.is(Tree.Kind.STRING_LITERAL)) {
value = value.substring(1, value.length() - 1);
} else {
// a text block begins and ends with 3 double quotes
value = value.substring(3, value.length() - 3);
}

// stop escaping double quotes (") and single quotes (')
value = value.replaceAll("\\\\\"", "\"");
value = value.replaceAll("\\\\\'", "\'");

// search for svg beginning tag
int beginIndex = value.indexOf(SVG_BEGIN);
if (beginIndex < 0) {
// the string doesn't contain any svg
return;
}

// search for svg ending tag
int endIndex = -1;
int j = 0;
while (j < SVG_END.length && endIndex == -1) {
endIndex = value.indexOf(SVG_END[j]);
++j;
}
if (endIndex == -1) {
return; // svg is invalid or is broken into multiple strings
}

// Parse svg as xml and explore its tree
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you use XML parsing and not simplier way, for example string pattern search on all java code ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into regex matching at first, but I realized it may not suit some rules I defined.
The name of some superfluous attributes can be used as content/text instead of tags attributes, e.g: xmlns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should consider using XPath? SVGs files being XML, this would allow us to perform the analysis independently of what language/framework is used. This is mentionned in the SonarQube documentation

try {
// Build the doc from the XML file
InputSource source = new InputSource(new StringReader(value));

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
DocumentBuilder db = dbf.newDocumentBuilder();
Document doc = db.parse(source);

// the string is a valid xml file

XPath xpath = XPathFactory.newInstance().newXPath();

// Note to developers: check xpath expressions by using xpather: http://xpather.com/

// Superfluous tags, e.g: <sodipodi:namedview>, <metadata>
if (((Double) xpath.evaluate("count(*//sodipodi:namedview)",
doc, XPathConstants.NUMBER)).intValue() > 0 || ((Double) xpath.evaluate("count(*//metadata)",
doc, XPathConstants.NUMBER)).intValue() > 0) {
reportIssue(tree, MESSAGERULE);
return;
}

int nbId = ((Double) xpath.evaluate("count(//@id)", doc, XPathConstants.NUMBER)).intValue();
int nbDistinctId = ((Double) xpath.evaluate("count(//@id[not(. = preceding::*/@id)])", doc, XPathConstants.NUMBER)).intValue();
int nbHref = ((Double) xpath.evaluate("count(//@xlink:href)", doc, XPathConstants.NUMBER)).intValue();
int nbDistinctHref = ((Double) xpath.evaluate("count(//@xlink:href[not(. = preceding::*/@xlink:href)])", doc, XPathConstants.NUMBER)).intValue();

// Duplicated tags (tags with the same xlink:href or same id attributes)
if (nbId != nbDistinctId || nbHref != nbDistinctHref) {
// count(//@id) returns the number of elements having an attribute "id"
// count(//@id[not(. = preceding::*/@id)]) returns the number of unique "id" attribute values
// if the number of "id" attributes and the number of unique "id" attributes values
// aren't equal, that means at least two elements have an "id" attribute.
reportIssue(tree, MESSAGERULE);
return;
}

// The attribute value length can be reduced
if ((boolean) xpath.evaluate("boolean(//@id[string-length() > 3])",
doc, XPathConstants.BOOLEAN)) {
reportIssue(tree, MESSAGERULE);
return;
}

// The attributes' value should be approximated (there are attributes with a numeric value that have more than 3 decimals)
// @* selects all the attributes of the document
// [number(.) = .] is a predicate that filters all attributes whose value can be converted to a numeric one.
if ((boolean) xpath.evaluate("boolean(//@*[number(.) = . and contains(., '.') and string-length(substring-after(., '.')) > 3])", doc, XPathConstants.BOOLEAN)) {
reportIssue(tree, MESSAGERULE);
return;
}

// Avoid superfluous attributes
// We do not search for the superfluous attributes directly in the xml as a string as the attributes could be the text inside of xml tags.
NodeList list = (NodeList) xpath.evaluate("//*", doc, XPathConstants.NODESET);
int nbNodes = list.getLength();
for (int i = 0; i < nbNodes; ++i) {
NamedNodeMap attributes = list.item(i).getAttributes();
int nbAttr = attributes.getLength();
for (j = 0; j < nbAttr; ++j) {
if (SUPERFLUOUS_ATTRIBUTES.contains(((Attr) attributes.item(j)).getName())) {
reportIssue(tree, MESSAGERULE);
return;
}
}
}

} catch (Throwable ignored) {
}
}
}
52 changes: 52 additions & 0 deletions java-plugin/src/test/files/OptimizeImageVectorsSize.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;

class TestClass {

TestClass(TestClass tc) {
}

String compliantCase() {
return "<svg id=\"a\"></svg>";
}

String idCanBeReduced() {
return "<svg id=\"main-wrapper\"></svg>"; // Noncompliant {{Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner}}
}

String superfluousTag() {
return "<svg><metadata></metadata></svg>"; // Noncompliant {{Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner}}
}

String duplicatedTags() {
return "<svg><linearGradient inkscape:collect=\"always\" xlink:href=\"#linearGradient4405\" id=\"linearGradient2917\" gradientUnits=\"userSpaceOnUse\" gradientTransform=\"matrix(19.565,0,0,19.565,-95.619,79.961)\" x1=\"11.685\" y1=\"33.646\" x2=\"10.783\" y2=\"35.870\" /><linearGradient inkscape:collect=\"always\" xlink:href=\"#linearGradient4405\" id=\"linearGradient2917\" gradientUnits=\"userSpaceOnUse\" gradientTransform=\"matrix(19.565,0,0,19.565,-95.619,79.961)\" x1=\"11.685\" y1=\"33.646\" x2=\"10.783\" y2=\"35.870\" /></svg>"; // Noncompliant {{Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner}}
}

String unapproximatedNumericValues() {
return "<svg><linearGradient inkscape:collect=\"always\" xlink:href=\"#linearGradient4405\" id=\"linearGradient2917\" gradientUnits=\"userSpaceOnUse\" gradientTransform=\"matrix(19.565616,0,0,19.565616,-95.619433,79.96141)\" x1=\"11.685\" y1=\"33.646999\" x2=\"10.783\" y2=\"35.870998\" /></svg>"; // Noncompliant {{Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner}}
}

String validAttribute() {
return "<svg><div trucmuche=\"test\"></div></svg>";
}

String superfluousAttribute() {
return "<svg><div xmlns:dc=\"test\"></div></svg>"; // Noncompliant {{Optimize your svg by using open source tools such as SVGGO, Compressor.io or SVG Cleaner}}
}

String blockCompliantCase() {
return """
<svg id="a"></svg>
""";
}

/* can't write test as all comments are included in the text block and blocks the xml analysis
String blockIdCanBeReduced() {
return """
<svg id=\"main-wrapper\"></svg>
""";
}
*/

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package fr.greencodeinitiative.java.checks;

import org.junit.jupiter.api.Test;
import org.sonar.java.checks.verifier.CheckVerifier;

class OptimizeImageVectorsSizeTest {

@Test
void test() {
CheckVerifier.newVerifier()
.onFile("src/test/files/OptimizeImageVectorsSize.java")
.withCheck(new OptimizeImageVectorsSize())
.verifyIssues();
}

}