🎨 Add deces-update index #442
Conversation
WalkthroughThe pull request introduces a comprehensive update to the backend system, focusing on enhancing update management and indexing capabilities. The changes span multiple files and introduce new functions for initializing and managing update indices, modifying search and update controllers, and expanding the data model. The modifications improve the system's ability to handle updates across different indices, with a particular emphasis on flexibility in retrieving and processing update information. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Index as Update Index
participant ES as Elasticsearch
participant File as File System
App->>Index: initUpdateIndex()
Index->>ES: Check/Create Update Index
App->>Index: getAllUpdates()
Index->>ES: Retrieve Updates
App->>Index: updateFieldsToIndex(updates)
Index->>ES: Bulk Update Request
App->>File: addModification()
File->>File: Write Modification Files
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
backend/src/controllers/search.controller.ts (1)
Line range hint
390-413: Enhance file validation instoreProofRelying solely on
file.mimetypecan be insecure, as it can be manipulated. It's recommended to validate the file extension and, if possible, the actual file content to ensure only PDF files are accepted.Apply this diff to check the file extension:
if (file.mimetype !== 'application/pdf') { cb(new Error('Only PDF upload is allowed'), null) + } else if (path.extname(file.originalname).toLowerCase() !== '.pdf') { + cb(new Error('Only PDF upload is allowed'), null) }For enhanced security, consider using a library like
file-typeto verify the file's content.🧰 Tools
🪛 Biome (1.9.4)
[error] 382-383: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 383-384: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (9)
backend/src/updatedIds.ts (3)
47-48: Simplify property access using optional chainingIn lines 47-48, you can simplify the nested property access using optional chaining for better readability and to prevent potential errors when properties are undefined.
Apply this diff to use optional chaining:
- const analysis = settingsResponse.data && settingsResponse.data[modelIndex] && - settingsResponse.data[modelIndex].settings.index && settingsResponse.data[modelIndex].settings.index.analysis; + const analysis = settingsResponse.data?.[modelIndex]?.settings?.index?.analysis;🧰 Tools
🪛 Biome (1.9.4)
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
58-59: Simplify property access using optional chainingSimilarly, in lines 58-59, optional chaining can be used to simplify property access to
mappings.Apply this diff:
- const mappings = mappingsResponse.data && mappingsResponse.data[modelIndex] && - mappingsResponse.data[modelIndex].mappings; + const mappings = mappingsResponse.data?.[modelIndex]?.mappings;🧰 Tools
🪛 Biome (1.9.4)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
198-199: Avoid using thedeleteoperator for better performanceUsing the
deleteoperator can impact performance negatively. Instead, set the properties toundefinedor use object destructuring to omit them.Apply this diff to set properties to
undefined:- delete r.score; - delete r.scores; + r.score = undefined; + r.scores = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/src/controllers/search.controller.ts (2)
296-296: Avoid using thedeleteoperator for better performanceUsing
delete updateRequest.message;can impact performance. It's better to set the property toundefinedor use object rest operator to exclude it.Apply this diff:
- delete updateRequest.message; + updateRequest.message = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
307-312: Check the result ofaddModificationIn lines 307-312, after calling
addModification, you check for success but do not handle potential exceptions thrown byaddModification. Ensure that any thrown errors are caught and handled appropriately.Consider wrapping
addModificationin a try-catch block:- const success = await addModification(id, modification, date); + let success; + try { + success = await addModification(id, modification, date); + } catch (e) { + this.setStatus(500); + return { msg: "Update failed due to an error." }; + }backend/src/runRequest.ts (1)
9-9: LGTM! Consider index aliasing for better maintainability.The modification to include both indices (
deces,deces-updates) in search queries is correct. However, consider using Elasticsearch index aliases for better maintainability and flexibility.Instead of hardcoding indices, consider creating an alias that points to both indices:
#!/bin/bash # Create an alias pointing to both indices curl -X POST "http://elasticsearch:9200/_aliases" -H 'Content-Type: application/json' -d' { "actions" : [ { "add" : { "index" : "deces", "alias" : "deces-all" } }, { "add" : { "index" : "deces-updates", "alias" : "deces-all" } } ] }'Then modify the endpoints to use the alias:
- endpoint = `deces,deces-updates/_search?scroll=${scroll}` + endpoint = `deces-all/_search?scroll=${scroll}`Also applies to: 11-11
backend/src/models/entities.ts (1)
137-137: LGTM! Consider documenting index values.The addition of the optional
indexproperty to the Person interface is well-structured and aligns with the multi-index implementation.Consider adding JSDoc documentation to clarify allowed values:
+ /** + * The Elasticsearch index this record belongs to ('deces' or 'deces-updates') + */ index?: string;backend/src/controllers/search.spec.ts (1)
4-9: LGTM! Consider adding cleanup.The initialization of the update index before tests is a good practice. However, consider adding cleanup after tests.
Add an afterAll hook to clean up the test environment:
+import { cleanupUpdateIndex } from '../updatedIds'; + beforeAll(async () => { await initUpdateIndex(); }) + +afterAll(async () => { + await cleanupUpdateIndex(); +})backend/src/server.spec.ts (1)
78-85: Consider reducing the hardcoded delay.The test uses a 1-second delay to wait for index updates. This could make tests flaky and slow.
Consider using polling with a timeout:
- await new Promise(f => setTimeout(f, 1000)); + const timeout = 5000; + const interval = 100; + const startTime = Date.now(); + while (Date.now() - startTime < timeout) { + res = await server + .get(apiPath('search')) + .query({ lastName: 'Aiph7u' }); + if (res.body.response.persons.length > 0) break; + await new Promise(f => setTimeout(f, interval)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/src/buildRequest.ts(1 hunks)backend/src/controllers/search.controller.ts(5 hunks)backend/src/controllers/search.spec.ts(2 hunks)backend/src/index.ts(1 hunks)backend/src/models/entities.ts(1 hunks)backend/src/models/result.ts(4 hunks)backend/src/processStream.ts(1 hunks)backend/src/runRequest.ts(1 hunks)backend/src/server.spec.ts(3 hunks)backend/src/tsoa.ts(1 hunks)backend/src/updatedIds.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/processStream.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/updatedIds.ts
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/src/controllers/search.controller.ts
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ✅ Perfs test with artillery.io
🔇 Additional comments (6)
backend/src/models/result.ts (2)
158-158: LGTM! Index type definition looks good.The addition of the
_indexproperty with union type'deces'|'deces-updates'aligns well with the PR objective of implementing a second index for updates.
219-226: Verify the duplicate removal logic.The logic removes duplicates by keeping entries from the most recent index, but there are a few concerns:
- The logic modifies the array while iterating over it which could lead to skipped elements
- The comparison doesn't specify which index takes precedence
Consider refactoring to:
- filteredResults - .forEach((value, index, self) => { - const firstIndex = self.findIndex((item) => item.id === value.id); - if (index !== firstIndex && value.index !== self[firstIndex].index) { - self.splice(firstIndex, 1); - } - }) + const seen = new Map(); + filteredResults = filteredResults.reverse().filter(value => { + if (seen.has(value.id)) { + return false; + } + seen.set(value.id, true); + return true; + }).reverse();backend/src/buildRequest.ts (1)
Line range hint
392-409: LGTM! Exporting sort configuration improves modularity.The
referenceSortconstant is now properly exported, allowing reuse of the sorting configuration across different modules. The mapping between API fields and Elasticsearch fields is well-defined.backend/src/server.spec.ts (3)
7-17: LGTM! Proper test initialization.The
beforeAllhook ensures the update index is initialized before running tests, which is essential for testing the new functionality.
87-96: LGTM! Good admin access test coverage.The test properly verifies that admin users can access all updates.
98-107: LGTM! Good author access test coverage.The test properly verifies that authors can access their own updates.
backend/src/updatedIds.ts
Outdated
| export const updateFieldsToIndex = async (updates: any): Promise<boolean> => { | ||
| if (Object.keys(updates).length === 0) return |
There was a problem hiding this comment.
Ensure consistent return value in updateFieldsToIndex
The function updateFieldsToIndex is declared to return a Promise<boolean>, but when updates is empty, it returns undefined at line 87. To maintain consistency with the return type, consider returning true when there are no updates to process.
Apply this diff to return true when updates is empty:
export const updateFieldsToIndex = async (updates: any): Promise<boolean> => {
- if (Object.keys(updates).length === 0) return
+ if (Object.keys(updates).length === 0) return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const updateFieldsToIndex = async (updates: any): Promise<boolean> => { | |
| if (Object.keys(updates).length === 0) return | |
| export const updateFieldsToIndex = async (updates: any): Promise<boolean> => { | |
| if (Object.keys(updates).length === 0) return true; |
backend/src/updatedIds.ts
Outdated
| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | ||
| try { | ||
| await accessAsync(`${process.env.PROOFS}/${id}`); | ||
| } catch(err) { | ||
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | ||
| } | ||
| try { | ||
| await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification)); | ||
| if (!updatedFields[id]) { updatedFields[id] = [] } | ||
| updatedFields[id].push(modification); | ||
| updateFieldsToIndex({[id]: updatedFields[id]}); |
There was a problem hiding this comment.
Await the promise from updateFieldsToIndex
In the addModification function, the call to updateFieldsToIndex at line 215 is asynchronous but not awaited. This may lead to unexpected behavior if the function depends on the completion of this call.
Apply this diff to await the asynchronous function:
updatedFields[id].push(modification);
- updateFieldsToIndex({[id]: updatedFields[id]});
+ await updateFieldsToIndex({ [id]: updatedFields[id] });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | |
| try { | |
| await accessAsync(`${process.env.PROOFS}/${id}`); | |
| } catch(err) { | |
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | |
| } | |
| try { | |
| await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification)); | |
| if (!updatedFields[id]) { updatedFields[id] = [] } | |
| updatedFields[id].push(modification); | |
| updateFieldsToIndex({[id]: updatedFields[id]}); | |
| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | |
| try { | |
| await accessAsync(`${process.env.PROOFS}/${id}`); | |
| } catch(err) { | |
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | |
| } | |
| try { | |
| await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification)); | |
| if (!updatedFields[id]) { updatedFields[id] = [] } | |
| updatedFields[id].push(modification); | |
| await updateFieldsToIndex({ [id]: updatedFields[id] }); |
| @@ -262,16 +262,16 @@ export class SearchController extends Controller { | |||
| @Body() updateRequest: UpdateRequest, | |||
| @Request() request: express.Request | |||
| ): Promise<any> { | |||
| const date = new Date(Date.now()).toISOString() | |||
| await this.storeProof(request); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors from storeProof
At line 266, the call to await this.storeProof(request); may throw an error if file upload fails. Consider wrapping this call in a try-catch block to handle potential errors gracefully.
Apply this diff to handle errors:
const date = new Date(Date.now()).toISOString()
- await this.storeProof(request);
+ try {
+ await this.storeProof(request);
+ } catch (err) {
+ this.setStatus(400);
+ return { msg: err.message };
+ }Committable suggestion skipped: line range outside the PR's diff.
backend/src/index.ts
Outdated
| initUpdateIndex(); | ||
| const updates = getAllUpdates(); | ||
| updateFieldsToIndex(updates); |
There was a problem hiding this comment.
Ensure asynchronous initialization completes before starting the server
The functions initUpdateIndex and updateFieldsToIndex are asynchronous but not awaited. This might lead to race conditions where the server starts before these initializations are complete. Ensure they are awaited to guarantee proper initialization.
Apply this diff to await the asynchronous functions:
-initUpdateIndex();
-const updates = getAllUpdates();
-updateFieldsToIndex(updates);
const port = 8080;
-app.listen( port, () => {
- console.log( `server started at http://localhost:${ port }` );
-} );
+(async () => {
+ await initUpdateIndex();
+ const updates = getAllUpdates();
+ await updateFieldsToIndex(updates);
+
+ app.listen(port, () => {
+ console.log(`server started at http://localhost:${port}`);
+ });
+})();Committable suggestion skipped: line range outside the PR's diff.
| admin: "administration access", | ||
| user: "user access" |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused 'admin' scope definition
The 'admin' scope is defined in tsoa.ts but not used anywhere in the codebase. All endpoints currently use only the 'user' scope. Consider:
- Removing the unused 'admin' scope definition, or
- Adding a TODO comment if admin functionality is planned for future implementation
🔗 Analysis chain
LGTM! Verify scope requirements.
The addition of the 'user' scope alongside 'admin' enhances access control granularity. This is particularly relevant for managing access to the new updates index.
Let's verify the scope usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JWT scope usage in controllers and routes
rg -g '*.ts' -A 3 '@Security\(.*\)' ./backend/src/
Length of output: 2969
Script:
#!/bin/bash
# Search for any references to admin scope or other security decorators
rg -g '*.ts' -i '(admin.*access|@security|authentication|authorization)' ./backend/src/
Length of output: 6828
| @@ -90,7 +95,7 @@ describe('search.controller.ts - POST id', () => { | |||
| it('update id', async () => { | |||
| const body = { | |||
| 'author_id': '[email protected]', | |||
| lastName: 'Aiph7u', | |||
| lastName: 'Aeboox9e', | |||
There was a problem hiding this comment.
💡 Codebase verification
Test data inconsistency detected
The lastName value should remain as 'Aiph7u' to maintain consistency with server.spec.ts, as both tests are validating the same functionality with the same author_id ('[email protected]').
backend/src/controllers/search.spec.ts: Revert lastName from 'Aeboox9e' back to 'Aiph7u'
🔗 Analysis chain
Verify test data change rationale.
The modification of the test data from 'Aiph7u' to 'Aeboox9e' seems unrelated to the PR objective.
Let's check if this change is related to any other modifications:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for occurrences of both old and new test values
rg -g '*.ts' -g '*.js' "Aiph7u|Aeboox9e" ./backend/
Length of output: 256
Script:
#!/bin/bash
# Get context around the test data usage in both files
echo "=== search.spec.ts ==="
rg -B 5 -A 5 "Aeboox9e" ./backend/src/controllers/search.spec.ts
echo -e "\n=== server.spec.ts ==="
rg -B 5 -A 5 "Aiph7u" ./backend/src/server.spec.ts
Length of output: 1295
backend/src/models/result.ts
Outdated
| update.author = author ? author.substring(0,2) | ||
| + '...' + author.replace(/@.*/,'').substring(author.replace(/@.*/,'').length-2) | ||
| + '@' + author.replace(/.*@/,''); | ||
| + '@' + author.replace(/.*@/,'') : ""; |
There was a problem hiding this comment.
Review the author anonymization logic for potential PII leakage.
The current anonymization:
- Shows first 2 chars of username
- Shows last 2 chars before @
- Shows full domain
This might not provide sufficient privacy protection for short usernames.
Consider using a more robust anonymization:
- update.author = author ? author.substring(0,2)
- + '...' + author.replace(/@.*/,'').substring(author.replace(/@.*/,'').length-2)
- + '@' + author.replace(/.*@/,'') : "";
+ update.author = author ?
+ (author.length > 8 ?
+ author.substring(0,2) + '...' + author.replace(/@.*/,'').substring(author.replace(/@.*/,'').length-2)
+ : '...')
+ + '@' + author.replace(/.*@/,'') : "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| update.author = author ? author.substring(0,2) | |
| + '...' + author.replace(/@.*/,'').substring(author.replace(/@.*/,'').length-2) | |
| + '@' + author.replace(/.*@/,''); | |
| + '@' + author.replace(/.*@/,'') : ""; | |
| update.author = author ? | |
| (author.length > 8 ? | |
| author.substring(0,2) + '...' + author.replace(/@.*/,'').substring(author.replace(/@.*/,'').length-2) | |
| : '...') | |
| + '@' + author.replace(/.*@/,'') : ""; |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/src/controllers/search.controller.ts (1)
Line range hint
395-419: Improve error handling in storeProof method.The pipeline detected issues with error handling in the multer configuration:
- Unused 'err' variable
- Promise rejection without Error object
private async storeProof(request: express.Request): Promise<any> { const storage = multer.diskStorage({ destination: void(async (req: any, file: any, cb: any) => { if (file.mimetype !== 'application/pdf') { - cb(new Error('Only PDF upload is allowed'), null) + cb(new Error('Only PDF upload is allowed'), null); + return; } const { id } = req.params; const dir = `${process.env.PROOFS}/${id as string}`; try { await accessAsync(dir); } catch(err) { await mkdirAsync(dir, { recursive: true }); } cb(null, dir) }), filename: (_, file, cb) => { const uniqueSuffix = `${Date.now()}-${Math.round(Math.random() * 1E9)}` cb(null, `${uniqueSuffix}_${file.originalname}`) } }) const multerSingle = multer({storage}).any(); return new Promise((resolve, reject) => { multerSingle(request, undefined, (error: any) => { if (error) { - reject(error); + reject(new Error(error.message)); } resolve(true); }); }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 387-388: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 388-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (3)
backend/src/updatedIds.ts (3)
47-48: Simplify nested property access with optional chaining.The static analysis tool suggests using optional chaining for better readability.
- const analysis = settingsResponse.data && settingsResponse.data[modelIndex] && - settingsResponse.data[modelIndex].settings.index && settingsResponse.data[modelIndex].settings.index.analysis; + const analysis = settingsResponse.data?.[modelIndex]?.settings?.index?.analysis; - const mappings = mappingsResponse.data && mappingsResponse.data[modelIndex] && - mappingsResponse.data[modelIndex].mappings; + const mappings = mappingsResponse.data?.[modelIndex]?.mappings;Also applies to: 58-59
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
86-116: Add type definition for the updates parameter.The function uses
anytype for the updates parameter, which could be better typed for improved type safety.-export const updateFieldsToIndex = async (updates: any): Promise<boolean> => { +interface UpdateFields { + [id: string]: Array<{ + fields: Record<string, unknown>; + _id: string; + _source: Record<string, unknown>; + }>; +} +export const updateFieldsToIndex = async (updates: UpdateFields): Promise<boolean> => {
198-199: Consider using property assignment instead of delete operator.The static analysis tool suggests avoiding the delete operator for better performance.
- delete r.score; - delete r.scores; + r.score = undefined; + r.scores = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/controllers/search.controller.ts(5 hunks)backend/src/index.ts(1 hunks)backend/src/models/result.ts(4 hunks)backend/src/updatedIds.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test & Build
backend/src/index.ts
[error] 12-12: Unexpected console statement detected (no-console)
backend/src/controllers/search.controller.ts
[warning] 405-419: Variable 'err' is unused and Promise rejection should use Error object (@typescript-eslint/no-unused-vars, @typescript-eslint/prefer-promise-reject-errors)
🪛 Biome (1.9.4)
backend/src/controllers/search.controller.ts
[error] 301-301: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/src/updatedIds.ts
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (7)
backend/src/models/result.ts (3)
158-158: LGTM! Well-typed index property addition.The addition of the
_indexproperty with strict typing ('deces'|'deces-updates') effectively supports the PR objective of implementing a second index for modifications.
257-257: LGTM! Proper index propagation.The index information is correctly propagated from the raw hit to the Person object, which is essential for the duplicate filtering logic.
313-317: Review the author anonymization logic for potential PII leakage.The current implementation still has privacy concerns as noted in a previous review.
Run this script to check for short usernames that might be insufficiently anonymized:
#!/bin/bash # Description: Check for potentially exposed short usernames in the updates # Expected: No usernames shorter than 8 characters should be fully visible rg -A 2 'author.*@' | grep -E 'author.{0,8}@'backend/src/index.ts (1)
6-14: LGTM! Well-structured async initialization sequence.The initialization sequence is properly ordered and all async operations are correctly awaited before starting the server.
🧰 Tools
🪛 GitHub Actions: Test & Build
[error] 12-12: Unexpected console statement detected (no-console)
backend/src/updatedIds.ts (2)
31-84: LGTM! Robust index initialization with proper error handling.The implementation includes:
- Proper checking for existing index
- Retrieval of settings and mappings from model index
- Comprehensive error handling
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
205-221: LGTM! Well-structured modification handling.The addModification function properly:
- Creates directories if needed
- Handles file writes asynchronously
- Updates the in-memory state
- Properly propagates errors
backend/src/controllers/search.controller.ts (1)
389-392: LGTM! Clean implementation of update list retrieval.The implementation properly:
- Handles admin vs user permissions
- Uses the new update index functionality
- Cleans the updates before returning
backend/src/models/result.ts
Outdated
|
|
||
| filteredResults | ||
| .forEach((value, index, self) => { | ||
| const firstIndex = self.findIndex((item) => item.id === value.id); | ||
| if (index !== firstIndex && value.index !== self[firstIndex].index) { | ||
| self.splice(firstIndex, 1); | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider deterministic duplicate handling.
The current logic removes the first occurrence when finding duplicates across indices. This could lead to inconsistent results depending on the order of entries.
Consider explicitly prioritizing one index over the other:
filteredResults
.forEach((value, index, self) => {
const firstIndex = self.findIndex((item) => item.id === value.id);
- if (index !== firstIndex && value.index !== self[firstIndex].index) {
- self.splice(firstIndex, 1);
+ if (index !== firstIndex) {
+ // Prioritize 'deces-updates' index over 'deces'
+ const shouldRemoveFirst = self[firstIndex].index === 'deces' && value.index === 'deces-updates';
+ if (shouldRemoveFirst) {
+ self.splice(firstIndex, 1);
+ } else if (value.index === 'deces') {
+ self.splice(index, 1);
+ }
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filteredResults | |
| .forEach((value, index, self) => { | |
| const firstIndex = self.findIndex((item) => item.id === value.id); | |
| if (index !== firstIndex && value.index !== self[firstIndex].index) { | |
| self.splice(firstIndex, 1); | |
| } | |
| }) | |
| filteredResults | |
| .forEach((value, index, self) => { | |
| const firstIndex = self.findIndex((item) => item.id === value.id); | |
| if (index !== firstIndex) { | |
| // Prioritize 'deces-updates' index over 'deces' | |
| const shouldRemoveFirst = self[firstIndex].index === 'deces' && value.index === 'deces-updates'; | |
| if (shouldRemoveFirst) { | |
| self.splice(firstIndex, 1); | |
| } else if (value.index === 'deces') { | |
| self.splice(index, 1); | |
| } | |
| } | |
| }) |
backend/src/index.ts
Outdated
| app.listen(port, () => { | ||
| console.log(`server started at http://localhost:${port}`); | ||
| }); |
There was a problem hiding this comment.
Replace console.log with the logger.
The pipeline detected a linting error for the console.log statement.
- console.log(`server started at http://localhost:${port}`);
+ log({ msg: `server started at http://localhost:${port}` });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Test & Build
[error] 12-12: Unexpected console statement detected (no-console)
cristianpb
force-pushed
the
feat/index-update-rebase-2
branch
from
94fad34 to
cdf96b0
Compare
cristianpb
force-pushed
the
feat/index-update-rebase-2
branch
from
cdf96b0 to
a87465e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/src/updatedIds.ts (2)
31-84: Enhance error handling in index initialization.The error handling could be more robust:
- The catch block at line 40 silently swallows the error when the index doesn't exist.
- Error messages for failed settings/mappings retrieval are duplicated.
Apply this diff to improve error handling:
export const initUpdateIndex = async (): Promise<boolean> => { if (updateIndexCreated) { return true } try { const test = await axios(`http://elasticsearch:9200/${updateIndex}/_settings`); if (test.status === 200) { log({msg: `${updateIndex} already exists`}); updateIndexCreated = true; return true; } } catch(e) { + // Expected error when index doesn't exist + if (e.response?.status !== 404) { + log({msg: 'failed checking update index', error: e.message}); + return false; + } // create index const settingsResponse = await axios(`http://elasticsearch:9200/${modelIndex}/_settings`); if (settingsResponse.status !== 200) { - log({msg: 'failed initiating missing update index', error: "coudn't retrive settings from model index"}); + log({msg: 'failed initiating missing update index', error: `couldn't retrieve settings from model index: ${settingsResponse.status}`}); return false; } const analysis = settingsResponse.data && settingsResponse.data[modelIndex] && settingsResponse.data[modelIndex].settings.index && settingsResponse.data[modelIndex].settings.index.analysis; if (!analysis) { - log({msg: 'failed initiating missing update index', error: "coudn't retrive settings from model index"}); + log({msg: 'failed initiating missing update index', error: "settings.index.analysis not found in model index"}); return false }; const mappingsResponse = await axios(`http://elasticsearch:9200/${modelIndex}/_mappings`); if (mappingsResponse.status !== 200) { - log({msg: 'failed initiating missing update index', error: "coudn't retrive mappings from model index"}); + log({msg: 'failed initiating missing update index', error: `couldn't retrieve mappings from model index: ${mappingsResponse.status}`}); return false; } const mappings = mappingsResponse.data && mappingsResponse.data[modelIndex] && mappingsResponse.data[modelIndex].mappings; if (!mappings) { - log({msg: 'failed initiating missing update index', error: "coudn't retrive mappings from model index"}); + log({msg: 'failed initiating missing update index', error: "mappings not found in model index"}); return false; };🧰 Tools
🪛 Biome (1.9.4)
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
86-116: Improve type safety and error handling in update processing.The function uses
anytype extensively and could benefit from better error handling:
- The
updatesparameter andupdateListvariable useanytype.- Error handling could provide more context.
Consider creating interfaces for the update types and enhancing error handling:
interface Update { fields: Record<string, unknown>; id: string; date: string; // Add other required fields } interface UpdateMap { [key: string]: Update[]; } export const updateFieldsToIndex = async (updates: UpdateMap): Promise<boolean> => { if (Object.keys(updates).length === 0) return true; try { const updateList = await resultsFromUpdates(updates); // ... rest of the function } catch (err) { log({msg: 'Error processing updates', error: err.message, updates: Object.keys(updates)}); return false; } }backend/src/controllers/search.controller.ts (1)
Line range hint
395-422: Enhance file handling and validation in proof storage.Consider improving:
- File type validation could be more robust
- File size limits are not enforced
- Error handling could be more specific
Apply this diff to enhance file handling:
private async storeProof(request: express.Request): Promise<any> { + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB + const ALLOWED_MIME_TYPES = ['application/pdf']; const storage = multer.diskStorage({ destination: void(async (req: any, file: any, cb: any) => { - if (file.mimetype !== 'application/pdf') { - cb(new Error('Only PDF upload is allowed'), null) + if (!ALLOWED_MIME_TYPES.includes(file.mimetype)) { + cb(new Error(`Only ${ALLOWED_MIME_TYPES.join(', ')} uploads are allowed`), null) } const { id } = req.params; const dir = `${process.env.PROOFS}/${id as string}`; try { await accessAsync(dir); } catch(err) { + if (err.code !== 'ENOENT') { + cb(new Error(`Error accessing directory: ${err.message}`), null); + return; + } await mkdirAsync(dir, { recursive: true }); } cb(null, dir) }), filename: (_, file, cb) => { const uniqueSuffix = `${Date.now()}-${Math.round(Math.random() * 1E9)}` cb(null, `${uniqueSuffix}_${file.originalname}`) } }) - const multerSingle = multer({storage}).any(); + const multerSingle = multer({ + storage, + limits: { + fileSize: MAX_FILE_SIZE + } + }).any(); return new Promise((resolve, reject) => { multerSingle(request, undefined, (error: any) => { if (error) { + if (error.code === 'LIMIT_FILE_SIZE') { + reject(new Error(`File size must be less than ${MAX_FILE_SIZE / 1024 / 1024}MB`)); + return; + } reject(error); } resolve(true); }); }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 387-388: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 388-389: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/controllers/search.controller.ts(5 hunks)backend/src/index.ts(1 hunks)backend/src/models/result.ts(4 hunks)backend/src/updatedIds.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/updatedIds.ts
[error] 47-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/src/controllers/search.controller.ts
[error] 301-301: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🐳 Build docker image
🔇 Additional comments (4)
backend/src/models/result.ts (2)
158-158: LGTM! Well-typed index property.The addition of the
_indexproperty with strict typing to'deces'|'deces-updates'aligns well with the PR objective of implementing a second index.
319-323: LGTM! Improved author anonymization.The updated author anonymization logic provides better privacy protection by handling short usernames appropriately.
backend/src/controllers/search.controller.ts (2)
265-271: LGTM! Improved error handling and modification processing.The changes enhance the robustness of the update process:
- Added proper error handling for proof storage
- Streamlined modification handling with better success/failure reporting
Also applies to: 301-317
389-392: LGTM! Simplified update retrieval.The changes streamline the update retrieval process by using the new utility functions, making the code more maintainable.
| export const getAuthorUpdates = (author: string):any => { | ||
| const updates:any = {}; | ||
| Object.keys(updatedFields).forEach((id:any) => { | ||
| let keep = false; | ||
| const modifications = updatedFields[id].map((m:any) => { | ||
| const modif:any = {...m} | ||
| if (modif.author !== author) { | ||
| modif.author = modif.author ? modif.author.substring(0,2) | ||
| + '...' + modif.author.replace(/@.*/,'').substring(modif.author.replace(/@.*/,'').length-2) | ||
| + '@' + modif.author.replace(/.*@/,'') : ''; | ||
| modif.message = undefined; | ||
| modif.review = undefined; | ||
| } else { | ||
| keep = true | ||
| } | ||
| return modif; | ||
| }); | ||
| if (keep) { | ||
| updates[id] = modifications; | ||
| } | ||
| }); | ||
| return updates; | ||
| } |
There was a problem hiding this comment.
Enhance privacy protection in author anonymization.
The current author anonymization logic:
- Shows first 2 chars of username
- Shows last 2 chars before @
- Shows full domain
This might not provide sufficient privacy protection for short usernames.
Apply this diff to improve privacy protection:
- if (modif.author !== author) {
- modif.author = modif.author ? modif.author.substring(0,2)
- + '...' + modif.author.replace(/@.*/,'').substring(modif.author.replace(/@.*/,'').length-2)
- + '@' + modif.author.replace(/.*@/,'') : '';
+ if (modif.author && modif.author !== author) {
+ const [username, domain] = modif.author.split('@');
+ modif.author = username.length > 8
+ ? `${username.substring(0,2)}...${username.slice(-2)}@${domain}`
+ : `...@${domain}`;
modif.message = undefined;
modif.review = undefined;
} else {Also, consider adding TypeScript interfaces for better type safety:
interface Modification {
author: string;
message?: string;
review?: unknown;
// Add other required fields
}
interface UpdateMap {
[key: string]: Modification[];
}| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | ||
| try { | ||
| await accessAsync(`${process.env.PROOFS}/${id}`); | ||
| } catch(err) { | ||
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | ||
| } | ||
| try { | ||
| await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification)); | ||
| if (!updatedFields[id]) { updatedFields[id] = [] } | ||
| updatedFields[id].push(modification); | ||
| await updateFieldsToIndex({[id]: updatedFields[id]}); | ||
| return true; | ||
| } catch(e) { | ||
| log({ msg: "Update failed", error: e.message, id, modification }); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate environment variables and enhance error handling.
The function uses process.env.PROOFS without validation and could benefit from better error handling:
- Missing validation for required environment variables
- Generic error message in catch block
Apply this diff to improve environment variable validation and error handling:
export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => {
+ const proofsDir = process.env.PROOFS;
+ if (!proofsDir) {
+ log({ msg: "Update failed", error: "PROOFS environment variable not set" });
+ return false;
+ }
try {
- await accessAsync(`${process.env.PROOFS}/${id}`);
+ await accessAsync(`${proofsDir}/${id}`);
} catch(err) {
await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true });
}
try {
- await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification));
+ const filePath = `${proofsDir}/${id}/${date}_${id}.json`;
+ await writeFileAsync(filePath, JSON.stringify(modification));
if (!updatedFields[id]) { updatedFields[id] = [] }
updatedFields[id].push(modification);
await updateFieldsToIndex({[id]: updatedFields[id]});
return true;
} catch(e) {
- log({ msg: "Update failed", error: e.message, id, modification });
+ log({
+ msg: "Update failed",
+ error: e.message,
+ context: {
+ id,
+ date,
+ operation: e.code === 'ENOENT' ? 'file_write' : 'unknown'
+ }
+ });
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | |
| try { | |
| await accessAsync(`${process.env.PROOFS}/${id}`); | |
| } catch(err) { | |
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | |
| } | |
| try { | |
| await writeFileAsync(`${process.env.PROOFS}/${id}/${date}_${id}.json`, JSON.stringify(modification)); | |
| if (!updatedFields[id]) { updatedFields[id] = [] } | |
| updatedFields[id].push(modification); | |
| await updateFieldsToIndex({[id]: updatedFields[id]}); | |
| return true; | |
| } catch(e) { | |
| log({ msg: "Update failed", error: e.message, id, modification }); | |
| return false; | |
| } | |
| } | |
| export const addModification = async (id: string, modification: Modification, date: string): Promise<boolean> => { | |
| const proofsDir = process.env.PROOFS; | |
| if (!proofsDir) { | |
| log({ msg: "Update failed", error: "PROOFS environment variable not set" }); | |
| return false; | |
| } | |
| try { | |
| await accessAsync(`${proofsDir}/${id}`); | |
| } catch(err) { | |
| await mkdirAsync(`${process.env.PROOFS}/${id}`, { recursive: true }); | |
| } | |
| try { | |
| const filePath = `${proofsDir}/${id}/${date}_${id}.json`; | |
| await writeFileAsync(filePath, JSON.stringify(modification)); | |
| if (!updatedFields[id]) { updatedFields[id] = [] } | |
| updatedFields[id].push(modification); | |
| await updateFieldsToIndex({[id]: updatedFields[id]}); | |
| return true; | |
| } catch(e) { | |
| log({ | |
| msg: "Update failed", | |
| error: e.message, | |
| context: { | |
| id, | |
| date, | |
| operation: e.code === 'ENOENT' ? 'file_write' : 'unknown' | |
| } | |
| }); | |
| return false; | |
| } | |
| } |
|
Caution No docstrings were generated. |
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Great work. tested on small scale, to be tester with full dataset !
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates