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

Guard and cache fetchPermission chrome API. #975

Merged

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Sep 17, 2020

jira: https://issues.redhat.com/browse/RHCLOUD-9200

Changes

  • adds a guard for fetchPermission chrome API call
    • invoking fetchPermission x > 1 times will always result only in one API call.
  • use maximum page size for the fetchPermission config
    • previously the page size was 25, now it's 100
    • this change alone reduces the number of calls 4x
  • cache the fetchPermission call
    • this will remove many rbac API calls when switching between apps
    • the cache is invalidated after 10 minutes

@Hyperkid123 Hyperkid123 added the enhancement New feature or request label Sep 17, 2020
@Hyperkid123 Hyperkid123 requested review from iphands and a team September 17, 2020 09:49
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2020

Codecov Report

Merging #975 into master will increase coverage by 0.28%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #975      +/-   ##
==========================================
+ Coverage   52.59%   52.88%   +0.28%     
==========================================
  Files          57       57              
  Lines        1061     1076      +15     
  Branches      206      209       +3     
==========================================
+ Hits          558      569      +11     
- Misses        398      402       +4     
  Partials      105      105              
Impacted Files Coverage Δ
src/js/entry.js 23.94% <0.00%> (-1.06%) ⬇️
src/js/rbac/fetchPermissions.js 74.07% <93.75%> (+14.07%) ⬆️


export const createFetchPermissionsWatcher = (getUser, libjwt) => {
let currentCall = undefined;
const cache = new CacheAdapter(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't create new cache, I'd rather share chrome cache from all over places, meaning passing it to createFetchPermissionsWatcher

const createFetchPermissionsWatcher = (cache) => {
  let currentCall = undefined;
    return async (userToken, app = '') => {
        if (insights.chrome.getBundle() === 'openshift') {
            return Promise.resolve([]);
        }
        const permissions = await cache.getItem('permissions');
        if (permissions) {
            return permissions;
        }
        if (typeof currentCall === 'undefined') {
            currentCall = getUser().then(() => fetchPermissions(userToken, app).then((data) => {
                currentCall = undefined;
                cache.setItem('permissions', data);
                return data;
            }));
        }
        return currentCall;
    };
}

src/js/entry.js Outdated
libjwt.jwt.logoutAllTabs();
});
};
const fetchPermissions = createFetchPermissionsWatcher(getUser, libjwt);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this to consume only cahce

const fetchPermissions = getUser().then(() => createFetchPermissionsWatcher(cache));

Copy link
Contributor

Choose a reason for hiding this comment

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

In order to make it work correctly you'd have to moce let chromeCache on line 56 move above chrome bootstrap.

@Hyperkid123 Hyperkid123 force-pushed the serialize-permission-calls branch 2 times, most recently from d2b61c9 to 53409b2 Compare September 17, 2020 12:34
karelhala
karelhala previously approved these changes Sep 17, 2020
karelhala
karelhala previously approved these changes Sep 17, 2020
@Hyperkid123
Copy link
Contributor Author

@karelhala I have forgotten to update the tests.

@Hyperkid123
Copy link
Contributor Author

@karelhala also using the global cache does not work 😢 The endpoint is called before the cache is created... Let's use the local cache for now and I will come up with a solution to make the chrome cache global in a different PR.
This is the error. The cache is, unfortunately undefined.

fetchPermissions.js:69 Uncaught (in promise) TypeError: Cannot read property 'getItem' of undefined
    at _callee$ (fetchPermissions.js:69)
    at tryCatch (runtime.js:63)
    at Generator.invoke [as _invoke] (runtime.js:293)
    at Generator.eval [as next] (runtime.js:118)
    at asyncGeneratorStep (asyncToGenerator.js:3)
    at _next (asyncToGenerator.js:25)
    at eval (asyncToGenerator.js:32)
    at new Promise (<anonymous>)
    at eval (asyncToGenerator.js:21)
    at eval (fetchPermissions.js:101)

@Hyperkid123 Hyperkid123 requested a review from a team September 21, 2020 15:34
@karelhala karelhala merged commit 6a5f3f0 into RedHatInsights:master Sep 22, 2020
@Hyperkid123 Hyperkid123 deleted the serialize-permission-calls branch September 22, 2020 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants