Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
(feat) [NAN-677] enforce cap when calling nangoauth #1950
(feat) [NAN-677] enforce cap when calling nangoauth #1950
Changes from all commits
bd7c56c
c78711e
f6e4d07
d873fc0
2253964
1118e38
db1a39e
1bfda39
4fd9580
92b3ea7
5d694fd
b95ea12
3c50b11
8656bc9
44eabf7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are making more queries to the db on each request by adding a middleware which is doing something similar to the
access.middleware
, ie: fetching the account and environment from the db. Can we fetch theaccount.is_capped
as part of theaccess.middleware
and save it into theres.locals
like we do withaccountId
andenvironmentId
so it can be access down the line by controllers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea in theory, but the issue is that the value of
is_capped
can change via the api and then the value stored in theres.locals
is stale and potentially inaccurate. Since the user changing theis_capped
setting is different than the user who has the capping nor not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.locals
is only stored for the current request if that's your worryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, but still this is making it more complicated than it needs since right now the only controller it goes through is the
connection.controller
when it is imported. If it went into theaccess.middleware
instead of thecapping.middleware
then the logic to check the cap would need to go through:oauth.controller
unauth.controller
apiAuth.controller
appStoreController
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a new function? Can we not call
syncService.getSyncsByProviderConfigKey
and count the connections?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That method would be good, but we need both to check against syncs and actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending from Bastien.