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

fix(plugin) fixes a wrong constant for the request size limit plugin #1416

Merged
merged 4 commits into from
Jul 20, 2016

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Jul 18, 2016

Summary

constant for mb was 100.000 instead of 1.000.000 causing request to be rejected falsy.

Full changelog

  • fix wrong constant in request-size-limit plugin

Issues resolved

Fixes #1415


local RequestSizeLimitingHandler = BasePlugin:extend()

RequestSizeLimitingHandler.PRIORITY = 950

local function check_size(length, allowed_size, headers)
local allowed_bytes_size = allowed_size * 100000
local allowed_bytes_size = allowed_size * 1000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it much more convenient and requiring less cognitive load (as well as much less error prone) to declare constants representing size (including MBs) like so:

local CONST_NAME = X * 10^6 -- here X being 1

Then use CONST_NAME in the module's function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, including a linter fix to make ci run

local stringy = require "stringy"
local strip = require("pl.stringx").strip

local MEGABYTE = 10^6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 2^20 btw

@thibaultcha thibaultcha merged commit 74f642c into next Jul 20, 2016
@thibaultcha thibaultcha deleted the fix/request-size branch July 20, 2016 00:09
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

Successfully merging this pull request may close these issues.

2 participants