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.

Sign up for GitHub

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

Jump to bottom

Aggiunto endpoint API per generare il curriculum #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Aggiunto endpoint API per generare il curriculum #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions apps/registry/lib/renderResume.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { readFileSync } from 'fs';
import { join } from 'path';
import Handlebars from 'handlebars';
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using asynchronous file reading

The function is declared as asynchronous, but it uses readFileSync which is a blocking operation. Consider using the asynchronous version readFile from the fs/promises module to maintain consistency and prevent potential performance issues.

Here's a suggested change:

-import { readFileSync } from 'fs';
+import { readFile } from 'fs/promises';
import { join } from 'path';
import Handlebars from 'handlebars';
📝 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
import { readFileSync } from 'fs';
import { join } from 'path';
import Handlebars from 'handlebars';
import { readFile } from 'fs/promises';
import { join } from 'path';
import Handlebars from 'handlebars';


export async function renderResume(resumeData, theme) {
const themePath = join(process.cwd(), '..', '..', 'themes', theme);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use a more robust method for theme path construction

The current path construction method uses multiple .. to navigate the directory structure. This approach can be fragile if the directory structure changes. Consider using a configuration file or environment variable to store the base path to the themes directory.

Here's a suggested approach:

import { config } from '../config'; // Assume we have a config file

// In the function
const themePath = join(config.themesBasePath, theme);

const templateContent = readFileSync(join(themePath, 'resume.hbs'), 'utf8');
const template = Handlebars.compile(templateContent);

return template(resumeData);
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and input validation

The current implementation lacks error handling for file reading and template compilation. It also doesn't validate the inputs. This could lead to unhandled exceptions and potential security issues.

Consider adding the following improvements:

  1. Validate inputs:

    if (typeof theme !== 'string' || !theme.trim()) {
  throw new Error('Invalid theme specified');
}
if (typeof resumeData !== 'object' || resumeData === null) {
  throw new Error('Invalid resume data');
}

  2. Add error handling:

    try {
  const templateContent = await readFile(join(themePath, 'resume.hbs'), 'utf8');
  const template = Handlebars.compile(templateContent);
  return template(resumeData);
} catch (error) {
  if (error.code === 'ENOENT') {
    throw new Error(`Theme '${theme}' not found`);
  }
  throw new Error(`Error rendering resume: ${error.message}`);
}

These changes will make the function more robust and easier to debug.

}
26 changes: 26 additions & 0 deletions apps/registry/pages/api/generate-resume.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { renderResume } from '../../lib/renderResume';

export default async function handler(req, res) {
if (req.method !== 'POST') {
return res.status(405).json({ error: 'Metodo non consentito' });
}

try {
const { resumeData, theme = 'elegant' } = req.body;

if (!resumeData) {
return res
.status(400)
.json({ error: 'I dati del curriculum sono obbligatori' });
}
Comment on lines +9 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance request body validation.

While the current validation checks for the presence of resumeData, it could be more robust. Consider the following improvements:

  1. Validate the structure of resumeData to ensure it contains all necessary fields.
  2. Validate the theme value against a list of allowed themes.
  3. Use a validation library like Joi or Yup for more comprehensive input validation.

Example implementation:

const allowedThemes = ['elegant', 'modern', 'classic'];
const { resumeData, theme = 'elegant' } = req.body;

if (!resumeData || typeof resumeData !== 'object') {
  return res.status(400).json({ error: 'I dati del curriculum sono obbligatori e devono essere un oggetto' });
}

if (!allowedThemes.includes(theme)) {
  return res.status(400).json({ error: 'Tema non valido' });
}

// Additional validation for resumeData structure could be added here


const html = await renderResume(resumeData, theme);
const encodedHtml = Buffer.from(html).toString('base64');
const url = `data:text/html;base64,${encodedHtml}`;

res.status(200).json({ url });
} catch (error) {
console.error(error);
res.status(500).json({ error: 'Errore interno del server' });
}
Comment on lines +22 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling and logging.

While the current error handling catches errors, it could be improved:

  1. Use a logging library (e.g., Winston, Pino) for structured logging and log rotation.
  2. Consider adding more context to the logged error, such as request details (excluding sensitive information).
  3. In non-production environments, consider returning more detailed error information to aid in debugging.

Example implementation:

import logger from '../../lib/logger';  // Assume this is your logging setup

// In the catch block:
logger.error('Error generating resume', {
  error: error.message,
  stack: process.env.NODE_ENV !== 'production' ? error.stack : undefined,
  requestId: req.headers['x-request-id'],  // Assuming you're using request IDs
});

const errorResponse = process.env.NODE_ENV === 'production'
  ? { error: 'Errore interno del server' }
  : { error: error.message, stack: error.stack };

res.status(500).json(errorResponse);

}
Loading
Loading