feat(ArangoDB): Add basic support for ArangoDB#202
Conversation
MattiasMTS
left a comment
MattiasMTS
left a comment
There was a problem hiding this comment.
Thanks for your contribution @enchantednatures! 😄 ✨ much appreciated, I learned a new database today 😋
Left some early comments on the adapter code - I'll take a second look on the integration tests. Very nice and appreciated the addition of integration test in the PR 👍
|
|
||
| type Arango struct{} | ||
|
|
||
| func (p *Arango) Connect(rawUrl string) (core.Driver, error) { |
There was a problem hiding this comment.
I know too little about arangodb, but to me it feels like there is a lot happening in this method based on certain conditions that might be unknown to the user (i.e. implicit behaviour). Do you think we want to make these things a bit more explicit based on the rawUrl? i.e. choose beteween http, http2 and jwt based on ?connection_type='input' or something similar? 😄
| if u.User != nil { | ||
| // Basic Authentication | ||
| username := u.User.Username() | ||
| password, _ := u.User.Password() |
There was a problem hiding this comment.
will arangodb return error or panic if the password is empty? I'd say just add e.g.
password, ok = u.User.Password()
if !ok {
return nil, fmt.Errorf("arango: password not set for user: %w", err)
}There was a problem hiding this comment.
It is possible, albeit not recommended, to run Arango and not require a root password. I will add a test case to cover this scenario.
| @@ -0,0 +1,64 @@ | |||
| package adapters | |||
There was a problem hiding this comment.
nit: should we call the files arangodb.go and arangodb_driver.go given this is the name of the driver?
| return "", nil, errors.New("arangoDriver is not initialized") | ||
| } | ||
|
|
||
| log.Print("Fetching list of databases") |
There was a problem hiding this comment.
I appreciate logging are nice but for now we don't have a good logging file structure yet - planning to add this. Ergo, could you remove the logging statements used in this file? Thanks ✨
| } | ||
|
|
||
| func (a *arangoDriver) ListDatabases() (current string, available []string, err error) { | ||
| if a == nil || a.c == nil { |
There was a problem hiding this comment.
this shouldn't be needed to check due to the nature of init. Could you elaborate more on when you see this error?
There was a problem hiding this comment.
This goes for all the other method checks in this file.
There was a problem hiding this comment.
I'm not that familiar with golang. I was unsure if there was risk of a nil receiver here. I will remove.
| return nil, fmt.Errorf("failed to list databases: %w", err) | ||
| } | ||
|
|
||
| structures := make([]*core.Structure, len(databases)) |
Co-authored-by: Mattias Sjödin <86059470+MattiasMTS@users.noreply.github.com>
feat(ArangoDB): add additional ArangoDB connection setup
|
@MattiasMTS thanks for taking the time to review! I've added a few changes, and applied your suggestions. Please take a look specifically at the latest changes in |
2 participants
This adds basic support for ArangoDB, a multi-modal Graph Database. This initial implementation phase supports primarily the dedicated query language: AQL and the json responses.