-
Notifications
You must be signed in to change notification settings - Fork 273
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
UP-4792: Provide an annotation-based mechanism for soffits based on … #813
Conversation
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.
Great start!
This will be a nice addition to Soffits!
I left a few comments and suggestions below.
final String[] beanNames = applicationContext.getBeanDefinitionNames(); | ||
for (String name : beanNames) { | ||
final Object bean = applicationContext.getBean(name); | ||
final Class clazz = AopUtils.isAopProxy(bean) |
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.
would classObject
be clearer than clazz
here?
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.
In my experience, clazz
is a really common variable name for an object of type Class
; it's a convention. http://stackoverflow.com/questions/2529974/why-do-java-programmers-like-to-name-a-variable-clazz
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.
I'd use beanClass
. Say what it is. It's the class of the bean.
*/ | ||
final Map<AnnotatedElement,Object> map = new HashMap<>(); | ||
final String[] beanNames = applicationContext.getBeanDefinitionNames(); | ||
for (String name : beanNames) { |
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.
Would this be a place where using Java 8 streams would be effective?
|
||
logger.debug("Processing model attributes for viewName='{}'", viewName); | ||
|
||
for (Map.Entry<AnnotatedElement,Object> y : modelAttributes.entrySet()) { |
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.
Would this be a place where using Java 8 streams would be effective?
} else { | ||
final String msg = "Unsupported parameter type for SoffitModelAttribute method: " + pType; | ||
throw new UnsupportedOperationException(msg); | ||
} |
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.
Lines 304 to 331 seem to have a high cyclomatic complexity for a fairly straight forward outcome. Could this be simplified?
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.
Yep -- I agree completely.
I wan to break this method down at the same time I introduce some unit tests. I think designing the tests will help me choose wisely in breaking it down.
It's probably also a good idea to move this responsibility into another Java class, since it's more than a little bit of logic.
logger.debug("Selecting cacheScopeValue='{}' for property '{}'", cacheScopeValue, cacheScopeProperty); | ||
|
||
final String cacheMaxAgeProperty = String.format(CACHE_MAXAGE_PROPERTY_FORMAT, module); | ||
final String cacheMaxAgeValue = environment.getProperty(cacheMaxAgeProperty); |
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.
Max age always comes as an integer representing seconds, should that be reflected in the type here?
ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
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.
I usually include units of measure in variable names. I probably didn't in this case because the variable represents a standard HTTP header (with a name that doesn't include a unit). We could nevertheless add it.
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.
I was thinking that maybe it would be final int cacheMaxAgeValue
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.
@drewwills any thoughts on this?
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.
@ChristianMurphy it's a fine change I suppose (and I'll add it).
Now that I understand what you're getting at, I'd say it's more of a validation thing than a clarity thing. The value must be defined as a string, and must go out as a string, but we can validate that it's a valid integer.
* <li>Request and/or Response objects</li> | ||
* <li>Soffit Data Model objects, i.e. PortalRequest, Bearer, Preferences, and Definition</li> | ||
* </ul> | ||
* |
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.
@Since
to document when annotation entered codebase?
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.
Ah, certainly!
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.
Done -- plus I checked all the soffit-related java classes and added @since 5.0
where missing.
…792 (NOTE -- requires uPortal-Project/uPortal#813)
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.
LGTM
@ChristianMurphy @asgrant @bpowell @apetro -- I've added unit tests & documentation (the things I planned to add). I'm planning to merge this PR tomorrow, baring further input. |
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.
Nice work @drewwills!
The code and tests look good 👍
Thanks for adding docs and tests for the changes. 👏
I left a few comments and questions around the documentation.
* Dependencies=*Cache* (recommended) & *Web* (required) | ||
* Additional dependencies you intend to use (optional -- you can add them | ||
later) | ||
1. Add Soffit as a dependency to your project (see below) |
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.
Could the numbered bullet list include the current number?
# IE
1.
2.
3.
4.
# Instead Of
1.
1.
1.
1.
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.
I vote "no" for this one. My sense (before researching) was that it's a bad idea to use actual numbers because...
- It requires human maintenance; if you reorder or add one in the middle, you're obligated to re-do them all
- It's confusing; do the 1, 2, 3, 4 appear in the rendered document as plain text? Or is it markdown syntax? (With 1., 1., 1... it's clearer that Markdown manages the numbering)
But since you asked, I also did a little googling to see what others thought...
Prefer lists only with the marker 1. for ordered lists, unless you intend to refer to items by their number in the same markdown file or externally.
That's just one source, but it's the first hit for "Markdown Style Guide."
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.
It requires human maintenance; if you reorder or add one in the middle, you're obligated to re-do them all
True, #796 can a least partially automate this.
It's confusing; do the 1, 2, 3, 4 appear in the rendered document as plain text? Or is it markdown syntax? (With 1., 1., 1... it's clearer that Markdown manages the numbering)
Its clearer that Markdown does the ordering, its less clear what the actual ordering is while viewing source.
But since you asked, I also did a little googling to see what others thought...
I based styling thus far off off the default remark-lint value which is to include order.
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.
If using
1.
1.
1.
is the decision.
https://github.com/Jasig/uPortal/blob/master/docs/SKINNING_UPORTAL.md
will need to be updated at some point to match
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.
Write for readability. Use the style that includes the numbers in the documentation source.
## In the Portlet Manager | ||
|
||
1. Select _Register New Portlet_ | ||
1. Choose _Portlet_ in the list of types and click _Continue_ |
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.
Could the numbered bullet list include the current number?
Annotate a Spring bean with `@SoffitModelAttribute` to make the entire bean | ||
available within your JSP. | ||
|
||
``` |
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.
Could this be annotated as a Java example?
``` java
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.
done.
Annotate a method on a Spring bean with `@SoffitModelAttribute` to have Soffit | ||
invoke the method and make the return value available within your JSP. | ||
|
||
``` |
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.
Could this be annotated as a Java example?
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.
done.
docs/implement/README.md
Outdated
users, groups, layouts, *etc.* | ||
|
||
1. Authentication (TBD) | ||
1. User Attributes (TBD) |
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.
Could the numbered bullet list include the current number?
your markup (_e.g._ `<h2>Hello World!</h2>`) | ||
1. In `src/main/resources/application.properties`, define the `server.port` | ||
property and set it to an unused port (like 8090) | ||
1. Run the command `$gradle assemble` to build your application |
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.
Adding a $
in front of shell commands in a code block, makes the code harder to copy and paste, is that the intent?
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.
Not at all -- intent was to make it more visually clear that it's a shell command. Another alternative would be to include a space: $ gradle assemble
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.
got it, a space would help then.
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.
Space or removing the $
* [Configuring using JNDI](configure-using-jndi.md) | ||
## Sections | ||
|
||
* [Ant Help](antHelp.txt) |
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.
Unordered bullets in markdown are often given three spaces so all the bullet point styles line up.
* example
+ example
- example
1. example
99. example
* Dependencies=*Cache* (recommended) & *Web* (required) | ||
* Additional dependencies you intend to use (optional -- you can add them | ||
later) | ||
1. Add Soffit as a dependency to your project (see below) |
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.
Write for readability. Use the style that includes the numbers in the documentation source.
want to be using. | ||
|
||
Apereo Soffit is an alternative approach to producing content for uPortal that | ||
is not based on JSR-286 or the portlet container. |
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.
Soffits convey mode and window state, so are conceptually based on JSR-286
and the idioms of a portlet container.
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.
Those 2 concepts pre-date JSR-286 and (IMHO) are reasonably applicable to "dashboardy" web applications broadly. The point is that if -- as a producer of complex content for uPortal -- you want to leave portlets behind, you can do that with Soffits.
You won't need any...
- Portlet API Java classes
- Portlet JSP tag library
- portlet.xml file
$ ant deployPortletApp
- Action/Event/Render/Resource lifecycle
The Soffit approach give you a 100% break -- as a developer of content -- from portlety things.
@@ -57,6 +57,10 @@ | |||
import org.springframework.web.bind.annotation.RequestMapping; | |||
import org.springframework.web.portlet.bind.annotation.RenderMapping; | |||
|
|||
/** | |||
* @since 5.0 | |||
* @author drewwills |
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.
I'd favor a style of not introducing @author
annotations. They don't age well in shared-owner, shared-maintenance, shared-development code. The Git history is sufficient indication of the author.
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.
That makes sense to me. Some IDEs add them, and we have tons of them in existing .java files.
@@ -0,0 +1,46 @@ | |||
# uPortal Soffits |
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.
Still hoping for a name other than "Soffit".
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.
For my part, I'm not unwilling to switch the name.
Perhaps just the more generic "Module?"
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.
I do hope we can have modules in the portal that are not Soffits.
My issue with the name "Soffit" is that it obfuscates what these are. "Underside of an arch" doesn't help me understand what these are. They might be tunnels, or tubes, or tentacles, or puppet strings, or WSRP-but-no-SOAP.
Maybe URP "uPortal Remote Portlets"? Conceptually that's what this is, it's WSRP except not handcuffed to a dead-on-arrival formal specification and instead ad-hoc inventing what uPortal might need. URC "uPortal Remote Channels" to escape the overloaded "portlet" word? (I knew Channels would rise again.)
The linked naming discussion I thought was pretty good before it went off the rails with the "microservices" reference.
your markup (_e.g._ `<h2>Hello World!</h2>`) | ||
1. In `src/main/resources/application.properties`, define the `server.port` | ||
property and set it to an unused port (like 8090) | ||
1. Run the command `$gradle assemble` to build your application |
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.
Space or removing the $
@drewwills a merge conflict has cropped up in |
@ChristianMurphy of course. |
…he SoffitRendererController to inject model attributes
…unit tests for ModelAttributeService
…the SoffitRendererController to inject model attributes
https://issues.jasig.org/browse/UP-4792
Checklist
Description of change
This feature is inspired by the
ModelAttribute
annotation in Spring MVC: https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/web/bind/annotation/ModelAttribute.htmluPortal soffit support should provide a similar annotation (e.g.
SoffitModelAttribute
) that allows soffit developers to inject arbitrary beans and/or return values on bean methods into the EL context when a JSP is rendered via the SoffitRendererController.Code Examples
Bean class that uses
@SoffitModelAttribute
JSP page that uses these model attributes