Allowed resolution validation

[andybab]

Support for validating against a set of allowed resolutions configured via environment variable.

[jpignata]

Thanks for opening this!

When I built this, I wanted to make it a minimal example for folks to play with, learn from, and possibly fork and build off of, so I intentionally eschewed features required to make it production ready like constraining dimensions.

That said, I'm not opposed to adding this, and I'd like it to be optional. Your implementation seems to check that box.

• How does one set this in an environment variable?
• Is this checking against the whole heightxwidth string or just the width?

[andybab]

Hi,

The allowed resolutions are set using an env. variable (similar to BUCKET and URL). The variable is ALLOWED_RESOLUTIONS. One can define them in this format:

• 270x150,370x200,842x511 or 270x150, 370x200 ,842x511 or 270x150 , 370x200 , 842x511
  If no resolutions are defined, ALLOWED_RESOLUTIONS will become an empty Set and the implementation then behaves the same as yours, it will resize to any resolution the user requests.

The check is performed against the whole heightxwidth string between /(...)/ in key.match. I have just added another capturing group to your regex. The result will be available in match[1]. Other capturing groups remained but their index in match needed to be incremented.

[jpignata]

LGTM. A couple of things before we merge:

• I added some suggestions for easier readability for newbies. I'd like to err on the side of being verbose to make it easier to read as this is meant as a reference.
• Please add documentation of this feature in the README.

Thank you so much for your contribution! Looks great.

[jpignata]

Could this be phrased in the affirmative for easier parsing? I'd suggest something like:

if(ALLOWED_DIMENSIONS.size > 0 && ALLOWED_DIMENSIONS.has(dimensions)) {
  // Do the thing.
}

I'd also like to see the matches set to locals for easier parsing up top, so:

const dimensions = match[1];

[andybab]

No problem with this one.

[jpignata]

How about killing the ternary which could confuse newbies? Maybe something like:

const ALLOWED_DIMENSIONS = new Set();

if (process.env.ALLOWED_DIMENSIONS) {
  const dimensions = process.env.ALLOWED_DIMENSIONS.split(/\s?,\s?/);
  dimensions.forEach((dimension) => ALLOWED_DIMENSIONS.add(dimension));
}

A little too explicit, maybe.

[andybab]

Sure no problem, you can remove the ternary.

[jpignata]

Oh, and if you could move these up. I'm fine doing these assignments even if we bail. I think it reads better. Basically:

const key = event.queryStringParameters.key;
const match = key.match(/((\d+)x(\d+))\/(.*)/);
const dimensions = match[1];
const width = parseInt(match[2], 10);
const height = parseInt(match[3], 10);
const originalKey = match[4];

[andybab]

Sure no problem

[andybab]

Looks good

[andybab]

Sure no problem

[andybab]

I updated the README file and (hopefully) reviewed your changes.

[andybab]

I applied your recommendations. Also changed the README file to reflect the changed env. variable name. Please review. Thx.

[jpignata]

Thanks for contributing! ✨👊🎉