Skip to content
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

Implement a better API with tenancy-based configuration and storage #13

Open
wants to merge 20 commits into
base: staging
Choose a base branch
from

Conversation

benrobson
Copy link
Member

@benrobson benrobson commented Jan 14, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Added database configuration with MySQL support
    • Introduced new API routes for tenant, devotion, and verse of the day management
    • Implemented token-based API authentication
  • Configuration

    • Updated environment variables for database and API access
    • Increased Node.js version requirement to 20.0.0
  • Database

    • Created new database schema with tables for tenants, configurations, prayers, devotions, and verse of the day tracking
  • Security

    • Added API token verification for route access

Copy link

coderabbitai bot commented Jan 14, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a comprehensive update to the application's infrastructure, focusing on database integration, API route management, and environment configuration. The changes include setting up a new MySQL database connection, creating API routes for tenants, devotions, and verse of the day (VOTD), implementing token verification, and updating the project's dependencies and configuration. The modifications enhance the application's modularity, security, and database interaction capabilities.

Changes

File Change Summary
.env.example Added database and API configuration variables
CODEOWNERS Added @benrobson as code owner for all repository files
api/common.js Introduced utility functions for API request handling and validation
api/routes/devotion.js Added API routes for checking and adding devotion entries
api/routes/index.js Created a centralized route registration module
api/routes/tenant.js Implemented tenant management API routes
api/routes/verifyToken.js Added token verification middleware
api/routes/votd.js Created API routes for verse of the day operations
app.js Updated routing and HTML entity handling
controllers/databaseController.js Implemented MySQL database connection pool
dbinit.sql Created database schema with tables for tenants, configurations, and entries
package.json Updated Node.js version and added MySQL dependency
routes/index.js Modified import statements

Sequence Diagram

sequenceDiagram
    participant Client
    participant APIRoute
    participant VerifyToken
    participant Database
    
    Client->>VerifyToken: Send request with API token
    VerifyToken->>APIRoute: Validate token
    alt Token Valid
        APIRoute->>Database: Perform database operation
        Database-->>APIRoute: Return result
        APIRoute-->>Client: Send response
    else Token Invalid
        VerifyToken-->>Client: Send authentication error
    end
Loading

Poem

🐰 A Rabbit's Database Dance 🎲

With MySQL pools and routes so neat,
Our code now dances to a rhythm sweet
Tenants, devotions, tokens tight
Our app now shines so crisp and bright!

Hop, hop, hooray! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@benrobson
Copy link
Member Author

@coderabbitai review

Copy link

coderabbitai bot commented Jan 14, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (8)
api/routes/tenant.js (1)

142-148: Refactor to allow updating multiple fields simultaneously

The current implementation updates only one field (votd_channel or devotion_channel) per request. Allowing multiple fields to be updated at once improves API usability.

Consider this refactor:

-// Determine the field and value to update
-let fieldToUpdate = "";
-let valueToUpdate = null;

-if (votdChannel) {
-  fieldToUpdate = "votd_channel";
-  valueToUpdate = votdChannel;
-} else if (devotionChannel) {
-  fieldToUpdate = "devotion_channel";
-  valueToUpdate = devotionChannel;
-}

