Skip to content

Conversation

mangalaman93
Copy link
Member

@mangalaman93 mangalaman93 commented Mar 25, 2025

This PR cleans up the code for v25 APIs, unexports the SignInUser function because we have now ClientOption WithACLCreds. It moves all the code in files with suffix _v25.go as well as adds more tests for Open function.

@Copilot Copilot AI review requested due to automatic review settings March 25, 2025 10:35
@mangalaman93 mangalaman93 requested a review from a team as a code owner March 25, 2025 10:35
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the v25 client code, improves authentication and connection string parsing, and adds more tests to validate the new behavior while also removing deprecated code.

  • Refactored the client connection and authentication logic in client_v25.go and client.go.
  • Added new tests in client_v25_test.go to validate connection string parsing and SSL mode handling.
  • Removed deprecated dql.go and updated namespace and alter operations in ns_v25.go and alter_v25.go.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
client_v25_test.go Added test cases for connection string parsing and SSL modes.
client_v25.go Refactored authentication options, updated error messages, and added documentation.
ns_v25.go Added and refactored txnOptions and namespace functions.
alter_v25.go Updated function signature to accept proper lambda parameter.
client.go Cleaned up unused code and updated Alter logic without retrying JWT.
dql.go Removed deprecated code.

@mangalaman93 mangalaman93 enabled auto-merge (squash) March 25, 2025 10:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the v25 client code and introduces additional tests to exercise new connection string parsing and authentication behaviors. Key changes include refactoring and renaming of client option functions in client_v25.go, internal reorganization of connection and authentication logic, and removal of duplicate or legacy code (such as the Open function in client.go and the file dql.go).

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
client_v25_test.go Added tests to validate connection string parsing and SSL/authentication behavior.
client_v25.go Refactored client options, authentication logic (using signInUser), and constants.
ns_v25.go Updated namespace and transaction-related functions with generic retry mechanisms.
alter_v25.go Modified alter operations to leverage the updated retry login logic.
client.go Removed legacy Open function and related constants to consolidate connection logic.
dql.go Removed redundant DQL query functions now covered by updated v25 implementations.
Comments suppressed due to low confidence (3)

client_v25_test.go:31

  • [nitpick] The test uses 'dgraph://localhost:' which contains a colon but an empty port; please add a clarifying comment explaining that this case is acceptable and distinct from a missing port, to improve test clarity.
_, err = dgo.Open("dgraph://localhost:")

client_v25.go:237

  • [nitpick] Consider adding a brief comment clarifying that signInUser is an internal helper for user authentication, to better distinguish its purpose from the deprecated exported Login method.
if err := d.signInUser(ctx, co.username, co.password); err != nil {

client.go:29

  • [nitpick] Since the Open function has been removed from this file, update the surrounding documentation to avoid confusion about the supported connection string interface in the refactored code.
func Open(connStr string) (*Dgraph, error) {

@mangalaman93 mangalaman93 disabled auto-merge March 25, 2025 11:13
@mangalaman93 mangalaman93 merged commit bba7626 into main Mar 26, 2025
4 checks passed
@mangalaman93 mangalaman93 deleted the aman/refactor branch March 26, 2025 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants