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:allow to only override elevated limits but merging with original config #71

Merged

Conversation

pubalokta
Copy link

@pubalokta pubalokta commented May 30, 2024

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

Fixes an error we found when providing overrides.

Problem detected: If we only provided overrides for elevated_limits, with no override for base size and/or per_x, the final configuration would not include the size and per_x.

Example:

  • Base bucket config
buckets = {
   ip: {
      size: 10,
      per_second: 5,
      overrides: {
         'awesome-key': {
            elevated_limits: {
               size: 200,
               per_second: 200,
            }
         }
      }
   }
}

When the override is processed, the configuration of the limit for 'awesome-key' results in:

'awesome-key': {
    elevated_limits: {
       size: 200,
       per_second: 200,
    }
 }

This means that the base size and per_x for that key are undefined which causes an internal error.

Solution
The solution is merging the override with the base configuration. If the override includes elevated configuration but NO base configuration it gets merged, resulting in:

'awesome-key': {
    size: 10,
    per_second: 5,
    elevated_limits: {
       size: 200,
       per_second: 200,
    }
 }

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Auth0 Community post
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@pubalokta pubalokta requested a review from a team as a code owner May 30, 2024 12:44
test/utils.tests.js Outdated Show resolved Hide resolved
test/utils.tests.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Show resolved Hide resolved
@pubalokta pubalokta force-pushed the allow-override-elevated-merging-with-original-config branch from 00894c3 to a7c2514 Compare May 31, 2024 15:38
@pubalokta pubalokta merged commit 0aa08e1 into master May 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants