fix(billing): address review feedback on Subscription model

- Validate organization as ObjectId format in Zod schema
- Normalize empty Stripe IDs to undefined to prevent sparse index issues
- Fix id/\_id inconsistency in update/remove repository methods
- Add defensive guard in findByStripeCustomerId for falsy input
- Update JSDoc @return to @returns and add Promise return types
- Add unit tests for ObjectId validation and Stripe ID normalization

Author: PierreBrisorgueil

modules/billing/models/subscription.model.mongoose.js

@@ -50,8 +50,8 @@ const SubscriptionMongoose = new Schema(
 );
 
 /**
- * @desc Function to add id (+ _id) to all objects
- * @return {String} hex string of the ObjectId
+ * Returns the hex string representation of the document ObjectId.
+ * @returns {string} Hex string of the ObjectId.
  */
 function addID() {
   return this._id.toHexString();

modules/billing/models/subscription.schema.js

@@ -6,10 +6,23 @@ import { z } from 'zod';
 /**
  *  Data Schema
  */
+const objectIdRegex = /^[a-f\d]{24}$/i;
+
 const Subscription = z.object({
-  organization: z.string().trim().min(1),
-  stripeCustomerId: z.string().trim().optional(),
-  stripeSubscriptionId: z.string().trim().optional(),
+  organization: z
+    .string()
+    .trim()
+    .regex(objectIdRegex, 'organization must be a valid ObjectId'),
+  stripeCustomerId: z
+    .string()
+    .trim()
+    .optional()
+    .transform((val) => (val === '' ? undefined : val)),
+  stripeSubscriptionId: z
+    .string()
+    .trim()
+    .optional()
+    .transform((val) => (val === '' ? undefined : val)),
   plan: z.enum(['free', 'starter', 'pro']).default('free'),
   status: z.enum(['active', 'past_due', 'canceled', 'trialing', 'incomplete']).default('active'),
   currentPeriodEnd: z.coerce.date().nullable().optional(),

modules/billing/repositories/subscription.repository.js

@@ -12,27 +12,34 @@ const defaultPopulate = [
   },
 ];
 
+/**
+ * Resolves the identifier from a subscription object or raw id.
+ * @param {Object|string} value - The subscription object or id string.
+ * @returns {string|undefined} The resolved id.
+ */
+const resolveId = (value) => value?._id || value?.id || value;
+
 /**
  * @function list
  * @description Data access operation to fetch all subscriptions from the database with an optional filter.
  * @param {Object} [filter] - Optional filter to apply to the query.
- * @returns {Array} An array of subscriptions.
+ * @returns {Promise<Array>} A promise resolving to an array of subscriptions.
  */
 const list = (filter) => Subscription.find(filter).populate(defaultPopulate).sort('-createdAt').exec();
 
 /**
  * @function create
  * @description Data access operation to create a new subscription in the database.
  * @param {Object} subscription - The subscription object to create.
- * @returns {Object} The created subscription.
+ * @returns {Promise<Object>} A promise resolving to the created subscription.
  */
 const create = (subscription) => new Subscription(subscription).save().then((doc) => doc.populate(defaultPopulate));
 
 /**
  * @function get
  * @description Data access operation to fetch a single subscription by its ID.
  * @param {String} id - The ID of the subscription to fetch.
- * @returns {Object} The retrieved subscription or null if the ID is not valid.
+ * @returns {Promise<Object|null>} A promise resolving to the retrieved subscription or null if the ID is not valid.
  */
 const get = (id) => {
   if (!mongoose.Types.ObjectId.isValid(id)) return null;
@@ -43,11 +50,14 @@ const get = (id) => {
  * @function update
  * @description Data access operation to update an existing subscription in the database.
  * @param {Object} subscription - The subscription object containing the updated details.
- * @returns {Object} The updated subscription.
+ * @returns {Promise<Object>} A promise resolving to the updated subscription.
  */
 const update = (subscription) => {
-  if (subscription._id) {
-    return Subscription.findByIdAndUpdate(subscription._id, subscription, { returnDocument: 'after', runValidators: true })
+  const id = resolveId(subscription);
+  if (id && mongoose.Types.ObjectId.isValid(id)) {
+    // eslint-disable-next-line no-unused-vars
+    const { _id, id: _virtualId, ...payload } = subscription;
+    return Subscription.findByIdAndUpdate(id, payload, { returnDocument: 'after', runValidators: true })
       .populate(defaultPopulate)
       .exec();
   }
@@ -58,15 +68,19 @@ const update = (subscription) => {
  * @function remove
  * @description Data access operation to delete a single subscription by its ID.
  * @param {Object} subscription - The subscription object to delete.
- * @returns {Object} A confirmation of the deletion.
+ * @returns {Promise<Object|null>} A promise resolving to a confirmation of the deletion or null if invalid.
  */
-const remove = (subscription) => Subscription.deleteOne({ _id: subscription.id }).exec();
+const remove = (subscription) => {
+  const id = resolveId(subscription);
+  if (!id || !mongoose.Types.ObjectId.isValid(id)) return null;
+  return Subscription.deleteOne({ _id: id }).exec();
+};
 
 /**
  * @function findByOrganization
  * @description Data access operation to fetch a subscription by organization ID.
  * @param {String} organizationId - The organization ID.
- * @returns {Object} The retrieved subscription or null.
+ * @returns {Promise<Object|null>} A promise resolving to the retrieved subscription or null.
  */
 const findByOrganization = (organizationId) => {
   if (!mongoose.Types.ObjectId.isValid(organizationId)) return null;
@@ -77,10 +91,12 @@ const findByOrganization = (organizationId) => {
  * @function findByStripeCustomerId
  * @description Data access operation to fetch a subscription by Stripe customer ID.
  * @param {String} stripeCustomerId - The Stripe customer ID.
- * @returns {Object} The retrieved subscription or null.
+ * @returns {Promise<Object|null>} A promise resolving to the retrieved subscription or null.
  */
-const findByStripeCustomerId = (stripeCustomerId) =>
-  Subscription.findOne({ stripeCustomerId }).populate(defaultPopulate).exec();
+const findByStripeCustomerId = (stripeCustomerId) => {
+  if (!stripeCustomerId || !stripeCustomerId.trim()) return null;
+  return Subscription.findOne({ stripeCustomerId }).populate(defaultPopulate).exec();
+};
 
 export default {
   list,

modules/billing/tests/billing.unit.tests.js

@@ -121,5 +121,57 @@ describe('Billing unit tests:', () => {
       expect(result.data.plan).toBe('starter');
       done();
     });
+
+    test('should reject invalid ObjectId for organization', (done) => {
+      subscription.organization = 'not-an-objectid';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeDefined();
+      done();
+    });
+
+    test('should accept valid ObjectId for organization', (done) => {
+      subscription.organization = '507f1f77bcf86cd799439011';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeFalsy();
+      done();
+    });
+
+    test('should normalize empty stripeCustomerId to undefined', (done) => {
+      subscription.stripeCustomerId = '';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeFalsy();
+      expect(result.data.stripeCustomerId).toBeUndefined();
+      done();
+    });
+
+    test('should normalize empty stripeSubscriptionId to undefined', (done) => {
+      subscription.stripeSubscriptionId = '';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeFalsy();
+      expect(result.data.stripeSubscriptionId).toBeUndefined();
+      done();
+    });
+
+    test('should preserve valid stripeCustomerId', (done) => {
+      subscription.stripeCustomerId = 'cus_abc123';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeFalsy();
+      expect(result.data.stripeCustomerId).toBe('cus_abc123');
+      done();
+    });
+
+    test('should preserve valid stripeSubscriptionId', (done) => {
+      subscription.stripeSubscriptionId = 'sub_xyz789';
+
+      const result = schema.Subscription.safeParse(subscription);
+      expect(result.error).toBeFalsy();
+      expect(result.data.stripeSubscriptionId).toBe('sub_xyz789');
+      done();
+    });
   });
 });