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

Adding ExportReplace to support the Context object [breaking change for 1.0] #378

Merged
merged 11 commits into from
Feb 28, 2023

Conversation

element-of-surprise
Copy link
Contributor

This PR adds support for the Context object in caches that are implementing a cache.

We cannot update ExportReplace without breaking implementors, so we are introducing an ExportReplaceCtx as a superset.

Calls that use methods from ExportReplace will now check for the existence of ExportReplaceCtx and use those superset methods instead if available.

@element-of-surprise element-of-surprise added the enhancement New feature or request label Jan 30, 2023
@element-of-surprise element-of-surprise self-assigned this Jan 30, 2023
@chlowell chlowell changed the base branch from main to dev February 23, 2023 00:19
@chlowell
Copy link
Collaborator

I added some changes to help get this to the finish line:

  • retarget to dev branch
  • add Context parameters and error returns to existing methods--breaking compatibility--instead of adding new methods (closes Replace any *Ctx methods with the original method names #379)
  • methods accessing the cache fail on cache I/O errors and propagate those errors
    • I changed some functions to use named return params so they can return errors from deferred functions. I'm not fond of that syntax, feel free to suggest better ideas

@@ -740,12 +739,11 @@ func (cca Client) AcquireTokenOnBehalfOf(ctx context.Context, userAssertion stri
}

// Account gets the account in the token cache with the specified homeAccountID.
func (cca Client) Account(homeAccountID string) Account {
return cca.base.Account(homeAccountID)
func (cca Client) Account(ctx context.Context, homeAccountID string) (Account, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider naming the id accountId instead of homeAccountId. The concept of "home" implies tenancy, which is only in AAD. ADFS doesn't have this. And as we expand to other IdPs, they likely won't have this either.

@element-of-surprise element-of-surprise changed the title Adding ExportReplaceCtx to support the Context object Adding ExportReplace to support the Context object [breaking change for 1.0] Feb 23, 2023
@chlowell chlowell linked an issue Feb 24, 2023 that may be closed by this pull request
@chlowell chlowell added the GA label Feb 24, 2023
apps/internal/base/base.go Outdated Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
apps/internal/base/base.go Outdated Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
apps/internal/base/base.go Show resolved Hide resolved
Copy link
Contributor Author

@element-of-surprise element-of-surprise left a comment

Choose a reason for hiding this comment

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

As discussed, clothe the returns and ignore my comment on the named returns, I apparently was too tired to be doing reviews, I now see the defer with assignments.

This is approved, however since I created this thing and you are finishing it, I can't approve it.

@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@chlowell chlowell merged commit ec469b9 into dev Feb 28, 2023
@chlowell chlowell deleted the doak/context branch February 28, 2023 22:49
@rayluo rayluo mentioned this pull request Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request GA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace any *Ctx methods with the original method names
3 participants