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 client caching #54

Merged
merged 4 commits into from
Jun 20, 2024
Merged

fix client caching #54

merged 4 commits into from
Jun 20, 2024

Conversation

RalfHammer
Copy link
Contributor

@RalfHammer RalfHammer commented Jun 19, 2024

Client caches were based on organizationClient and spaceClient structures that also contain values like organizationName and spaceGuid. However, this was not reflected in the key for the caching.
This causes re-use of clients that do not interact with the desired organization or space.

This PR will just cache the actual CF client and some metadata like url, username and password.
However, the structures organizationClient and spaceClient will always be created with updated values for organizationName resp spaceGuid.

Two tests are added as well to test two different organizations and two different spaces.

bKiralyWdf
bKiralyWdf previously approved these changes Jun 19, 2024
Copy link
Contributor

@bKiralyWdf bKiralyWdf left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Comment on lines +108 to +129
// look up CF client in cache
identifier := clientIdentifier{url: url, username: username}
cacheEntry, isInCache := clientCache[identifier]

var err error = nil
var client *organizationClient = nil
if isInCache {
// re-use CF client and wrap it as organizationClient
client = &organizationClient{organizationName: organizationName, client: cacheEntry.client}
if cacheEntry.password != password {
// password was rotated => delete client from cache and create a new one below
delete(clientCache, identifier)
isInCache = false
}
}

if !isInCache {
// create new CF client and wrap it as organizationClient
client, err = newOrganizationClient(organizationName, url, username, password)
if err == nil {
orgClientCache[identifier] = client
}
} else {
// If the password has changed since we cached the client, we want to update it to the new one
if client.password != password {
client.password = password
// add CF client to cache
clientCache[identifier] = &clientCacheEntry{url: url, username: username, password: password, client: client.client}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a little bit of an odd way of doing it, but it should work.

Comment on lines +140 to +161
// look up CF client in cache
identifier := clientIdentifier{url: url, username: username}
cacheEntry, isInCache := clientCache[identifier]

var err error = nil
var client *spaceClient = nil
if isInCache {
// re-use CF client from cache and wrap it as spaceClient
client = &spaceClient{spaceGuid: spaceGuid, client: cacheEntry.client}
if cacheEntry.password != password {
// password was rotated => delete client from cache and create a new one below
delete(clientCache, identifier)
isInCache = false
}
}

if !isInCache {
// create new CF client and wrap it as spaceClient
client, err = newSpaceClient(spaceGuid, url, username, password)
if err == nil {
spaceClientCache[identifier] = client
}
} else {
// If the password has changed since we cached the client, we want to update it to the new one
if client.password != password {
client.password = password
// add CF client to cache
clientCache[identifier] = &clientCacheEntry{url: url, username: username, password: password, client: client.client}
Copy link
Contributor

Choose a reason for hiding this comment

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

For now I think we should merge this, but later think about factoring out this part.

@RalfHammer RalfHammer marked this pull request as ready for review June 20, 2024 10:22
@TheBigElmo TheBigElmo merged commit ccfa83d into main Jun 20, 2024
6 checks passed
@TheBigElmo TheBigElmo deleted the fix-cf-client-cache branch June 20, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants