-
Notifications
You must be signed in to change notification settings - Fork 413
fix(storage-resize-images): fix backfill and excluded paths parameter not being applied #2513
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: next
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR fixes issues in the storage-resize-images extension where the backfill process was experiencing exponential growth of items and excluded paths weren't being properly filtered during file listing operations.
- Adds path exclusion filtering to prevent scanning of already-resized images during backfill
- Implements optimized query handling with delimiter parameter for better performance
- Introduces comprehensive test coverage for the backfill fix functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
storage-resize-images/functions/src/index.ts | Adds path exclusion functions and integrates them into backfill process |
storage-resize-images/functions/tests/backfill-fix.test.ts | New test file validating path exclusion logic and preventing exponential growth |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
function buildOptimizedQuery(baseQuery: any, excludePathList?: string[]): any { | ||
if (!excludePathList || excludePathList.length === 0) { | ||
return baseQuery; | ||
} | ||
|
||
const optimizedQuery = { ...baseQuery }; | ||
optimizedQuery.delimiter = "/"; | ||
|
||
return optimizedQuery; | ||
} | ||
|
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 buildOptimizedQuery
function is defined but never used in the codebase. Consider removing it or integrating it into the getFilesWithPathExclusions
function to avoid code duplication.
function buildOptimizedQuery(baseQuery: any, excludePathList?: string[]): any { | |
if (!excludePathList || excludePathList.length === 0) { | |
return baseQuery; | |
} | |
const optimizedQuery = { ...baseQuery }; | |
optimizedQuery.delimiter = "/"; | |
return optimizedQuery; | |
} |
Copilot uses AI. Check for mistakes.
@@ -18,6 +18,7 @@ import * as admin from "firebase-admin"; | |||
import { getFunctions } from "firebase-admin/functions"; | |||
import { getExtensions } from "firebase-admin/extensions"; | |||
import * as functions from "firebase-functions/v1"; | |||
import { logger } from "firebase-functions"; |
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 logger
import is added but not used anywhere in the code. Remove this unused import.
import { logger } from "firebase-functions"; |
Copilot uses AI. Check for mistakes.
} | ||
|
||
return files.filter((file) => { | ||
const filePath = path.dirname(file.metadata.name); |
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.
Using path.dirname()
on a storage object name may not work correctly for files in the root directory, as it would return '.' instead of an empty string. Consider using file.metadata.name.split('/').slice(0, -1).join('/')
or handle the root case explicitly.
const filePath = path.dirname(file.metadata.name); | |
const filePath = file.metadata.name.split('/').slice(0, -1).join('/'); |
Copilot uses AI. Check for mistakes.
for (const excludePath of excludePathList) { | ||
const trimmedExcludePath = excludePath | ||
.trim() | ||
.replace(/\*/g, "([a-zA-Z0-9_\\-\\+.\\s\\/]*)?"); |
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 regex pattern allows arbitrary characters including forward slashes, which could lead to unintended path matching. Consider using a more restrictive character set or escaping special regex characters properly to prevent regex injection.
Copilot uses AI. Check for mistakes.
Fixes #2506
This PR fixes the exponential growth of items for the backfill listing and excluded paths parameter not being applied during listing