Skip to content
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

Rewrite doesn't seem to be working #121

Closed
vlidholt opened this issue Jul 17, 2019 · 8 comments
Closed

Rewrite doesn't seem to be working #121

vlidholt opened this issue Jul 17, 2019 · 8 comments

Comments

@vlidholt
Copy link

I've tried every single combination I can think of to get the rewrite rules to work, but not succeeded. There seem to be no examples whatsoever out there.

Also, the REWRITE_MATCH_PATTERN / REWRITE_SUBSTITUTION fields seem to suggest it's only possible to make one rewrite rule.

Is it possible to get an example of how to write a rule, and is it possible to have more than one rule?

@mandys
Copy link

mandys commented Jul 23, 2019

We faced a similar issue @vlidholt as we were using many regex rules to get our old urls working ( in the python based deployment ).

As you can see in their source,
https://github.com/awslabs/serverless-image-handler/blob/master/source/image-handler/thumbor-mapping.js

parseCustomPath(path) {
    // Setup from the environment variables
    const matchPattern = process.env.REWRITE_MATCH_PATTERN;
    const substitution = process.env.REWRITE_SUBSTITUTION;
    // Perform the substitution and return
    if (path !== undefined && matchPattern !== undefined && substitution !== undefined) {
        const parsedPath = path.replace(matchPattern, substitution);
        const output = { path : parsedPath };
        return output;
    } else {
        throw new Error('ThumborMapping::ParseCustomPath::ParsingError');
    }
}

The rule is always going to be treated like a string.
It's more like replace A with B. Complex regex rules replacement doesn't seem to be built.

We had to modify it and redeploy.
[ partial code below ]

We did it like the python based solution.
Rather than having pattern and substitution in 2 different options, we are passing regex and replacement in the same array.

eg:
['([0-9.v_]+)_(thumb).(gif|jpg|jpeg|png)', 'fit-in/132x100/filters:quality(90):fill(fff)/$1_source.$3'],

const matchPatternStr = process.env.REWRITE_MATCH_PATTERN;
let matchPattern = eval('[' + matchPatternStr + ']');
let parsedPath;

        for (let i = 0; i < matchPattern.length; i++) {
            let matchPatternRegex = matchPattern[i][0];
            let re = new RegExp(matchPatternRegex, 'gi');	
            parsedPath = path.replace(re, matchPattern[i][1]);

I know that aws needs to fix this, but till then hope this helps!

@hayesry
Copy link
Member

hayesry commented Aug 4, 2019

Hey @mandys - Thanks for bringing this to our attention and providing a code snippet that's been working for you so far. Apologies you had to go in and modify then re-deploy to get everything to work. I've added a bug/issue ticket for this but if you'd like to take the code that's been working for you and wrap it into a PR, we'd be happy to review and merge it into our next minor release!

@rybakit
Copy link

rybakit commented Dec 6, 2019

Yet another simple workaround:

- const parsedPath = path.replace(matchPattern, substitution);
+ const parsedPath = path.replace(new RegExp(matchPattern), substitution);

Note, that you have to normalize the matchPattern value by omitting leading and trailing /.

@beomseoklee
Copy link
Member

We have updated our solution, and I believe your issue has been fixed. If you still see the issue with the latest version (v4.2), please feel free to reopen the issue.

You can refer to the recent changes here

@neilsh
Copy link

neilsh commented Feb 7, 2020

@beomseoklee are you sure version 4.2 addresses this issue? I checked through the diff for version 4.2 and I can't find any changes related to the path rewrite functionality: 19cbc3c

@beomseoklee
Copy link
Member

@neilsh I think it was my mistake to group the open issues.
We will take a look at this issue again. I'm sorry for the confusion.

@pasali
Copy link

pasali commented Nov 18, 2020

Hi @beomseoklee, any news on this issue ?

@beomseoklee
Copy link
Member

@pasali @neilsh @vlidhot Thanks for waiting. We've release v5.1.0, and hopefully the new version fixes this issue.
Basically, it simply replaces the string. If it does not work, please open a new issue with your use case so that we can take a look at your use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants