Optimize Booking Retrieval in Cron Job#277
Conversation
- Introduced `getBookings` helper in `helpers/booking.helper.js` for batch fetching of bookings by type and IDs. - Refactored `getUnpaidOnlineBookingsAndTransactions` in `cron.js` to use the new batch fetching logic. - Added unit tests in `tests/optimize_booking_retrieval.test.js` verifying the optimization. - Resolved N+1 problem by replacing individual queries inside a loop with a single query per booking type. Co-authored-by: VitraagVigyaan <211280194+VitraagVigyaan@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @VitraagVigyaan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the performance of the cron job responsible for processing unpaid online bookings. By identifying and resolving an N+1 query issue, the system now efficiently retrieves bookings by grouping them by type and fetching them in batches. This change reduces the number of database calls, leading to faster execution and reduced load on the database. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the N+1 query problem by optimizing the booking retrieval process in the cron job. The introduction of the getBookings helper for batch retrieval is a solid approach. The changes are well-implemented and include relevant unit tests. I have a couple of suggestions to further improve code maintainability and follow testing best practices, but overall, this is a great optimization.
| export async function getBookings(bookingType, bookingids) { | ||
| var bookings = []; | ||
|
|
||
| switch (bookingType) { | ||
| case TYPE_ROOM: | ||
| bookings = await RoomBooking.findAll({ | ||
| where: { bookingid: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| case TYPE_FLAT: | ||
| bookings = await FlatBooking.findAll({ | ||
| where: { bookingid: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| case TYPE_ADHYAYAN: | ||
| bookings = await ShibirBookingDb.findAll({ | ||
| where: { bookingid: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| case TYPE_FOOD: | ||
| bookings = await FoodDb.findAll({ | ||
| where: { id: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| case TYPE_TRAVEL: | ||
| bookings = await TravelDb.findAll({ | ||
| where: { bookingid: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| case TYPE_UTSAV: | ||
| bookings = await UtsavBooking.findAll({ | ||
| where: { bookingid: bookingids } | ||
| }); | ||
| break; | ||
|
|
||
| default: | ||
| throw new ApiError(400, `${ERR_INVALID_BOOKING_TYPE}: ${bookingType}`); | ||
| } | ||
|
|
||
| return bookings; | ||
| } |
There was a problem hiding this comment.
The getBookings function can be made more concise and easier to maintain by using a map to associate booking types with their corresponding models and primary key fields. This avoids the repetitive switch statement, reduces code duplication, and makes it simpler to add or modify booking types in the future.
export async function getBookings(bookingType, bookingids) {
const modelMapping = {
[TYPE_ROOM]: { model: RoomBooking, key: 'bookingid' },
[TYPE_FLAT]: { model: FlatBooking, key: 'bookingid' },
[TYPE_ADHYAYAN]: { model: ShibirBookingDb, key: 'bookingid' },
[TYPE_FOOD]: { model: FoodDb, key: 'id' },
[TYPE_TRAVEL]: { model: TravelDb, key: 'bookingid' },
[TYPE_UTSAV]: { model: UtsavBooking, key: 'bookingid' }
};
const mapping = modelMapping[bookingType];
if (!mapping) {
throw new ApiError(400, `${ERR_INVALID_BOOKING_TYPE}: ${bookingType}`);
}
const { model, key } = mapping;
return model.findAll({
where: { [key]: bookingids }
});
}
There was a problem hiding this comment.
I've updated the getBookings function to use let/const for better adherence to modern JavaScript standards. I also added an early return for empty bookingids to make the function more robust.
| it('should throw error for invalid booking type', async () => { | ||
| try { | ||
| await getBookings('INVALID', ['id1']); | ||
| expect(true).toBe(false); // Should not reach here | ||
| } catch (e) { | ||
| expect(e.message).toContain('Invalid booking type'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test for checking exceptions can be simplified by using the expect().rejects.toThrow() assertion provided by the bun:test framework. This makes the test more readable and idiomatic, avoiding the manual try/catch block.
it('should throw error for invalid booking type', async () => {
await expect(getBookings('INVALID', ['id1'])).rejects.toThrow(
'Invalid booking type'
);
});There was a problem hiding this comment.
I've added these tests using Bun's test runner to verify the batch fetching logic and error handling for invalid booking types. The tests confirm that getBookings correctly maps booking types to their respective models and uses the appropriate field for IDs (e.g., id for food bookings and bookingid for others).
- Implemented batch fetching for bookings in `helpers/booking.helper.js` to solve N+1 query problem. - Refactored cron job in `cron.js` to use the new batch retrieval logic. - Added comprehensive unit tests in `tests/optimize_booking_retrieval.test.js` using Bun. - Verified logic via automated tests. Co-authored-by: VitraagVigyaan <211280194+VitraagVigyaan@users.noreply.github.com>
- Switched to using `Set` for unique booking IDs to prevent redundant queries. - Updated `getBookings` to use `let`/`const` and added robustness check for empty IDs. - Used `Object.entries()` for cleaner iteration over booking groups. - Updated unit tests with additional cases. Co-authored-by: VitraagVigyaan <211280194+VitraagVigyaan@users.noreply.github.com>
1 participant
The current implementation was fetching bookings one by one inside a loop in the cron job, leading to an N+1 query problem. This PR optimizes the retrieval process by grouping bookings by type and fetching them in batches using a single query per type.
Key changes:
getBookingshelper function for batch retrieval.PR created automatically by Jules for task 1743021283839662414 started by @VitraagVigyaan