-
Notifications
You must be signed in to change notification settings - Fork 571
Servlet filters support #18
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
Conversation
Still open 1. Support filter annotations 2. Add support for filters in spark
Bug fixes and additional unit tests. Annotation support completes the feature required to address #15. The service does not automatically load annotated filter classes, these will have to be added manually with the new startup event.
jt0
left a comment
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.
Partial review.
| protected Cookie[] parseCookies(String headerValue) { | ||
| List<Cookie> output = new ArrayList<>(); | ||
|
|
||
| for (AbstractMap.SimpleEntry<String, String> entry : this.parseHeaderValue(headerValue)) { |
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.
Replace AbstractMap.SimpleEntry w/ Map.Entry.
Also, consider replacing w/ Java8 streams:
List<Map.Entry<String, String>> parsedHeaders = this.parseHeaderValue(headerValue);
return parsedHeaders.stream()
.filter(e -> e.getKey() != null)
.map(e -> new Cookie(e.getKey(), e.getValue()))
.collect(Collectors.toList())
.toArray(new Cookie[entrySet.size()]);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.
neat trick!
| * @param headerValue The string value of the HTTP Cookie header | ||
| * @return An array of Cookie objects from the header | ||
| */ | ||
| protected Cookie[] parseCookies(String headerValue) { |
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.
Rename to cookieHeaderValue or similar.
|
|
||
|
|
||
| protected String readPathInfo(String path, String resource) { | ||
| // TODO: Implement |
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.
Needed?
|
|
||
|
|
||
| protected String readPathTranslated(String path) { | ||
| // TODO: Implement |
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.
Needed?
| attributes = new HashMap<>(); | ||
|
|
||
| // TODO: We are setting this to request by default | ||
| dispatcherType = DispatcherType.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.
Is there anyway to change this value? If not, remove class variable and hard code response in getDispatcherType() method.
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.
Changed to static return. Cannot see where we'll use the other dispatcher types any time soon: http://docs.oracle.com/javaee/6/api/javax/servlet/DispatcherType.html
| */ | ||
| package com.amazonaws.serverless.proxy.internal.servlet; | ||
|
|
||
| import javax.servlet.*; |
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.
Don't use '*' imports.
| DispatcherType type = request.getDispatcherType(); | ||
|
|
||
| if (filtersSize == -1) { | ||
| getFilterHolders().size(); |
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.
What is this for?
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.
Good catch, bug. forgot to remove.
| if (!key.getClass().isAssignableFrom(TargetCacheKey.class)) { | ||
| return false; | ||
| } else { | ||
| return hashCode() == key.hashCode(); |
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.
This isn't valid (e.g. o1.targetPath could be null and o2.dispatcherType could be null, both resulting in a hashcode of -1).
|
|
||
| @Override | ||
| public void init(FilterConfig filterConfig) throws ServletException { | ||
| if (filterConfig.getInitParameter("invalid_status_code") != null) { |
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.
Make "invalid_status_code" a constant.
| try { | ||
| invalidStatusCode = Integer.parseInt(statusCode); | ||
| } catch (NumberFormatException e) { | ||
| invalidStatusCode = DEFAULT_ERROR_CODE; |
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.
Log?
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.
Need to find a better log strategy. Tired of just calling the lambdaContext.getLogger from places I can't reach it. I'll add this as an issue for the next release.
Merging support for servlet filters into master branch. This should address issue #15. For containers that support the servlet specs you can use a
StartupHandlerto register filters in the servlet context:The container does NOT load filter implementations annotated with
@WebFilterautomatically, they still have to be registered in theonStartupmethod. This is to avoid impacting cold starts.