You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by cnenning <gi...@git.apache.org> on 2016/01/25 13:56:00 UTC
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
GitHub user cnenning opened a pull request:
https://github.com/apache/struts/pull/85
WW-4594: Configure TilesDefs by annotating Actions
Adds annotations for each element from `tiles.xml` to annotate actions. Those annotations are processed by a new class in tiles-plugin which is used by TilesResult.
With those annotations it is possible to keep `tiles.xml` very short (e.g. just put layout in there) and configure concrete tiles-definitions just by annotating actions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cnenning/struts master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/struts/pull/85.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #85
----
commit d9f4054b1367cd7ab6e3f22b9cc677f62def4e83
Author: cnenning <cn...@apache.org>
Date: 2016-01-22T13:59:48Z
fixed tiles showcase by setting dtd to 3.0
commit 9ac326aa2458fe43140c1b13b61c87752d282e3d
Author: cnenning <cn...@apache.org>
Date: 2016-01-22T14:27:09Z
Added tiles annotations, see WW-4594.
Added tiles annotations, created StrutsTilesAnnotationProcessor to
create Definitons from them and using it in TilesResult.
commit e50c37c5ba5781900edf53f9ec71b8649471d448
Author: cnenning <cn...@apache.org>
Date: 2016-01-22T14:27:50Z
added sample for tiles annotations
commit d76357fd829a3ea8ddca21d625c49b606cca88d5
Author: cnenning <cn...@apache.org>
Date: 2016-01-25T10:23:10Z
added tests for StrutsTilesAnnotationProcessor
commit a53deac7ce8732053edd43dddac329448055aef0
Author: cnenning <cn...@apache.org>
Date: 2016-01-25T12:39:47Z
updated javadoc
commit b1588ddc84d876a676bab66ad80ec34637e98536
Author: cnenning <cn...@apache.org>
Date: 2016-01-25T12:50:45Z
fixed line endings
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r50692670
--- Diff: plugins/tiles/src/test/java/org/apache/struts2/tiles/TestStrutsTilesAnnotationProcessor.java ---
@@ -0,0 +1,148 @@
+package org.apache.struts2.tiles;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.struts2.tiles.annotation.TilesDefinition;
+import org.apache.tiles.Attribute;
+import org.apache.tiles.Definition;
+import org.apache.tiles.Expression;
+import org.junit.Test;
+
+import org.junit.Assert;
+
+public class TestStrutsTilesAnnotationProcessor {
--- End diff --
Can we keep convention and instead of `TestStrutsTilesAnnotationProcessor` use `StrutsTilesAnnotationProcessorTest`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51284943
--- Diff: plugins/tiles/src/main/java/org/apache/struts2/views/tiles/TilesResult.java ---
@@ -99,6 +109,18 @@ public TilesResult(String location) {
* HTTP request.
*/
public void doExecute(String location, ActionInvocation invocation) throws Exception {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = null;
+ Object action = invocation.getAction();
+ String actionName = invocation.getInvocationContext().getName();
+
+ if (StringUtils.isEmpty(location)) {
+ // location not set -> action must have one @TilesDefinition
--- End diff --
Could you turn this comment into a `LOG.trace`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/struts/pull/85
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51285068
--- Diff: plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsTilesAnnotationProcessorTest.java ---
@@ -0,0 +1,148 @@
+package org.apache.struts2.tiles;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.struts2.tiles.annotation.TilesDefinition;
+import org.apache.tiles.Attribute;
+import org.apache.tiles.Definition;
+import org.apache.tiles.Expression;
+import org.junit.Test;
+
+import org.junit.Assert;
+
+public class StrutsTilesAnnotationProcessorTest {
+
+ @Test
+ public void findAnnotationSingleAction() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionSingleAnnotation(), null);
+ Assert.assertNotNull(tilesDefinition);
+ Assert.assertEquals("definition-name", tilesDefinition.name());
+ }
+
+ @Test
+ public void findAnnotationMultipleActionNameNull() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionMultipleAnnotations(), null);
+ Assert.assertNotNull(tilesDefinition);
+ Assert.assertEquals("def1", tilesDefinition.name());
+ }
+
+ @Test
+ public void findAnnotationMultipleActionNameGiven() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionMultipleAnnotations(), "def2");
+ Assert.assertNotNull(tilesDefinition);
+ Assert.assertEquals("def2", tilesDefinition.name());
+ }
+
+ @Test
+ public void findAnnotationMultipleActionNotFound() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionMultipleAnnotations(), "def3");
+ Assert.assertNull(tilesDefinition);
+ }
+
+ @Test
+ public void buildDefiniton() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionSingleAnnotation(), null);
+
+ Definition definition = annotationProcessor.buildTilesDefinition("tileName", tilesDefinition);
+
+ Assert.assertNotNull(definition);
+ Assert.assertEquals("tileName", definition.getName());
+ Assert.assertEquals("preparer", definition.getPreparer());
+ Assert.assertEquals("base-definition", definition.getExtends());
+ Attribute templateAttribute = definition.getTemplateAttribute();
+ Assert.assertEquals("template", templateAttribute.getValue());
+ Assert.assertEquals("type", templateAttribute.getRenderer());
+ Assert.assertEquals("role", templateAttribute.getRole());
+ Expression definitionExpressionObject = templateAttribute.getExpressionObject();
+ Assert.assertEquals("templ*", definitionExpressionObject.getExpression());
+ Assert.assertNull(definitionExpressionObject.getLanguage());
+
+ Attribute putAttribute = definition.getAttribute("put-attr");
+ Assert.assertNotNull(putAttribute);
+ Assert.assertEquals("attr-val", putAttribute.getValue());
+ Assert.assertEquals("attr-type", putAttribute.getRenderer());
+ Assert.assertEquals("attr-role", putAttribute.getRole());
+ Expression putAttrExpressionObject = putAttribute.getExpressionObject();
+ Assert.assertEquals("expr", putAttrExpressionObject.getExpression());
+ Assert.assertEquals("lang", putAttrExpressionObject.getLanguage());
+
+ Attribute listAttribute = definition.getAttribute("list-name");
+ Assert.assertEquals("list-role", listAttribute.getRole());
+ List<Attribute> listValue = getListValue(listAttribute);
+ Assert.assertEquals(2, listValue.size());
+
+ Attribute addAttribute = listValue.get(0);
+ Assert.assertEquals("list-attr-role", addAttribute.getRole());
+ Assert.assertEquals("list-attr-val", addAttribute.getValue());
+ Assert.assertEquals("list-attr-type", addAttribute.getRenderer());
+ Expression addAttrExpressionObject = addAttribute.getExpressionObject();
+ Assert.assertEquals("list-attr-expr", addAttrExpressionObject.getExpression());
+
+ Attribute addListAttribute = listValue.get(1);
+ Assert.assertEquals("list-list-attr-role", addListAttribute.getRole());
+ List<Attribute> addListValue = getListValue(addListAttribute);
+ Assert.assertEquals(1, addListValue.size());
+ Assert.assertEquals("list-list-add-attr", addListValue.get(0).getValue());
+
+ Set<String> cascadedAttributeNames = definition.getCascadedAttributeNames();
+ Assert.assertEquals(2, cascadedAttributeNames.size());
+ Assert.assertTrue(cascadedAttributeNames.contains("put-attr"));
+ Assert.assertTrue(cascadedAttributeNames.contains("list-name"));
+ }
+
+ @Test
+ public void buildDefinitonAllEmpty() {
+ StrutsTilesAnnotationProcessor annotationProcessor = new StrutsTilesAnnotationProcessor();
+ TilesDefinition tilesDefinition = annotationProcessor.findAnnotation(new TilesTestActionSingleAnnotationAllEmpty(), null);
+
+ Definition definition = annotationProcessor.buildTilesDefinition(null, tilesDefinition);
+
+ Assert.assertNotNull(definition);
+ Assert.assertNull(definition.getName());
+ Assert.assertNull(definition.getPreparer());
+ Assert.assertNull(definition.getExtends());
+ Attribute templateAttribute = definition.getTemplateAttribute();
+ Assert.assertNull(templateAttribute.getValue());
+ //Assert.assertNull(templateAttribute.getRenderer());
--- End diff --
If not used maybe it can be dropped?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by cnenning <gi...@git.apache.org>.
Github user cnenning commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r50693282
--- Diff: plugins/tiles/src/test/java/org/apache/struts2/tiles/TestStrutsTilesAnnotationProcessor.java ---
@@ -0,0 +1,148 @@
+package org.apache.struts2.tiles;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.struts2.tiles.annotation.TilesDefinition;
+import org.apache.tiles.Attribute;
+import org.apache.tiles.Definition;
+import org.apache.tiles.Expression;
+import org.junit.Test;
+
+import org.junit.Assert;
+
+public class TestStrutsTilesAnnotationProcessor {
--- End diff --
oh, of course :sweat_smile:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51285002
--- Diff: plugins/tiles/src/main/java/org/apache/struts2/views/tiles/TilesResult.java ---
@@ -111,6 +133,31 @@ public void doExecute(String location, ActionInvocation invocation) throws Excep
Request request = new ServletRequest(applicationContext, httpRequest, httpResponse);
+ boolean definitionValid = false;
+ try {
+ LOG.debug("checking if tiles definition exists '{}'", location);
+ definitionValid = container.isValidDefinition(location, request);
+ } catch (TilesException e) {
+ LOG.warn("got TilesException while checking if definiton exists, ignoring it", e);
+ }
+ if (!definitionValid) {
+ if (tilesDefinition == null) {
+ // tilesDefinition not found yet, search in action
--- End diff --
The same here, `LOG.trace`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on the pull request:
https://github.com/apache/struts/pull/85#issuecomment-179082503
Great!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on the pull request:
https://github.com/apache/struts/pull/85#issuecomment-176859924
Left some minor comment, this looks solid, great job!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51284785
--- Diff: apps/showcase/src/main/java/org/apache/struts2/showcase/tiles/TilesAnnotationsAction.java ---
@@ -0,0 +1,21 @@
+package org.apache.struts2.showcase.tiles;
--- End diff --
Could you add license header to top of the file?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by cnenning <gi...@git.apache.org>.
Github user cnenning commented on the pull request:
https://github.com/apache/struts/pull/85#issuecomment-179082240
Alright, thanks for reviewing. I agree with your findings and pushed updates.
I'm going to merge it later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51284673
--- Diff: apps/showcase/src/main/java/org/apache/struts2/showcase/tiles/TilesAnnotationsAction.java ---
@@ -0,0 +1,21 @@
+package org.apache.struts2.showcase.tiles;
+
+import org.apache.struts2.convention.annotation.Namespace;
+import org.apache.struts2.convention.annotation.ParentPackage;
+import org.apache.struts2.convention.annotation.Result;
+import org.apache.struts2.tiles.annotation.TilesDefinition;
+import org.apache.struts2.tiles.annotation.TilesPutAttribute;
+
+import com.opensymphony.xwork2.ActionSupport;
+
+@Namespace("/tiles")
+@ParentPackage("tiles")
+@Result(name = "success", type="tiles")
+@TilesDefinition(extend = "showcase.annotations", putAttributes = {
+ @TilesPutAttribute(name = "header", value = "/WEB-INF/tiles/header.jsp"),
+ @TilesPutAttribute(name = "body", value = "/WEB-INF/tiles/body.ftl"), })
--- End diff --
I think the last comma isn't needed and also what do you think about formatting like this:
```java
@Namespace("/tiles")
@ParentPackage("tiles")
@Result(name = "success", type="tiles")
@TilesDefinition(extend = "showcase.annotations", putAttributes = {
@TilesPutAttribute(name = "header", value = "/WEB-INF/tiles/header.jsp"),
@TilesPutAttribute(name = "body", value = "/WEB-INF/tiles/body.ftl")
})
```
?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4594: Configure TilesDefs by annotating Ac...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/85#discussion_r51285182
--- Diff: plugins/tiles/src/test/java/org/apache/struts2/tiles/TilesTestActionSingleAnnotation.java ---
@@ -0,0 +1,49 @@
+package org.apache.struts2.tiles;
+
+import org.apache.struts2.tiles.annotation.TilesAddAttribute;
+import org.apache.struts2.tiles.annotation.TilesAddListAttribute;
+import org.apache.struts2.tiles.annotation.TilesDefinition;
+import org.apache.struts2.tiles.annotation.TilesPutAttribute;
+import org.apache.struts2.tiles.annotation.TilesPutListAttribute;
+
+@TilesDefinition(
+ name = "definition-name",
+ extend = "base-definition",
+ preparer = "preparer",
+ role = "role",
+ template = "template",
+ templateExpression = "templ*",
+ templateType = "type",
+ putAttributes = {
+ @TilesPutAttribute(
+ cascade = true,
+ expression = "lang:expr",
+ name = "put-attr",
+ role = "attr-role",
+ type = "attr-type",
+ value = "attr-val")
+ },
+ putListAttributes = {
+ @TilesPutListAttribute(
+ cascade = true,
+ inherit = true,
+ name = "list-name",
+ role = "list-role",
+ addAttributes = {
+ @TilesAddAttribute(
+ expression = "list-attr-expr",
+ role = "list-attr-role",
+ type = "list-attr-type",
+ value = "list-attr-val")
+ },
+ addListAttributes = {
+ @TilesAddListAttribute(
+ role = "list-list-attr-role",
+ addAttributes = {@TilesAddAttribute("list-list-add-attr")})
+ }
+ )
+ }
+)
--- End diff --
Extreme example :D
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org