-
Notifications
You must be signed in to change notification settings - Fork 560
support for variables in 'param-value' from properties file without depending on any 3rd party libs #106
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
base: master
Are you sure you want to change the base?
Conversation
Enhancement - Properties file support for web.xml
README.md
Outdated
|
||
* It's simple -- a single source file implementation | ||
* It's tested -- have confidence it works [](https://travis-ci.org/mitre/HTTP-Proxy-Servlet) | ||
* It's tested -- have confidence it works [](https://travis-ci.org/madhbhavikar/HTTP-Proxy-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.
Why this change?
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 was local change, for my company, will revert. and repush
<connection>scm:git:https://[email protected]/dsmiley/HTTP-Proxy-Servlet.git</connection> | ||
<developerConnection>scm:git:[email protected]:dsmiley/HTTP-Proxy-Servlet.git</developerConnection> | ||
<tag>HEAD</tag> | ||
<tag>smiley-http-proxy-servlet-1.9</tag> |
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.
hmm; I'm not sure what the implication of this change is... but I see it's something to maintain between versions :-/
pom.xml
Outdated
<!-- I used to work for MITRE for many years but I don't anymore. --> | ||
<!--<organization>MITRE</organization>--> | ||
</developer> | ||
<developer> |
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 think there's a <role>contributor</role>
or something like that which is supported. Is the presence of your name here a big deal here for you? I can think of a contributor or two that contributed substantially more than this PR (which doesn't mean I'm not grateful for all contributions!). Maybe me finally creating a CHANGES.txt would be a good thing to appease the desire to get credit beyond git history. It'd list names.
|
||
/** | ||
* Reads a configuration parameter. By default it reads servlet init parameters but | ||
* it can be overridden. |
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 think these javadocs are now missing info about the ${}
syntax you added and properties files.
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.
Java docs added to the method
* if defined for this proxy URL. | ||
*/ | ||
protected static final Pattern TEMPLATE_PATTERN = Pattern.compile("\\{([a-zA-Z0-9_%.]+)\\}"); | ||
protected static final Pattern TEMPLATE_PATTERN = Pattern.compile("\\{([a-zA-Z0-9_%-.]+)\\}"); |
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 just looking at the RFC spec. It turns out most characters are supported except a small number. To keep this simple excepting all the characters we should (yet some we shouldn't admittedly), lets simplify this: Pattern.compile("\\{(.+?)\\}")
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.
pattern has been corrected
params.put(pair.getName(), pair.getValue()); | ||
} | ||
|
||
LinkedHashMap<String, String> specialHeaders = getVariablesFromRequestHeaders(servletRequest); |
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.
Why are the headers pulled in advanced to a map here? Instead, why not simply resolve via request.getHeader(headerName)
?
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.
Implemented
All changes have been incorporated, let me know if any changes are required. |
Ok; perhaps this weekend I'll take a look. I'm ridiculously behind in my day job work. |
That would be Great!, My company needs the official fix ;) |
The only thing missing is a test. |
Sorry to have neglected this. If you're still around and address what's missing, I can give it another look. Seems I last asked for a test. At least some test; needn't be comprehensive. |
Support for properties file
Fix for possible memory leak in formatter
Support for properties file
Documentation updated
version bumped to 1.9.1
Reference : Previous pull request