Alexander Wels has posted comments on this change.
Change subject: userportal: Added caching to user portal page webadmin: Added
caching to webadmin page
......................................................................
Patch Set 1: (13 inline comments)
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/CachingFilter.java
Line 55: // Cast to HttpServletRequest/Response.
Line 56: final HttpServletRequest httpRequest = (HttpServletRequest)
request;
Line 57: final HttpServletResponse httpResponse = (HttpServletResponse)
response;
Line 58:
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Done
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 62: httpResponse.setHeader(EXPIRES, getNowPlusYearHttpDate());
Line 63: chain.doFilter(request, responseWrapper);
Line 58:
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 62: httpResponse.setHeader(EXPIRES, getNowPlusYearHttpDate());
No because NOW+1 year of milliseconds is not accurate, years are not static,
they change (leap years and such), the 3 extra operations to properly calculate
a years difference is not going to break the bank.
Line 63: chain.doFilter(request, responseWrapper);
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 62: httpResponse.setHeader(EXPIRES, getNowPlusYearHttpDate());
Line 63: chain.doFilter(request, responseWrapper);
Because the responseWrapper, WRAPs the original response, and it gets the
headers from the original response further down the chain.
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
Line 63: chain.doFilter(request, responseWrapper);
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
No because the expires format is required to be a certain format, I can't just
pop out the default java format. Also certain browsers don't handle jan 1st
1970 well. Better to just calculate yesterday and use that.
Line 68: } else {
Line 69: chain.doFilter(request, response);
Line 70: }
Line 71: }
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
Line 68: } else {
Line 69: chain.doFilter(request, response);
This is an if, else if, else. The else can happen when both other conditions
are not met, so this is needed.
Line 70: }
Line 71: }
Line 72:
Line 73: private String getYesterdayHttpDate() {
Line 69: chain.doFilter(request, response);
Line 70: }
Line 71: }
Line 72:
Line 73: private String getYesterdayHttpDate() {
We do, we need to calculate a date in the past and properly format it for the
header.
Line 74: // Get Calendar instance
Line 75: Calendar calendar = Calendar.getInstance();
Line 76: // Subtract a day, so it is in the past.
Line 77: calendar.add(Calendar.DAY_OF_MONTH, -1);
Line 81: dateFormat.setTimeZone(TimeZone.getTimeZone(GMT));
Line 82: return dateFormat.format(calendar.getTime());
Line 83: }
Line 84:
Line 85: private String getNowPlusYearHttpDate() {
We do need this method, we need to properly calculate a year into the future
and return it in the proper format.
Line 86: // Get Calendar instance
Line 87: Calendar calendar = Calendar.getInstance();
Line 88: // Add a year to now.
Line 89: calendar.add(Calendar.YEAR, 1);
Line 93: dateFormat.setTimeZone(TimeZone.getTimeZone(GMT));
Line 94: return dateFormat.format(calendar.getTime());
Line 95: }
Line 96:
Line 97: private HttpServletResponseWrapper getCacheHeaderResponseWrapper(
I know the wrapper is a subclass of HttpServletResponse, but the name of the
method is getChacheHeaderResponseWrapper, it would make sense that it returns a
wrapper.
Line 98: final HttpServletResponse httpResponse) {
Line 99: return new HttpServletResponseWrapper(httpResponse) {
Line 100: @Override
Line 101: public void setHeader(final String name, final String
value) {
Line 104: httpResponse.setHeader(name, value);
Line 105: }
Line 106: }
Line 107: };
Line 108: }
If you are talking about the @Override for setHeader. We are using the wrapper
to prevent other things down in the chain from setting the expires header. We
are in charge of it, and this code prevents anyone else from setting it.
Line 109:
Line 110: private boolean cacheFilterPatternMathes(HttpServletRequest
httpRequest) {
Line 111: boolean result = false;
Line 112: if (null != cachePattern) {
Line 135: List<String> filterParams = Collections.list(filterConfig
Line 136: .getInitParameterNames());
Line 137: // No need to worry about concurrency, worst case scenario
the same
Line 138: // pattern is calculated a couple of times.
Line 139: if (null == cachePattern) {
Old C code days habit, could potentially still be a problem for boolean
variables. But I can change it.
Line 140: // Get a list of parameters.
Line 141: for (String paramName : filterParams) {
Line 142: if (CACHE.equals(paramName)) {
Line 143: cachePattern = Pattern.compile(filterConfig
Line 137: // No need to worry about concurrency, worst case scenario
the same
Line 138: // pattern is calculated a couple of times.
Line 139: if (null == cachePattern) {
Line 140: // Get a list of parameters.
Line 141: for (String paramName : filterParams) {
Done
Line 142: if (CACHE.equals(paramName)) {
Line 143: cachePattern = Pattern.compile(filterConfig
Line 144: .getInitParameter(paramName));
Line 145: break;
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/UserportalDynamicHostingServlet.java
Line 7: /**
Line 8: * A landing servlet for UserPortal project.
Line 9: *
Line 10: * @author Asaf Shakarchi
Line 11: * @author Alexander Wels
Yes isn't that a Java standard to mark what you worked on?
Line 12: *
Line 13: */
Line 14: @WebServlet(name="UserPortalDynamicHosting",
value="/org.ovirt.engine.ui.userportal.UserPortal/UserPortal.html")
Line 15: public class UserportalDynamicHostingServlet extends
GwtDynamicHostPageServlet {
Line 25: protected void initQueryParams(VdcQueryParametersBase queryParams,
String sessionId) {
Line 26: super.initQueryParams(queryParams, sessionId);
Line 27:
Line 28: // All UserPortal queries are filtered according to user
permissions
Line 29: queryParams.setFiltered(true);
I believe for the UserPortal, setFiltered should be true, for the webadmin
portal setFiltered is set to false by default.
Line 30: }
Line 31:
--
To view, visit http://gerrit.ovirt.org/10449
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d8e02ae542a4aa37bd421bde5582c0f3e9820ad
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
