Skip to content

Conversation

joyjwang
Copy link
Contributor

@joyjwang joyjwang commented Sep 3, 2024

GODRIVER-2529

Summary

Return error if a ClientEncryption is used after Close

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the review-priority-low Low Priority PR for Review: within 3 business days label Sep 3, 2024
Copy link
Contributor

API Change Report

No changes found!

Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

Looking at this closely, we should probably use the underlying keyVaultClient to determine if a client has been disconnected or not. mongo.ClientEncryption appears to just wrap mongo.Client. So instead of adding a closed bool, we can check if a connection has ended by seeing if the pointer to the pool has been nullified. For example:

func (ce *ClientEncryption) AddKeyAltName(ctx context.Context, id bson.Binary, keyAltName string) *SingleResult {
	if ce.keyVaultClient.sessionPool == nil {
		return &SingleResult{err: ErrClientDisconnected}
	}
}

Additionally, we only want to apply this to methods that actually use either keyValueClient or keyVaultColl (which is constructed from the client).

You can update the unit tests in 1 of 2 ways:

  1. Keep a purely mocked solution:
ce := &ClientEncryption{keyVaultClient: &Client{}}
  1. Actually connect to a client and then disconnect (it doesn't even matter if a server is live):
client, _ := Connect(options.Client().ApplyURI("mongodb://invalid"))
_ = client.Disconnect(context.Background())

ce := &ClientEncryption{keyVaultClient: client}

// [...]

The latter is probably better coverage.

prestonvasquez
prestonvasquez previously approved these changes Sep 5, 2024
Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

LGTM w/ one small suggestion!

qingyang-hu
qingyang-hu previously approved these changes Sep 5, 2024
Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

Great job! 🌟

@prestonvasquez
Copy link
Member

@joyjwang The static analysis test is failing. Please run the check-fmt make target and push the results:

make check-fmt

The race detector failure is a known issue.

@joyjwang joyjwang merged commit a4a5bc3 into mongodb:master Sep 6, 2024
28 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-priority-low Low Priority PR for Review: within 3 business days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants