feat(log-viewer-webui): Refactor S3Manager into a fastify plugin. #689
Conversation
WalkthroughThe pull request introduces modifications to the S3 management functionality in the log viewer web UI. The changes focus on improving the S3 client initialization and integration with the Fastify server. The Changes
Sequence DiagramsequenceDiagram
participant App as Fastify App
participant S3Manager as S3Manager
participant S3 as S3 Client
App->>S3Manager: Create with region
S3Manager-->>App: S3Manager instance
App->>App: Decorate with S3Manager
alt S3 Enabled
App->>S3Manager: isEnabled()
S3Manager-->>App: true
App->>S3Manager: getPreSignedURL()
S3Manager->>S3: Generate Pre-Signed URL
S3-->>S3Manager: Return URL
S3Manager-->>App: Pre-Signed URL
else S3 Disabled
App->>S3Manager: isEnabled()
S3Manager-->>App: false
App->>App: Use local path
end
Possibly Related PRs
Suggested Reviewers
✨ 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 (
|
| @@ -55,4 +69,8 @@ class S3Manager { | |||
| } | |||
| } | |||
|
|
|||
| export default S3Manager; | |||
| export default fastifyPlugin(async (app, region) => { | |||
There was a problem hiding this comment.
fastifyPlugin requires the second argument in the callback to be an options object: See https://fastify.dev/docs/latest/Reference/Plugins/#plugin-options
because Fastify specific options like logLevel, logSerializers, prefix are expected to show up in the options object if they exist.
| export default fastifyPlugin(async (app, region) => { | |
| export default fastifyPlugin(async (app, options) => { |
| export default fastifyPlugin(async (app, region) => { | ||
| console.log(`Region in Plugin init is ${region}`) | ||
| console.log(region) | ||
| await app.decorate("s3Manager", new S3Manager(region)); |
There was a problem hiding this comment.
I think we can directly pass all options to the S3Manager constructor, assuming we will be adding more options in the future.
| await app.decorate("s3Manager", new S3Manager(region)); | |
| await app.decorate("s3Manager", new S3Manager(options)); |
| @@ -41,6 +42,8 @@ const app = async ({ | |||
| port: settings.MongoDbPort, | |||
| }, | |||
| }); | |||
| console.log(settings.StreamFilesS3Region) | |||
| await server.register(S3Manager, settings.StreamFilesS3Region); | |||
There was a problem hiding this comment.
Then we will be calling this plugin registration with an option object:
| await server.register(S3Manager, settings.StreamFilesS3Region); | |
| await server.register(S3Manager, {region: settings.StreamFilesS3Region}); |
haiqi96
changed the title
haiqi96
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/log-viewer-webui/server/src/S3Manager.js (1)
25-31: Handle potential undefinedregionparameterIn the constructor, consider handling the case where
regionmight beundefined. Currently, ifregionisundefined,null !== regionevaluates totrue, which could lead to unintended behaviour when initializingS3Clientwith anundefinedregion.Apply this diff to ensure
regionis notundefined:constructor ({region}) { - if (null !== region) { + if (null !== region && undefined !== region) { this.#s3Client = new S3Client({ region: region, }); } }Alternatively, you can use a more concise check:
constructor ({region}) { - if (null !== region) { + if (region) { this.#s3Client = new S3Client({ region: region, }); } }components/log-viewer-webui/server/src/routes/query.js (2)
11-13: Improve JSDoc parameter type annotationsThe use of the union type
|in the JSDoc comments may not accurately represent the structure of thefastifyobject, which includes all listed properties. Consider using the intersection type&to indicate thatfastifycontains all specified properties.Apply this diff to adjust the JSDoc annotations:
/** * @param {object} props - * @param {import("fastify").FastifyInstance | - * {dbManager: DbManager} | - * {s3Manager: S3Manager}} props.fastify + * @param {import("fastify").FastifyInstance & + * {dbManager: DbManager} & + * {s3Manager: S3Manager}} props.fastify * @param {EXTRACT_JOB_TYPES} props.jobType * @param {number} props.logEventIdx * @param {string} props.streamId
59-61: Adjust JSDoc annotations for accurate typingSimilar to the earlier comment, update the parameter type annotations to reflect that
fastifyincludes all the specified properties.Apply this diff:
/** * @param {import("fastify").FastifyInstance & * {dbManager: DbManager} & * {s3Manager: S3Manager}} fastify * @param {import("fastify").FastifyPluginOptions} options * @return {Promise<void>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/log-viewer-webui/server/src/S3Manager.js(4 hunks)components/log-viewer-webui/server/src/app.js(2 hunks)components/log-viewer-webui/server/src/routes/query.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/log-viewer-webui/server/src/app.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/query.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/S3Manager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/log-viewer-webui/server/src/S3Manager.js (5)
19-19: Initialization of private field#s3ClientProperly initializing
#s3Clienttonullenhances code safety by ensuring the variable has a known state before use.
22-23: Clarify constructor parameter documentationThe JSDoc comments accurately reflect the updated constructor parameters, improving code readability and maintainability.
33-34:isEnabledmethod implementation looks goodThe
isEnabledmethod correctly checks the initialization state of#s3Client, enhancing the class's usability.
45-47: Usage offalse === <expression>complies with coding guidelinesThe condition
if (false === this.isEnabled())aligns with the project's coding standards.
68-70: Proper use of Fastify plugin to registerS3ManagerExporting
S3Manageras a Fastify plugin and decorating it onto the app instance improves modularity and follows best practices.components/log-viewer-webui/server/src/app.js (2)
9-9: ImportingS3ManagerThe import statement correctly brings in the
S3Managerplugin for registration.
45-45: RegisteringS3Managerwith appropriate optionsRegistering
S3Managerwith theregionoption ensures proper configuration. This aligns with previous review suggestions and enhances code clarity.
| if (fastify.s3Manager.isEnabled()) { | ||
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | ||
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | ||
| ); | ||
| } else { | ||
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
There was a problem hiding this comment.
Ensure s3Manager is defined before use
Accessing fastify.s3Manager without checking if it is defined could lead to runtime errors, especially in environments where s3Manager is not registered (e.g., when NODE_ENV === "test"). Modify the condition to check for the existence of s3Manager before calling isEnabled().
Apply this diff to prevent potential null reference errors:
-if (fastify.s3Manager.isEnabled()) {
+if (fastify.s3Manager && fastify.s3Manager.isEnabled()) {
streamMetadata.path = await fastify.s3Manager.getPreSignedUrl(
`s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}`
);
} else {
streamMetadata.path = `/streams/${streamMetadata.path}`;
}📝 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.
| if (fastify.s3Manager.isEnabled()) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; | |
| if (fastify.s3Manager && fastify.s3Manager.isEnabled()) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( | |
| `s3://${settings.StreamFilesS3PathPrefix}${streamMetadata.path}` | |
| ); | |
| } else { | |
| streamMetadata.path = `/streams/${streamMetadata.path}`; |
Description
This PR refactors the S3Manager into a fastify plugin, which allows as to avoid initializing S3 Manager as a global variable.
Validation performed
Manually tested FS and S3 stream viewing.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates provide more flexible and reliable file management when working with S3 storage.