-
-
Notifications
You must be signed in to change notification settings - Fork 252
feat: vector buckets #774
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: master
Are you sure you want to change the base?
feat: vector buckets #774
Conversation
⛔ Snyk checks have failed. 8 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
01797cc
to
a2715e7
Compare
Pull Request Test Coverage Report for Build 18461456855Details
💛 - Coveralls |
src/app.ts
Outdated
app.register(routes.cdn, { prefix: 'cdn' }) | ||
app.register(routes.healthcheck, { prefix: 'health' }) | ||
app.register(routes.iceberg, { prefix: 'iceberg/v1' }) | ||
app.register(routes.vectors, { prefix: 'vectors' }) |
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.
should this be singular ("vector")? to match: bucket, object, ...
src/http/error-handler.ts
Outdated
: 400 | ||
|
||
if (statusCode === 500) { | ||
console.log('error') |
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.
debug log, remove
} | ||
} | ||
|
||
throw ERRORS.TransactionError('Transaction failed after maximum retries', lastError) |
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.
Might be nice to make "after maximum retries" conditional on if there were retires, and/or maybe include the retry count in the error message to avoid confusion if we need to debug this in the future.
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.
Not sure i understood this, but in isolation mode: 'serialize' retrying the transaction is a common pattern
if the max retries are reached means that there are too many concurrent operations, which makes this transaction fail
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.
I just meant adding the count to the error message or only saying "after maximum retries" if there were retries as currently it says that even for requests that had no retries.
a2715e7
to
6bf5f59
Compare
6bf5f59
to
f2ae953
Compare
search: query.search, | ||
}) | ||
|
||
return response.status(200).send(bucket) |
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.
Cross-site Scripting (XSS)
Unsanitized input from an HTTP parameter
flows into send
, where it is used to render an HTML page returned to the user. This may result in a Cross-Site Scripting attack (XSS).
Line 119 | CWE-79 | Priority score 848 | Learn more about this vulnerability
Data flow: 14 steps
Step 1 - 3
storage/src/http/routes/iceberg/bucket.ts
Line 109 in f2ae953
const query = request.query |
Step 4 - 7
src/http/routes/iceberg/bucket.ts#L112
Step 8 - 12
src/http/routes/iceberg/bucket.ts#L111
Step 13 - 14
storage/src/http/routes/iceberg/bucket.ts
Line 119 in f2ae953
return response.status(200).send(bucket) |
What kind of change does this PR introduce?
Feature
What is the new behaviour?
Implement Vector Bucket data source
Supported Operations:
Authentication mechanisms: