You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2021/01/02 16:53:37 UTC
[GitHub] [struts] lukaszlenart opened a new pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
lukaszlenart opened a new pull request #462:
URL: https://github.com/apache/struts/pull/462
Implements [WW-5021](https://issues.apache.org/jira/browse/WW-5021)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r552245545
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -523,6 +525,11 @@ public void setUIThemeExpansionToken(String uiThemeExpansionToken) {
this.uiThemeExpansionToken = uiThemeExpansionToken;
}
+ @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+ public void setUiStaticContentPath(String uiStaticContentPath) {
+ this.uiStaticContentPath = uiStaticContentPath;
Review comment:
It looks like the _staticContentPath_ logic depends on the String not ending with/terminating with "/", nor being null or empty. Maybe the setter should refuse to accept certain "unacceptable" values, or at least produce log warnings if they are encountered ?
For example:
```
if (uiStaticContentPath == null || uiStaticContentPath.isEmpty() || uiStaticContentPath.endsWith("/") {
throw new IllegalArgumentException("Static content path cannot be null, empty or end with '/'. Provided value:" + uiStaticContentPath);
// or
LOG.warn("Static content path cannot be null, empty or end with '/'. Provided value:{0}", uiStaticContentPath);
}
```
Refusal via exception might be safer/cleaner, if it makes sense, since it cannot be ignored. That behaviour could be unit tested as well.
If exceptions are too disruptive, then maybe log a warning and fall-back (override) to the "/static" default path if an unacceptable value is provided to the setter ?
##########
File path: core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
##########
@@ -1344,4 +1346,12 @@ public Integer getOgnlAutoGrowthCollectionLimit() {
public void setOgnlAutoGrowthCollectionLimit(Integer ognlAutoGrowthCollectionLimit) {
this.ognlAutoGrowthCollectionLimit = ognlAutoGrowthCollectionLimit;
}
+
+ public String getStaticContentPath() {
+ return staticContentPath;
+ }
+
+ public void setStaticContentPath(String staticContentPath) {
+ this.staticContentPath = staticContentPath;
Review comment:
If a guard or log makes sense for `UIBean`, it is the same consideration here.
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java
##########
@@ -100,20 +110,23 @@
/**
* Modify state of StrutsConstants.STRUTS_SERVE_STATIC_CONTENT setting.
*
- * @param serveStaticContent
- * New setting
+ * @param serveStaticContent New setting
*/
@Inject(StrutsConstants.STRUTS_SERVE_STATIC_CONTENT)
public void setServeStaticContent(String serveStaticContent) {
this.serveStatic = BooleanUtils.toBoolean(serveStaticContent);
}
+ @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+ public void setStaticContentPath(String staticContentPath) {
+ this.staticContentPath = staticContentPath;
Review comment:
If a guard or log makes sense for `UIBean` and `ConstantConfig`, it is the same consideration here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r552736741
##########
File path: core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
##########
@@ -1344,4 +1346,12 @@ public Integer getOgnlAutoGrowthCollectionLimit() {
public void setOgnlAutoGrowthCollectionLimit(Integer ognlAutoGrowthCollectionLimit) {
this.ognlAutoGrowthCollectionLimit = ognlAutoGrowthCollectionLimit;
}
+
+ public String getStaticContentPath() {
+ return staticContentPath;
+ }
+
+ public void setStaticContentPath(String staticContentPath) {
+ this.staticContentPath = staticContentPath;
Review comment:
Same here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #462:
URL: https://github.com/apache/struts/pull/462#issuecomment-753501657
[](https://coveralls.io/builds/36185490)
Coverage increased (+0.06%) to 49.88% when pulling **1f70627058d74cf9911e1c52234437bb866b2bdf on WW-5021-static-content-path** into **213990944ffdc131b4f7595f52f05f5fb8d56b31 on master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #462:
URL: https://github.com/apache/struts/pull/462#issuecomment-753501657
[](https://coveralls.io/builds/36130807)
Coverage increased (+0.06%) to 49.877% when pulling **f956910d20bbc162b4ba9bbc8899f88df9f8917a on WW-5021-static-content-path** into **213990944ffdc131b4f7595f52f05f5fb8d56b31 on master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r552736634
##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -523,6 +525,11 @@ public void setUIThemeExpansionToken(String uiThemeExpansionToken) {
this.uiThemeExpansionToken = uiThemeExpansionToken;
}
+ @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+ public void setUiStaticContentPath(String uiStaticContentPath) {
+ this.uiStaticContentPath = uiStaticContentPath;
Review comment:
Right, added warning and fallback
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart merged pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #462:
URL: https://github.com/apache/struts/pull/462
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] coveralls commented on pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #462:
URL: https://github.com/apache/struts/pull/462#issuecomment-753501657
[](https://coveralls.io/builds/36040596)
Coverage increased (+0.003%) to 49.824% when pulling **ef72783f502e5773ab630d1f4238efc27b6c554f on WW-5021-static-content-path** into **213990944ffdc131b4f7595f52f05f5fb8d56b31 on master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r553786793
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/StaticContentLoader.java
##########
@@ -57,4 +67,32 @@
*/
void findStaticResource(String path, HttpServletRequest request, HttpServletResponse response) throws IOException;
+ class Validator {
+
+ private static final Logger LOG = LogManager.getLogger(DefaultStaticContentLoader.class);
+
+ public static String validateStaticContentPath(String uiStaticContentPath) {
+ if (StringUtils.isBlank(uiStaticContentPath)) {
+ LOG.warn("\"{}\" has been set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if ("/".equals(uiStaticContentPath)) {
+ LOG.warn("\"{}\" cannot be set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if(!uiStaticContentPath.startsWith("/")) {
Review comment:
fixed!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r553788293
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/StaticContentLoader.java
##########
@@ -57,4 +67,32 @@
*/
void findStaticResource(String path, HttpServletRequest request, HttpServletResponse response) throws IOException;
+ class Validator {
+
+ private static final Logger LOG = LogManager.getLogger(DefaultStaticContentLoader.class);
+
+ public static String validateStaticContentPath(String uiStaticContentPath) {
+ if (StringUtils.isBlank(uiStaticContentPath)) {
+ LOG.warn("\"{}\" has been set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if ("/".equals(uiStaticContentPath)) {
+ LOG.warn("\"{}\" cannot be set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if(!uiStaticContentPath.startsWith("/")) {
+ LOG.warn("\"{}\" must start with \"/\", but has been set to \"{}\", prepending the missing \"/\"!",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath);
+ return "/" + uiStaticContentPath;
+ } else {
Review comment:
Right, on the first thought I want to make the trailing "/" required, but after all I decided the above approach is better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r552737419
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java
##########
@@ -100,20 +110,23 @@
/**
* Modify state of StrutsConstants.STRUTS_SERVE_STATIC_CONTENT setting.
*
- * @param serveStaticContent
- * New setting
+ * @param serveStaticContent New setting
*/
@Inject(StrutsConstants.STRUTS_SERVE_STATIC_CONTENT)
public void setServeStaticContent(String serveStaticContent) {
this.serveStatic = BooleanUtils.toBoolean(serveStaticContent);
}
+ @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+ public void setStaticContentPath(String staticContentPath) {
+ this.staticContentPath = staticContentPath;
Review comment:
Right, done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r553431390
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/StaticContentLoader.java
##########
@@ -57,4 +67,32 @@
*/
void findStaticResource(String path, HttpServletRequest request, HttpServletResponse response) throws IOException;
+ class Validator {
+
+ private static final Logger LOG = LogManager.getLogger(DefaultStaticContentLoader.class);
+
+ public static String validateStaticContentPath(String uiStaticContentPath) {
+ if (StringUtils.isBlank(uiStaticContentPath)) {
+ LOG.warn("\"{}\" has been set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if ("/".equals(uiStaticContentPath)) {
+ LOG.warn("\"{}\" cannot be set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if(!uiStaticContentPath.startsWith("/")) {
Review comment:
Minor spacing item: "} else if(" could be "} else if ("
##########
File path: core/src/main/java/org/apache/struts2/dispatcher/StaticContentLoader.java
##########
@@ -57,4 +67,32 @@
*/
void findStaticResource(String path, HttpServletRequest request, HttpServletResponse response) throws IOException;
+ class Validator {
+
+ private static final Logger LOG = LogManager.getLogger(DefaultStaticContentLoader.class);
+
+ public static String validateStaticContentPath(String uiStaticContentPath) {
+ if (StringUtils.isBlank(uiStaticContentPath)) {
+ LOG.warn("\"{}\" has been set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if ("/".equals(uiStaticContentPath)) {
+ LOG.warn("\"{}\" cannot be set to \"{}\", falling back into default value \"{}\"",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath,
+ StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH);
+ return StaticContentLoader.DEFAULT_STATIC_CONTENT_PATH;
+ } else if(!uiStaticContentPath.startsWith("/")) {
+ LOG.warn("\"{}\" must start with \"/\", but has been set to \"{}\", prepending the missing \"/\"!",
+ StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
+ uiStaticContentPath);
+ return "/" + uiStaticContentPath;
+ } else {
Review comment:
I think there may be one validation condition missing from the checks (trailing "/"), as it looks like that would break processing as well. Adding a new clause like this:
```
} else if (uiStaticContentPath.endsWith("/")) {
LOG.warn("\"{}\" must not end with \"/\", but has been set to \"{}\", removing all trailing \"/\"!",
StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH,
uiStaticContentPath);
return StringUtils.stripEnd(uiStaticContentPath, "/");
}
```
before the final else might work. What do you think ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #462:
URL: https://github.com/apache/struts/pull/462#issuecomment-753501657
[](https://coveralls.io/builds/36185490)
Coverage increased (+0.06%) to 49.88% when pulling **1f70627058d74cf9911e1c52234437bb866b2bdf on WW-5021-static-content-path** into **213990944ffdc131b4f7595f52f05f5fb8d56b31 on master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org