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

[msal-node] Cache Lookup - 1: Logical keys for cache entities #1624

Merged
merged 34 commits into from Jun 4, 2020

Conversation

sameerag
Copy link
Member

This PR generates AccountId and CredentialId as a part of the CredentialEntity and AccountEntity which will be later used in looking up cache elements as needed.

Context :

Schema

Account and Credential cache has the below schema for keys:

Entity type Key components
Account <home_account_id>-< environment>-<realm*>
Credential <home_account_id*>-< environment>-<credential_type>-<client_id>-<realm*>-<target*>

But to tie up a Credential with Account, the keys are logically divided as follows:

Logical key components

Field Format Applicable to
account_id <home_account_id>-< environment> Account
Credential
realm < realm> Account
credential_id <credential_type>-<client_id>-< realm> Credential
target < target> Credential

@sameerag sameerag changed the title [msal-node] Move credential key generation to a common place [msal-node] Logical keys for cache entities May 11, 2020
@sameerag sameerag changed the title [msal-node] Logical keys for cache entities [msal-node] Cache Lookup - 1: Logical keys for cache entities May 11, 2020
Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

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

:shipit:

* Generate Account Id key component as per the schema: <home_account_id>-<environment>
*/
generateRealm(): string {
return (this.realm || "").toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think empty space is used in multiple places, consider adding it to the constants file

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for EMPTY_STRING constant

This was referenced May 20, 2020
@sameerag sameerag changed the base branch from msal-node-cache-fileops-async to dev May 27, 2020 02:09
/**
* Generate Account Id key component as per the schema: <home_account_id>-<environment>
*/
generateAccountId(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be used as utils for cache lookup (i.e. generate keys for looking up a specific cred)?

Copy link
Member Author

Choose a reason for hiding this comment

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

So generating accountId for an account object instance should be separate from generating it from a set of values. I will try coming up with common code as needed, for now I added static helpers in CacheHelper.ts in a later PR to address this. Point noted though.

Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

Couple questions but otherwise looks ok

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Jun 3, 2020
@coveralls
Copy link

coveralls commented Jun 3, 2020

Coverage Status

Coverage decreased (-1.06%) to 77.35% when pulling 4a4dc16 on msal-node-cache-keys into 68fa685 on dev.

@sameerag sameerag removed the msal-browser Related to msal-browser package label Jun 3, 2020
@github-actions github-actions bot added the msal-browser Related to msal-browser package label Jun 3, 2020
@github-actions github-actions bot added the msal-node Related to msal-node package label Jun 4, 2020
@sameerag sameerag merged commit 2ca4cd4 into dev Jun 4, 2020
@sameerag sameerag deleted the msal-node-cache-keys branch June 8, 2020 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants