-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fiqare perseo-core improvements #151
Changes from 64 commits
6d25f8d
ab122f2
f651423
a1c8d23
fa2197f
99263f2
5c7825b
cbfad26
18d5393
c4c89f3
04e9cbb
e90be87
9dc7421
032f6b8
c361bf3
8ff7828
8050a3d
86f9044
6fa32ad
7de5632
40094b8
3e386ac
2ad08b7
bc7419d
30ef6b4
4ab8d47
149d591
cd90313
5e43ec8
65ac79d
7ccb457
c0dfb12
dd1ad0e
5852c95
0ac3d7e
33c8328
e53c7eb
4eb356c
7883507
1395a50
b510393
4ddbdb4
6e26193
5adca52
f73d8f1
d08fdcd
b694d53
ed78f2f
8e42d0e
f94f1bf
b38f18b
5491d95
fb3005d
a17edf1
9eb801a
0413a41
5b174d7
6ad2461
97b23e9
7a1e6e5
90a8cba
5c7b891
64f4713
a5ff18a
660056e
654f7bc
d3cd3f4
4d08aff
05982ba
c8154a4
61b8a4c
cff907d
47ab836
b4a4d73
b6d7148
728e600
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,10 @@ | |
|
||
.idea | ||
.swp | ||
.classpath | ||
.project | ||
.scannerwork | ||
.settings | ||
|
||
# virtual machine crash logs, see http://www.java.com/en/download/help/error_hotspot.xml | ||
hs_err_pid* | ||
|
@@ -18,3 +22,4 @@ target | |
nb-configuration.xml | ||
nbactions.xml | ||
|
||
"" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this "" ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4d08aff |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,7 @@ jdk: | |
- openjdk11 | ||
branches: | ||
only: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be changed in .travis.yml. Please, rollback the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4d08aff |
||
- /.*/ | ||
|
||
- fiqare-perseo-core-improvements | ||
install: | ||
- mvn test -B | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,11 @@ | |
<version>1.1.1</version> | ||
<scope>compile</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.owasp.encoder</groupId> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What this new dependency does ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In several classes the following issue has been detected: Security - Potential XSS in Servlet For example:
Solution:
References: Due to this issue, org.owasp.encoder dependency has been added. |
||
<artifactId>encoder</artifactId> | ||
<version>1.2.2</version> | ||
</dependency> | ||
</dependencies> | ||
|
||
<build> | ||
|
@@ -122,31 +127,6 @@ | |
<failOnMissingWebXml>false</failOnMissingWebXml> | ||
</configuration> | ||
</plugin> | ||
<plugin> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate on the removal of this plugin, please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d3cd3f4 |
||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-dependency-plugin</artifactId> | ||
<version>3.1.1</version> | ||
<executions> | ||
<execution> | ||
<phase>validate</phase> | ||
<goals> | ||
<goal>copy</goal> | ||
</goals> | ||
<configuration> | ||
<outputDirectory>${endorsed.dir}</outputDirectory> | ||
<silent>true</silent> | ||
<artifactItems> | ||
<artifactItem> | ||
<groupId>javax</groupId> | ||
<artifactId>javaee-endorsed-api</artifactId> | ||
<version>6.0</version> | ||
<type>jar</type> | ||
</artifactItem> | ||
</artifactItems> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-site-plugin</artifactId> | ||
|
@@ -194,4 +174,4 @@ | |
</plugin> | ||
</plugins> | ||
</reporting> | ||
</project> | ||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
sonar.projectKey=fiware-perseo-core-dev-jafernandez | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand this connect with some sonar server out there. Could you elaborate on this? Where is that server located? Who is managing it? If this is a local configuration (i.e. for a local sonar server) it shouldn't be included in perseo master branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4d08aff |
||
sonar.projectName=Fiware Perseo Core Dev JaFernandez | ||
sonar.projectVersion=1.0 | ||
sonar.sources=. | ||
sonar.java.binaries=. | ||
sonar.exclusions=target/** | ||
sonar.junit.reportPaths=target/surefire-reports | ||
sonar.jacoco.reportPaths=target/jacoco.exec | ||
sonar.coverage.jacoco.xmlReportPaths=target/site/jacoco/jacoco.xml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,8 @@ | |
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.util.Properties; | ||
import java.util.ResourceBundle; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
|
@@ -45,15 +47,15 @@ private Configuration() { | |
private static final String PERSEO_MAX_AGE_ENV = "MAX_AGE"; | ||
|
||
private static final Properties PROPERTIES = new Properties(); | ||
private static final String PATH = "/etc/perseo-core.properties"; | ||
private static final String PATH = ResourceBundle.getBundle("perseo-core").getString("properties.path"); | ||
private static final String ACTION_URL_PROP = "action.url"; | ||
private static final String MAX_AGE_PROP = "rule.max_age"; | ||
|
||
private static String actionRule; | ||
private static long maxAge; | ||
|
||
static { | ||
LOGGER.debug("Configuration init: " + reload()); | ||
LOGGER.debug(String.format("Configuration init: %s",reload())); | ||
} | ||
|
||
/** | ||
|
@@ -67,7 +69,7 @@ public static synchronized boolean reload() { | |
InputStream stream; | ||
String defaultMaxAge; | ||
String defaultURL; | ||
String actionPath = "/actions/do"; | ||
String actionPath = ResourceBundle.getBundle("perseo-core").getString("action.path"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure of understanding this change... where "action.path" configuration is located in the resources of the proyect? Some .properties file? Which one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is this is due to an issue of the portability axis (URIs should not be hardcoded). "action.path" as been located in perseo-core.properties file. References: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NTC |
||
|
||
// Check configuration file. If exist, set as default configuration for perseo-core | ||
try { | ||
|
@@ -95,21 +97,21 @@ public static synchronized boolean reload() { | |
// Add actions/do path if perseoFeURLEnv not contains it yet | ||
actionRule = perseoFeURLEnv.contains(actionPath) ? perseoFeURLEnv : perseoFeURLEnv + actionPath; | ||
} else { | ||
LOGGER.error("Invalid value for " + PERSEO_FE_URL_ENV + ": " + perseoFeURLEnv); | ||
LOGGER.error(String.format("Invalid value for %s: %s",PERSEO_FE_URL_ENV, perseoFeURLEnv)); | ||
return false; | ||
} | ||
LOGGER.info("actionRule configuration is: " + actionRule); | ||
LOGGER.info(String.format("actionRule configuration is: %s",actionRule)); | ||
|
||
// Get MAX_AGE from env var if exist, else default | ||
String maxAgeEnv = System.getenv(PERSEO_MAX_AGE_ENV); | ||
// Check maxAge numerical value | ||
try { | ||
maxAge = maxAgeEnv != null ? Long.parseLong(maxAgeEnv) : Long.parseLong(defaultMaxAge); | ||
} catch (NumberFormatException nfe) { | ||
LOGGER.error("Invalid value for " + PERSEO_MAX_AGE_ENV + ": " + nfe); | ||
LOGGER.error(String.format("Invalid value for %s: %s",PERSEO_MAX_AGE_ENV,nfe)); | ||
return false; | ||
} | ||
LOGGER.info("maxAge configuration is: " + maxAge); | ||
LOGGER.info(String.format("maxAge configuration is: %s",maxAge)); | ||
|
||
return true; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ | |
import org.apache.log4j.MDC; | ||
import org.json.JSONException; | ||
import org.json.JSONObject; | ||
import org.owasp.encoder.Encode; | ||
|
||
/** | ||
* | ||
|
@@ -42,7 +43,7 @@ | |
public class EventsServlet extends HttpServlet { | ||
|
||
private static final Logger logger = LoggerFactory.getLogger(EventsServlet.class); | ||
EPServiceProvider epService; | ||
private static EPServiceProvider epService; | ||
|
||
@Override | ||
public void init() { | ||
|
@@ -76,25 +77,27 @@ protected void doPost(HttpServletRequest request, HttpServletResponse response) | |
Utils.putCorrelatorAndTrans(request); | ||
logger.debug("events doPost"); | ||
response.setCharacterEncoding("UTF-8"); | ||
response.setContentType("application/json;charset=UTF-8"); | ||
PrintWriter out = response.getWriter(); | ||
try { | ||
StringBuilder sb = new StringBuilder(); | ||
response.setContentType("application/json;charset=UTF-8"); | ||
try { | ||
String eventText = Utils.getBodyAsString(request); | ||
logger.info("incoming event:" + eventText); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a general change from string concatenation style (+) to format(). Why this change is neeed? Or, in other words, why usage of format() is prefereable over string concatenation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is this is due to an issue of the maintainability axis that suggests using format() instead of string concatenation. References: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine. NTC. |
||
logger.info(String.format("incoming event: %s", eventText)); | ||
org.json.JSONObject jo = new JSONObject(eventText); | ||
logger.debug("event as JSONObject: " + jo); | ||
logger.debug(String.format("event as JSONObject: %s", jo)); | ||
Map<String, Object> eventMap = Utils.JSONObject2Map(jo); | ||
logger.debug("event as map: " + eventMap); | ||
logger.debug(String.format("event as map: %s" , eventMap)); | ||
epService.getEPRuntime().sendEvent(eventMap, Constants.IOT_EVENT); | ||
logger.debug("event was sent: " + eventMap); | ||
} catch (JSONException je) { | ||
logger.error("error: " + je); | ||
response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
out.printf("{\"error\":\"%s\"}\n", je.getMessage()); | ||
|
||
} finally { | ||
out.close(); | ||
logger.debug(String.format("event was sent: %s", eventMap)); | ||
} catch (Exception je) { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please review indentation on this block. Have you done a checkstyle pass (configured using telefonica_checkstyle.xml file in the root of the repository) after the changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d3cd3f4 |
||
logger.error(String.format("error: %s" ,je)); | ||
response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
response.getOutputStream().print(String.format("{\"error\":\"%s\"}%n", Encode.forHtmlContent(je.getMessage()))); | ||
}catch(IOException exception) { | ||
response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
} | ||
|
||
|
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to add .project, .settings and .classpath, as they are typically used by IDES. However, what about .scannerwork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4d08aff