-if (fieldToUpdate) {
+// Build an object of fields to update
+let fieldsToUpdate = {};
+if (votdChannel) {
+  fieldsToUpdate.votd_channel = votdChannel;
+}
+if (devotionChannel) {
+  fieldsToUpdate.devotion_channel = devotionChannel;
+}

+if (Object.keys(fieldsToUpdate).length > 0) {
   // Proceed with the update

Then construct the SQL query dynamically to update all provided fields.

api/routes/verifyToken.js (2)

3-3: Use const instead of var for variable declaration

Using const for variables that are not reassigned enhances code clarity and prevents accidental reassignments.

Apply this diff:

-var token = req.headers["x-access-token"];
+const token = req.headers["x-access-token"];

2-23: Consider implementing a scalable authentication mechanism

Using a static API key from the environment is suitable for simple cases but doesn't scale well for multiple users or enhanced security requirements.

Consider adopting a robust authentication solution like JWT, OAuth 2.0, or API keys associated with user accounts to improve security and scalability.

app.js (1)

28-33: Consider route order and error handling in API registration

The API routes registration could be improved with better error handling and explicit route prefixing.

Consider this enhanced implementation:

await app.register(async (instance) => {
  instance.addHook("preValidation", verifyToken);
  instance.prefix('/api'); // Explicit API prefix
  return apiRoutes(instance, db);
}, { prefix: '/api' }).catch(err => {
  app.log.error('Failed to register API routes:', err);
  process.exit(1);
});
api/common.js (1)

14-31: Add proper HTTP status codes to error responses

The error responses should include appropriate HTTP status codes for better REST API compliance.

 if (!body || !(field in body))
   return res.send({
+    status: 400,
     success: false,
     message: `Body requires field '${field}'`,
   });

 if (body[field] === null)
   return res.send({
+    status: 400,
     success: false,
     message: `Field ${field} cannot be null`,
   });
package.json (1)

18-18: Use version range for Node engine requirement

Using an exact version is too restrictive and may cause unnecessary compatibility issues.

-    "node": "20.0.0"
+    "node": ">=20.0.0"
dbinit.sql (2)

4-8: Optimize tenant table schema

Consider the following improvements:

  1. Increase tenantId size for future scaling
  2. Add table and column comments
+-- Stores tenant information for multi-tenant system
 CREATE TABLE tenants (
-    tenantId VARCHAR(20) PRIMARY KEY,
-    tenantName VARCHAR(100) NOT NULL,
+    tenantId VARCHAR(36) PRIMARY KEY COMMENT 'Unique identifier for tenant (UUID)',
+    tenantName VARCHAR(100) NOT NULL COMMENT 'Display name of the tenant',
     createdAt TIMESTAMP DEFAULT CURRENT_TIMESTAMP
 );

18-43: Consider normalizing similar tables and adding indexes

The prayers, devotions, and votd tables have identical structure. Consider:

  1. Creating a base table with common fields
  2. Adding indexes for frequently queried columns
  3. Adding table and column comments

Example normalized structure:

-- Base table for all message types
CREATE TABLE messages (
    messageId VARCHAR(255) PRIMARY KEY,
    tenantId VARCHAR(36) NOT NULL,
    userId VARCHAR(20) NOT NULL,
    messageType ENUM('prayer', 'devotion', 'votd') NOT NULL,
    createdAt TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
    FOREIGN KEY (tenantId) REFERENCES tenants(tenantId) ON DELETE CASCADE,
    INDEX idx_tenant_type (tenantId, messageType),
    INDEX idx_user (userId)
) COMMENT 'Unified message storage for prayers, devotions, and verse of the day';
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f067bc2 and 846d3a5.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (13)
  • .env.example (1 hunks)
  • CODEOWNERS (1 hunks)
  • api/common.js (1 hunks)
  • api/routes/devotion.js (1 hunks)
  • api/routes/index.js (1 hunks)
  • api/routes/tenant.js (1 hunks)
  • api/routes/verifyToken.js (1 hunks)
  • api/routes/votd.js (1 hunks)
  • app.js (3 hunks)
  • controllers/databaseController.js (1 hunks)
  • dbinit.sql (1 hunks)
  • package.json (1 hunks)
  • routes/index.js (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • CODEOWNERS
  • routes/index.js
🔇 Additional comments (2)
api/routes/tenant.js (1)

66-71: Ensure proper response when no results are found

After correcting the destructuring, the check if (!results.length) will work as expected. Ensure that the response message accurately reflects the situation.

api/routes/index.js (1)

1-9: Well-organized route initialization

The route handlers are properly imported and registered. The modular approach enhances maintainability.

} catch (error) {
res.send({
success: false,
message: `${error}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid exposing detailed error messages to clients

Returning raw error messages can reveal sensitive information. It's better to send a generic message to the client and log the detailed error on the server.

Apply this diff to modify the error responses:

-        message: `${error}`,
+        message: 'An unexpected error occurred. Please try again later.',

Repeat this change for all error handlers.

Also applies to: 80-80, 126-126, 193-193

try {
// Get Tenant Configuration by specific ID
const dbQuery = `SELECT * FROM tenantConfiguration WHERE tenantId = ?`;
const results = await db.query(dbQuery, [tenantId]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Destructure query results to access data correctly

The db.query() method returns an array [results, fields]. Assigning it directly to results can lead to unexpected behavior when accessing results.length.

Apply this diff to destructure the returned array:

-const results = await db.query(dbQuery, [tenantId]);
+const [results, fields] = await db.query(dbQuery, [tenantId]);
📝 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.

Suggested change
const results = await db.query(dbQuery, [tenantId]);
const [results, fields] = await db.query(dbQuery, [tenantId]);

Comment on lines +153 to +155
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correctly handle existing configuration query results

Similar to earlier, destructure the result of db.query() to properly access existingConfig.

Apply this diff:

-const existingConfig = await db.query(
+const [existingConfig, fields] = await db.query(
   `SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
   [tenantId]
);
📝 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.

Suggested change
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);
const [existingConfig, fields] = await db.query(
`SELECT id FROM tenantConfiguration WHERE tenantId = ?`,
[tenantId]
);

try {
// Get Tenant by specific ID
if (tenantId) {
let dbQuery = `SELECT * FROM tenants WHERE tenantId=${tenantId};`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent SQL Injection by using parameterized queries

The current query constructs the SQL statement by directly embedding tenantId, which can lead to SQL injection vulnerabilities if tenantId is not properly sanitized.

Apply this diff to use a parameterized query:

-let dbQuery = `SELECT * FROM tenants WHERE tenantId=${tenantId};`;
+let dbQuery = `SELECT * FROM tenants WHERE tenantId = ?;`;
+const results = await getTenants(dbQuery, [tenantId]);

Update the getTenants function to accept parameters:

-async function getTenants(dbQuery) {
+async function getTenants(dbQuery, params = []) {
   const [results, fields] = await db.query(dbQuery, params);
   return results;
}

Committable suggestion skipped: line range outside the PR's diff.

// Token not included
return res.send({
success: false,
message: `lang.api.noToken`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace placeholder messages with meaningful error responses

The messages lang.api.noToken and lang.api.invalidToken appear to be placeholders. Clients will receive these as-is, which might be confusing.

Update the messages to provide clear feedback:

-      message: `lang.api.noToken`,
+      message: 'No access token provided.',
-      message: `lang.api.invalidToken`,
+      message: 'Invalid access token.',

Also applies to: 20-20

Comment on lines +11 to +31
try {
// Perform a SELECT query to check if the record exists
const [rows] = await db.query(
`SELECT 1 FROM votd WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
[tenantId, messageId, userId]
);

// If any row is returned, that means the record exists
const exists = rows.length > 0;

// Return a boolean indicating if the record exists
res.send({
success: true,
exists: exists,
});
} catch (error) {
res.send({
success: false,
message: `Error checking VOTD entry: ${error}`,
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and response structure

The error handling could be improved with:

  1. HTTP status codes
  2. Consistent error response structure
  3. SQL error sanitization
 try {
   const [rows] = await db.query(
     `SELECT 1 FROM votd WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
     [tenantId, messageId, userId]
   );

   const exists = rows.length > 0;

-  res.send({
+  res.status(200).send({
     success: true,
     exists: exists,
   });
 } catch (error) {
-  res.send({
+  // Avoid exposing SQL errors to clients
+  const safeError = error.code === 'ER_DUP_ENTRY' ? 
+    'Record already exists' : 'Internal server error';
+  res.status(500).send({
     success: false,
-    message: `Error checking VOTD entry: ${error}`,
+    error: safeError,
   });
 }
📝 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.

Suggested change
try {
// Perform a SELECT query to check if the record exists
const [rows] = await db.query(
`SELECT 1 FROM votd WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
[tenantId, messageId, userId]
);
// If any row is returned, that means the record exists
const exists = rows.length > 0;
// Return a boolean indicating if the record exists
res.send({
success: true,
exists: exists,
});
} catch (error) {
res.send({
success: false,
message: `Error checking VOTD entry: ${error}`,
});
}
try {
// Perform a SELECT query to check if the record exists
const [rows] = await db.query(
`SELECT 1 FROM votd WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
[tenantId, messageId, userId]
);
// If any row is returned, that means the record exists
const exists = rows.length > 0;
// Return a boolean indicating if the record exists
res.status(200).send({
success: true,
exists: exists,
});
} catch (error) {
// Avoid exposing SQL errors to clients
const safeError = error.code === 'ER_DUP_ENTRY' ?
'Record already exists' : 'Internal server error';
res.status(500).send({
success: false,
error: safeError,
});
}

Comment on lines +34 to +56
// Add votd endpoint
app.post(baseEndpoint + "/add", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const messageId = required(req.body, "messageId", res);
const userId = required(req.body, "userId", res);

try {
await db.query(
`INSERT INTO votd (tenantId, messageId, userId) VALUES (?, ?, ?)`,
[tenantId, messageId, userId]
);

res.send({
success: true,
content: `VOTD entry created for tenant ${tenantId} with message ID ${messageId}`,
});
} catch (error) {
res.send({
success: false,
message: `Error creating devotion entry: ${error}`,
});
}
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and unique constraint handling

The POST endpoint needs additional validation and proper unique constraint handling.

 app.post(baseEndpoint + "/add", async function (req, res) {
   const tenantId = required(req.body, "tenantId", res);
   const messageId = required(req.body, "messageId", res);
   const userId = required(req.body, "userId", res);

+  // Validate input
+  if (!Number.isInteger(Number(tenantId)) || !Number.isInteger(Number(messageId))) {
+    return res.status(400).send({
+      success: false,
+      error: "Invalid tenantId or messageId format"
+    });
+  }

   try {
     await db.query(
       `INSERT INTO votd (tenantId, messageId, userId) VALUES (?, ?, ?)`,
       [tenantId, messageId, userId]
     );

-    res.send({
+    res.status(201).send({
       success: true,
       content: `VOTD entry created for tenant ${tenantId} with message ID ${messageId}`,
     });
   } catch (error) {
+    if (error.code === 'ER_DUP_ENTRY') {
+      return res.status(409).send({
+        success: false,
+        error: 'Record already exists'
+      });
+    }
-    res.send({
+    res.status(500).send({
       success: false,
-      message: `Error creating devotion entry: ${error}`,
+      error: 'Internal server error'
     });
   }
 });
📝 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.

Suggested change
// Add votd endpoint
app.post(baseEndpoint + "/add", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const messageId = required(req.body, "messageId", res);
const userId = required(req.body, "userId", res);
try {
await db.query(
`INSERT INTO votd (tenantId, messageId, userId) VALUES (?, ?, ?)`,
[tenantId, messageId, userId]
);
res.send({
success: true,
content: `VOTD entry created for tenant ${tenantId} with message ID ${messageId}`,
});
} catch (error) {
res.send({
success: false,
message: `Error creating devotion entry: ${error}`,
});
}
});
// Add votd endpoint
app.post(baseEndpoint + "/add", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const messageId = required(req.body, "messageId", res);
const userId = required(req.body, "userId", res);
// Validate input
if (!Number.isInteger(Number(tenantId)) || !Number.isInteger(Number(messageId))) {
return res.status(400).send({
success: false,
error: "Invalid tenantId or messageId format"
});
}
try {
await db.query(
`INSERT INTO votd (tenantId, messageId, userId) VALUES (?, ?, ?)`,
[tenantId, messageId, userId]
);
res.status(201).send({
success: true,
content: `VOTD entry created for tenant ${tenantId} with message ID ${messageId}`,
});
} catch (error) {
if (error.code === 'ER_DUP_ENTRY') {
return res.status(409).send({
success: false,
error: 'Record already exists'
});
}
res.status(500).send({
success: false,
error: 'Internal server error'
});
}
});

Comment on lines +1 to +57
import { required, optional } from "../common.js";

export default function devotionApiRoute(app, db) {
const baseEndpoint = "/api/devotion";

app.get(baseEndpoint + "/check", async function (req, res) {
const tenantId = required(req.query, "tenantId", res);
const messageId = required(req.query, "messageId", res);
const userId = required(req.query, "userId", res);

try {
// Perform a SELECT query to check if the record exists
const [rows] = await db.query(
`SELECT 1 FROM devotions WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
[tenantId, messageId, userId]
);

// If any row is returned, that means the record exists
const exists = rows.length > 0;

// Return a boolean indicating if the record exists
res.send({
success: true,
exists: exists,
});
} catch (error) {
res.send({
success: false,
message: `Error checking Devotion entry: ${error}`,
});
}
});

// Create devotion endpoint
app.post(baseEndpoint + "/add", async function (req, res) {
const tenantId = required(req.body, "tenantId", res);
const messageId = required(req.body, "messageId", res);
const userId = required(req.body, "userId", res);

try {
await db.query(
`INSERT INTO devotions (tenantId, messageId, userId) VALUES (?, ?, ?)`,
[tenantId, messageId, userId]
);

res.send({
success: true,
content: `Devotion entry created for tenant ${tenantId} with message ID ${messageId}`,
});
} catch (error) {
res.send({
success: false,
message: `Error creating devotion entry: ${error}`,
});
}
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Extract common API functionality to reduce duplication

The code is nearly identical to votd.js. Consider creating a reusable factory function for these endpoints.

Create a new file api/common/recordEndpoints.js:

export function createRecordEndpoints(options) {
  const { app, db, tableName, baseEndpoint } = options;

  const checkRecord = async (req, res) => {
    const tenantId = required(req.query, "tenantId", res);
    const messageId = required(req.query, "messageId", res);
    const userId = required(req.query, "userId", res);

    if (!Number.isInteger(Number(tenantId)) || !Number.isInteger(Number(messageId))) {
      return res.status(400).send({
        success: false,
        error: "Invalid tenantId or messageId format"
      });
    }

    try {
      const [rows] = await db.query(
        `SELECT 1 FROM ${tableName} WHERE tenantId = ? AND messageId = ? AND userId = ? LIMIT 1`,
        [tenantId, messageId, userId]
      );

      res.status(200).send({
        success: true,
        exists: rows.length > 0,
      });
    } catch (error) {
      res.status(500).send({
        success: false,
        error: 'Internal server error'
      });
    }
  };

  const addRecord = async (req, res) => {
    // Similar implementation for POST
  };

  app.get(`${baseEndpoint}/check`, checkRecord);
  app.post(`${baseEndpoint}/add`, addRecord);
}

Then simplify both route files:

// devotion.js
import { createRecordEndpoints } from '../common/recordEndpoints.js';

export default function devotionApiRoute(app, db) {
  createRecordEndpoints({
    app,
    db,
    tableName: 'devotions',
    baseEndpoint: '/api/devotion'
  });
}

Comment on lines +61 to +89
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});

const data = await response.json();

console.log(data);

if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}

if (!data.success) {
return res.redirect(failureRedirectURL);
}

return console.log(data);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix critical issues in postAPIRequest function

Several issues need to be addressed:

  1. Missing error handling for fetch failures
  2. Logging sensitive data to console
  3. Undefined setBannerCookie function
  4. Incorrect return value
 export async function postAPIRequest(
   postURL,
   apiPostBody,
   failureRedirectURL,
   res
 ) {
-  const response = await fetch(postURL, {
-    method: "POST",
-    body: JSON.stringify(apiPostBody),
-    headers: {
-      "Content-Type": "application/json",
-      "x-access-token": process.env.APIKEY,
-    },
-  });
+  try {
+    const response = await fetch(postURL, {
+      method: "POST",
+      body: JSON.stringify(apiPostBody),
+      headers: {
+        "Content-Type": "application/json",
+        "x-access-token": process.env.APIKEY,
+      },
+    });
 
-  const data = await response.json();
+    if (!response.ok) {
+      throw new Error(`HTTP error! status: ${response.status}`);
+    }
 
-  console.log(data);
+    const data = await response.json();
 
-  if (data.alertType) {
-    setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
-  }
+    // TODO: Import or implement setBannerCookie function
+    if (data.alertType) {
+      setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
+    }
 
-  if (!data.success) {
-    return res.redirect(failureRedirectURL);
-  }
+    if (!data.success) {
+      return res.redirect(failureRedirectURL);
+    }
 
-  return console.log(data);
+    return data;
+  } catch (error) {
+    console.error('API request failed:', error.message);
+    return res.redirect(failureRedirectURL);
+  }
 }
📝 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.

Suggested change
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});
const data = await response.json();
console.log(data);
if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}
if (!data.success) {
return res.redirect(failureRedirectURL);
}
return console.log(data);
}
export async function postAPIRequest(
postURL,
apiPostBody,
failureRedirectURL,
res
) {
try {
const response = await fetch(postURL, {
method: "POST",
body: JSON.stringify(apiPostBody),
headers: {
"Content-Type": "application/json",
"x-access-token": process.env.APIKEY,
},
});
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
const data = await response.json();
// TODO: Import or implement setBannerCookie function
if (data.alertType) {
setBannerCookie(`${data.alertType}`, `${data.alertContent}`, res);
}
if (!data.success) {
return res.redirect(failureRedirectURL);
}
return data;
} catch (error) {
console.error('API request failed:', error.message);
return res.redirect(failureRedirectURL);
}
}

Comment on lines +3 to +9

DBHOST=localhost
DBPORT=3306
DBUSER=root
DBPASSWORD=root
DBNAME=devoteMe
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve security in environment variable examples

  1. Using root credentials in examples is not recommended
  2. API key example is too simple
  3. Missing documentation for required formats
-APIKEY=KEY
+# Required: Strong API key for authentication
+APIKEY=<STRONG_API_KEY_HERE>
 
-DBHOST=localhost
-DBPORT=3306
-DBUSER=root
-DBPASSWORD=root
-DBNAME=devoteMe
+# Database Configuration
+DBHOST=localhost
+DBPORT=3306
+# Avoid using root credentials in production
+DBUSER=app_user
+DBPASSWORD=<STRONG_PASSWORD_HERE>
+DBNAME=devoteMe
📝 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.

Suggested change
APIKEY=KEY
DBHOST=localhost
DBPORT=3306
DBUSER=root
DBPASSWORD=root
DBNAME=devoteMe
# Required: Strong API key for authentication
APIKEY=<STRONG_API_KEY_HERE>
# Database Configuration
DBHOST=localhost
DBPORT=3306
# Avoid using root credentials in production
DBUSER=app_user
DBPASSWORD=<STRONG_PASSWORD_HERE>
DBNAME=devoteMe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant