-
-
Notifications
You must be signed in to change notification settings - Fork 27
[Store] Add SurrealDB #139
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
base: main
Are you sure you want to change the base?
Conversation
8c9ad55
to
e9b7322
Compare
We just created some Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're really on a great streak here to level up the store bridges - thanks for that! 🙏
Somehow the example doesn't work for me again - see below
e9b7322
to
e38a096
Compare
@OskarStark 👋🏻 I updated the |
Needs a rebase please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small things about the inner contract of the Store implementation here
$response = $this->httpClient->request($method, $url, array_merge($finalPayload, [ | ||
'headers' => array_merge($extraHeaders, [ | ||
'Accept' => 'application/json', | ||
'Content-Type' => 'application/json', | ||
]), | ||
])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about changing the array $extraHeaders
argument to string $authenticationToek
and moving everything to here?
$response = $this->httpClient->request($method, $url, array_merge($finalPayload, [ | |
'headers' => array_merge($extraHeaders, [ | |
'Accept' => 'application/json', | |
'Content-Type' => 'application/json', | |
]), | |
])); | |
$response = $this->httpClient->request($method, $url, array_merge($finalPayload, [ | |
'headers' => [ | |
'Accept' => 'application/json', | |
'Content-Type' => 'application/json', | |
'Surreal-NS' => $this->namespace, | |
'Surreal-DB' => $this->database, | |
'Authorization' => \sprintf('Bearer %s', $authenticationToken), | |
], | |
])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The authentication token is not always required, same for Surreal-NS
and Surreal-DB
, those headers are not required during signin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, missed that, than please let's decouple those two types of requests.
meaning, instead of using the ->request(...)
in authenticate - just use the plain httpClient.
this will also remove that is_string ? body : json
handling in the request
method.
* namespacedUser?: bool | ||
* } $options The namespacedUser option is used to determine if the user is root or not, if not, both the namespace and database must be specified | ||
*/ | ||
private function authenticate(array $options): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we only use that one specific option here, we could replace the blurry array $options
by bool $namespacedUser
in general i wonder why that is an option and not part of the contructor arguments like $user
and $password
- isn't it directly connected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespacedUser
is used if you're not root
, this force the user to specify a user with its namespace, I can define the options as false
by default.
e38a096
to
23ab86a
Compare
{ | ||
$authenticationPayload = [ | ||
'user' => $this->user, | ||
'pass' => $this->password, | ||
]; | ||
|
||
if (\array_key_exists('namespacedUser', $options) && !$options['namespacedUser']) { | ||
$authenticationPayload['ns'] = $this->namespace; | ||
$authenticationPayload['db'] = $this->database; | ||
} | ||
|
||
$authenticationResponse = $this->request('POST', 'signin', $authenticationPayload); | ||
|
||
if (!\array_key_exists('token', $authenticationResponse)) { | ||
throw new RuntimeException('The SurrealDB authentication response does not contain a token.'); | ||
} | ||
|
||
return $authenticationResponse['token']; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about a local cache var to make sure we authenticate only once per request/command/message?
{
if (isset($this->authenticationToken) {
return $this->authenticationToken;
}
$authenticationPayload = ['user' => $this->user, 'pass' => $this->password];
if ($this->isNamespacedUser) {
$authenticationPayload['ns'] = $this->namespace;
$authenticationPayload['db'] = $this->database;
}
$authenticationResponse = $this->httpClient->request('POST', sprintf('%s/signin'), $this->endpointUrl, $authenticationPayload)->toArray();
if (!\array_key_exists('token', $authenticationResponse)) {
throw new RuntimeException('The SurrealDB authentication response does not contain a token.');
}
return $this->authenticationToken = $authenticationResponse['token'];
}
#[\SensitiveParameter] | ||
private readonly string $user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[\SensitiveParameter] | |
private readonly string $user, | |
#[\SensitiveParameter] private readonly string $user, |
same for the others
Hi 👋🏻
This PR aim to introduce
SurrealDB
as a store for embeddings, the store use theHTTP
API as the PHP SDK is ugly to use (and does not allow mocks), the tests needs to be improved / completed but the main implementation is ready.PS: By default, we can't send more than
1275
embeddings during the insertion phase, the SDK can bypass this limitation but again, testing issues, API and more so, the road to follow is up to the team.