feat(rewrite-pattern): rewrite pattern substitution support with unit tests #423
Conversation
| * @param str Input string to evaluate | ||
| * @returns Either a json object or a plain string | ||
| */ | ||
| export function parseJson(str) { |
There was a problem hiding this comment.
The name of this function is a little misleading, I would change it to "getParamsFromString" or something. It being in a generic utils/helpers file puts it at real risk of being reused by someone.
There was a problem hiding this comment.
I am ok with your suggestion. Maybe we can say "convertPatterns"
| } else { | ||
| path = path.replace(REWRITE_MATCH_PATTERN, REWRITE_SUBSTITUTION); | ||
| const REWRITE_MATCH_PATTERNS = parseJson(REWRITE_MATCH_PATTERN); | ||
| const REWRITE_SUBSTITUTIONS = parseJson(REWRITE_SUBSTITUTION); |
There was a problem hiding this comment.
Do we ever check that these are the same length?
| * @param matchPattern string to operate on | ||
| * @returns regex | ||
| */ | ||
| export function generateRegExp(matchPattern) { |
There was a problem hiding this comment.
Can we add some tests for invalid regexes? Invalid flags?
There was a problem hiding this comment.
Your comment is important but I think it is not relevant to this pull request. Only the location of the code has changed, the logic has not changed.
| path = path.replace(regExp, REWRITE_SUBSTITUTIONS[k]); | ||
| if (generateRegExp(matchPattern).test(path)) break; | ||
| } else { | ||
| path = path.replace(matchPattern, REWRITE_SUBSTITUTIONS[k]); |
There was a problem hiding this comment.
If matchPattern isn't a string, what is it? Should we check? I don't see any relevant tests
There was a problem hiding this comment.
Hi, this PR converts "REWRITE_MATCH_PATTERNS" variable into an array and loop each item as "matchPattern" variable. There is no change in the logic.
Change is like below
- if (typeof REWRITE_MATCH_PATTERN === "string") {
+ if (typeof matchPattern === "string") {If a test is going to be written on this subject, a different pull request may be requested.
| const patternStrings = matchPattern.split("/"); | ||
| const flags = patternStrings.pop(); | ||
| const parsedPatternString = matchPattern.slice(1, matchPattern.length - 1 - flags.length); | ||
| return new RegExp(parsedPatternString, flags); |
There was a problem hiding this comment.
I realize this feature is already out there, but do we really need regex to solve this problem?
It looks like these values are controlled in the lambda env, but are we worried about customers redos-ing themselves?
There was a problem hiding this comment.
Your comment is important but I think it is not relevant to this pull request. Only the location of the code has changed, the logic has not changed.
|
This pr has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
|
Hi @dougtoppin, I received an email regarding the closure of this pull request. Are you not considering implementing this feature? I have checked the Implementation Guide, and it mentions that the instructions for the rewrite feature have been clarified. However, it is still not possible to create more than one rewrite rule with the current code. In the old Thumbor version, it was possible to make multiple rewrites. This pull request makes it possible to do so. |
|
@tasarsu I deleted an old personal fork of mine where I had made some changes. |
Description of changes:
Add unit tests to rewrite pattern support from original PR #399
All changes, including original:
Serverless Image Handler provides support for remapping requests to reduce the effort required to migrate an existing image request model to the solution. This is done by configuring the REWRITE_MATCH_PATTERN and REWRITE_SUBSTITUTION regular expression environment variables for the backend Lambda. REWRITE_MATCH_PATTERN describes the incoming request that is to be changed. REWRITE_SUBSTITUTION contains the pattern to be mapped to.
The initial implementation of this facility supported a single rewrite/substitution pair.
This PR provides the ability to remap/substitute multiple patterns.
The env vars are set to a matching set of entries.
REWRITE_MATCH_PATTERN and REWRITE_SUBSTITUTION values can be written like JSON Arrays so multiple rewrite rules can be applied.
For example, for the following env var in the. backend Lambda, 3 request patterns can be specified which will be mapped to the rewrite substitution values
REWRITE_MATCH_PATTERN:
["//thumb/g","//small/g","//large/g"]
REWRITE_SUBSTITUTION:
["/300x300/filters:quality(80)","/fit-in/600x600/filters:quality(80)","/fit-in/1200x1200/filters:quality(80)"]
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.