Or conditions#1481
Merged
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds configurable authentication method support (basic or JWT) to the ArangoDB driver connection setup and bumps the library version. Sequence diagram for ArangoDB connection with configurable authentication methodsequenceDiagram
participant Client
participant ArangoDBDriver
participant ArangoClient
participant SysDB
Client->>ArangoDBDriver: __init__(params)
ArangoDBDriver->>ArangoDBDriver: set _username from params or default
ArangoDBDriver->>ArangoDBDriver: set _password from params or default
ArangoDBDriver->>ArangoDBDriver: set _auth_method from params or basic
Client->>ArangoDBDriver: connection(database)
ArangoDBDriver->>ArangoDBDriver: build url from _protocol, _host, _port
ArangoDBDriver->>ArangoClient: ArangoClient(hosts=url)
ArangoClient-->>ArangoDBDriver: client instance
alt auth_method is jwt
ArangoDBDriver->>ArangoDBDriver: token = params.jwt_token or _password
ArangoDBDriver->>ArangoClient: db(_system, auth_method=jwt, password=token)
ArangoClient-->>SysDB: create sys_db with jwt
else auth_method is basic
ArangoDBDriver->>ArangoClient: db(_system, username=_username, password=_password)
ArangoClient-->>SysDB: create sys_db with basic
end
ArangoDBDriver->>SysDB: databases()
SysDB-->>ArangoDBDriver: list of databases
ArangoDBDriver->>SysDB: create or select target database as needed
Class diagram for ArangoDB driver with basic and JWT authentication supportclassDiagram
class ArangoDBDriver {
+dict params
-str _username
-str _password
-str _auth_method
-str _protocol
-str _host
-int _port
-str _database_name
-ArangoClient _client
__init__(params)
connection(database)
}
class ArangoClient {
ArangoClient(hosts)
db(database_name, username, password)
db(database_name, auth_method, password)
}
class SysDB {
databases()
}
ArangoDBDriver --> ArangoClient : uses
ArangoClient --> SysDB : returns
%% Authentication configuration in constructor
ArangoDBDriver : _auth_method set from params.auth_method or basic
%% Connection behavior
ArangoDBDriver : connection uses _auth_method to choose jwt or basic
ArangoDBDriver : for jwt - token from params.jwt_token or _password
ArangoDBDriver : for basic - uses _username and _password
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the
__init__exception path you reset_usernameand_passwordbut never initialize_auth_method, which can lead to attribute errors later; consider setting_auth_methodto a sensible default there as well. - In
connection, onlyjwtandbasicvalues for_auth_methodare handled; consider validatingself._auth_methodand raising a clear error or falling back to a default for unsupported values instead of silently doing nothing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `__init__` exception path you reset `_username` and `_password` but never initialize `_auth_method`, which can lead to attribute errors later; consider setting `_auth_method` to a sensible default there as well.
- In `connection`, only `jwt` and `basic` values for `_auth_method` are handled; consider validating `self._auth_method` and raising a clear error or falling back to a default for unsupported values instead of silently doing nothing.
## Individual Comments
### Comment 1
<location> `asyncdb/drivers/arangodb.py:112-119` </location>
<code_context>
+ hosts=url
)
+ # Connect to system database first
+ if self._auth_method == "jwt":
+ token = self.params.get('jwt_token', self._password)
+ sys_db = self._client.db(
+ '_system',
+ auth_method="jwt",
+ password=token
+ )
+ elif self._auth_method == "basic":
+ sys_db = self._client.db(
+ '_system',
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle unsupported `auth_method` values to avoid `sys_db` being undefined.
If `self._auth_method` is anything other than `"jwt"` or `"basic"`, `sys_db` remains undefined and `sys_db.databases()` will raise `UnboundLocalError`. Consider either defaulting to a supported method or explicitly raising an error for unsupported auth methods.
</issue_to_address>
### Comment 2
<location> `asyncdb/drivers/arangodb.py:111-114` </location>
<code_context>
)
+ # Connect to system database first
+ if self._auth_method == "jwt":
+ token = self.params.get('jwt_token', self._password)
+ sys_db = self._client.db(
+ '_system',
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid defaulting the JWT token to the password value.
Reusing `self._password` as the default `jwt_token` risks confusing/failed auth when someone switches to JWT without explicitly setting a token, and mixes two distinct credential types. Prefer requiring an explicit `jwt_token` (and failing fast if it’s missing) rather than silently falling back to the password.
```suggestion
# Connect to system database first
if self._auth_method == "jwt":
token = self.params.get('jwt_token')
if not token:
raise ValueError("jwt_token parameter is required when auth_method is 'jwt'")
sys_db = self._client.db(
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+112
to
+119
| if self._auth_method == "jwt": | ||
| token = self.params.get('jwt_token', self._password) | ||
| sys_db = self._client.db( | ||
| '_system', | ||
| auth_method="jwt", | ||
| password=token | ||
| ) | ||
| elif self._auth_method == "basic": |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Handle unsupported auth_method values to avoid sys_db being undefined.
If self._auth_method is anything other than "jwt" or "basic", sys_db remains undefined and sys_db.databases() will raise UnboundLocalError. Consider either defaulting to a supported method or explicitly raising an error for unsupported auth methods.
Comment on lines
+111
to
+114
| # Connect to system database first | ||
| if self._auth_method == "jwt": | ||
| token = self.params.get('jwt_token', self._password) | ||
| sys_db = self._client.db( |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Avoid defaulting the JWT token to the password value.
Reusing self._password as the default jwt_token risks confusing/failed auth when someone switches to JWT without explicitly setting a token, and mixes two distinct credential types. Prefer requiring an explicit jwt_token (and failing fast if it’s missing) rather than silently falling back to the password.
Suggested change
| # Connect to system database first | |
| if self._auth_method == "jwt": | |
| token = self.params.get('jwt_token', self._password) | |
| sys_db = self._client.db( | |
| # Connect to system database first | |
| if self._auth_method == "jwt": | |
| token = self.params.get('jwt_token') | |
| if not token: | |
| raise ValueError("jwt_token parameter is required when auth_method is 'jwt'") | |
| sys_db = self._client.db( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add configurable authentication method for the ArangoDB driver and bump the library version.
New Features:
Enhancements: