בירור | nodejs תקינות פונקציה

[A0533057932]

@עידו300

---

זה בהחלט שיפור. עברת משלב "גן חובה" לכיתה א'.
הכנסת try/catch/finally כמו שצריך, יש לך טרנזקציה, ויש ניסיון לייצר סכימה. כל הכבוד.

אבל...
הקוד הזה עדיין יקרוס (Crash), הוא מכיל באג לוגי חמור מול הדרישות המקוריות, והוא חושף אותך לסיכון SQL Injection ברמת התכנון.

בוא ננתח למה ה"שיפור" הזה עדיין לא עובר Code Review אצלי:

1. קריסה מובטחת (Runtime Error)

תסתכל על השורה הזו ב-createAdObject:

cities: validateResult.data.area || validateResult.data.city || ["all"]

אם המשתמש שלח area (שהוא מחרוזת, למשל "Tel Aviv"), אז createResult.cities יהיה שווה ל-"Tel Aviv".
ואז ב-addAdNEw אתה עושה:

citiesToInsert = createResult.cities.map(city => ...)

בום. לפונקציה map אין קיום על String (בצורה הזו). הקוד יזרוק TypeError והבקשה תיפול.
מתכנת רציני מוודא שתמיד חוזר מערך (Array), גם אם יש איבר אחד.

2. משתנה גלובלי דולף (Strict Mode Violation)

citiesToInsert = createResult.cities.map(...)

איפה ה-const? איפה ה-let?
במודולים של JS (שבהם export קיים), הקוד רץ אוטומטית ב-Strict Mode.
שימוש במשתנה ללא הצהרה יזרוק ReferenceError: citiesToInsert is not defined. השרת שלך נפל שוב.

3. התעלמות מהדרישות העסקיות (Business Logic Regression)

זוכר את הקוד המקורי?

• אם זה "מחפש" -> העמודה היא max_payment.
• אם זה "מפרסם" -> העמודה היא min_payment.

בקוד החדש החלטת על דעת עצמך:

...(validateResult.data.min_max_price && { 'min_max_price': ... })

אתה מנסה לדחוף לעמודה בשם min_max_price. העמודה הזו לא קיימת במסד הנתונים! ה-SQL יחזיר שגיאה: Unknown column 'min_max_price'.
שיפרת את המבנה, אבל הרסת את הלוגיקה. קוד יפה שלא עובד הוא חסר ערך.

4. פרצת אבטחה ב-insertRecord (SQL Injection via Design)

אתה כותב פונקציה גנרית יפה:

const columns = Object.keys(records[0]);
const sql = `INSERT INTO ${tableName} (${columns.join(', ')}) ...`

כרגע, במקרה, האובייקט שאתה שולח נוצר ידנית ב-createAdObject.
אבל הפונקציה insertRecord היא גנרית. מחר מתכנת זוטר ישתמש בה וישלח אובייקט req.body ישירות מהמשתמש.
אם המשתמש ישלח מפתח בשם id) VALUES (1); DROP TABLE ads; --, והפונקציה שלך משרשרת את המפתחות ישר ל-SQL... הלך ה-DB.
כלל ברזל: לעולם אל תסמוך על מפתחות של אובייקטים (Object Keys) כשמות עמודות ב-SQL ללא בדיקה מול רשימה מותרת (Allowlist) בתוך הפונקציה הגנרית.

5. צימוד (Coupling) בין שם המשתנה לשם העמודה

בסכמה שלך:

min_max_price: {validate: checkNumber, required: false},

אבל ב-DB ובקלט המקורי זה נקרא price.
היצרת ערבוב: הפונקציה שלך מצפה לקלט בשם min_max_price מהלקוח? או שהלקוח שולח price ואתה ממפה את זה? הקוד שלך מניח שהלקוח שולח min_max_price. שינית את ה-API בלי לשים לב.

---

התיקון (כדי שזה באמת יעבוד)

הנה איך הקוד צריך להיראות כדי לפתור את כל הבעיות הנ"ל, תוך שמירה על המבנה הנקי שניסית ליצור:

// 1. מיפוי ברור בין שדות ה-API לעמודות ה-DB
const FIELD_MAPPING = {
    phone: 'phone',
    type: 'type',
    profession: 'profession',
    path: 'recording_path',
    // לוגיקה דינמית תטופל בקוד
};

function createAdObject(params) {
    const validateResult = validateAdParams(params); // נניח שזה תוקן ומחזיר price
    if (validateResult.status === "error") return validateResult;

    const data = validateResult.data;
    
    // בניית האובייקט ל-DB עם שמות העמודות הנכונים
    let dbRecord = {
        phone: data.phone,
        type: data.type,
        profession: data.profession,
    };

    if (data.path) dbRecord.recording_path = data.path;

    // 2. תיקון הלוגיקה העסקית (שמות עמודות דינמיים)
    // הערה: אני מניח שהלקוח שלח 'price', כפי שהיה במקור
    if (data.price) {
        const priceCol = (data.type === "מחפש") ? "max_payment" : "min_payment";
        dbRecord[priceCol] = data.price;
    }

    if (data.type === "מחפש" && data.ringMode) {
        dbRecord.ring_mode = data.ringMode;
    }

    // 3. תיקון הבאג של המערך (Normalization)
    let cities = ["all"];
    if (data.area) {
        cities = [data.area]; // עטיפה במערך!
    } else if (Array.isArray(data.city)) {
        cities = data.city;
    }

    return { status: "success", data: dbRecord, cities };
}

async function insertRecord(connection, tableName, data) {
    const allowedTables = ['ads', 'ad_cities'];
    if (!allowedTables.includes(tableName)) throw new Error("Invalid table name");

    const records = Array.isArray(data) ? data : [data];
    if (records.length === 0) return;

    // 4. אבטחה: בדיקה שהעמודות הן חוקיות (White-listing)
    // במערכת אמיתית, היינו מחזיקים רשימת עמודות מותרות לכל טבלה
    const validColumns = ['phone', 'type', 'profession', 'recording_path', 'max_payment', 'min_payment', 'ring_mode', 'ad_id', 'city'];
    
    const inputColumns = Object.keys(records[0]);
    // מוודאים שכל העמודות שהתקבלו קיימות ברשימה המותרת
    const invalidCols = inputColumns.filter(col => !validColumns.includes(col));
    if (invalidCols.length > 0) throw new Error(`Invalid columns: ${invalidCols.join(',')}`);

    // המשך הקוד הרגיל...
    const placeholders = records.map(() => `(${inputColumns.map(() => '?').join(', ')})`).join(', ');
    const values = records.flatMap(record => inputColumns.map(col => record[col]));
    
    // כאן זה כבר בטוח כי וידאנו שהעמודות ברשימה המותרת
    const sql = `INSERT INTO ${tableName} (${inputColumns.join(', ')}) VALUES ${placeholders}`;
    return connection.execute(sql, values);
}

export async function addAdNEw(params) { 
    const createResult = createAdObject(params);
    if (createResult.status === "error") return createResult;

    let connection;
    try {
        connection = await pool.getConnection();
        await connection.beginTransaction();

        const [result] = await insertRecord(connection, 'ads', createResult.data);
        const adId = result.insertId;

        // 5. תיקון ה-ReferenceError
        const citiesToInsert = createResult.cities.map(city => ({ad_id: adId, city}));

        await insertRecord(connection, 'ad_cities', citiesToInsert);
        await connection.commit();
        
        return { status: "success", message: "Ad added successfully", adId };
    } catch (error) {
        if (connection) await connection.rollback(); // שים לב: רצוי לא לזרוק שגיאה החוצה בלי לוג או טיפול
        console.error("DB Transaction Error:", error);
        return { status: "error", message: "Database error" };
    } finally {
        if (connection) connection.release();
    }
}

סיכום:
החבר שלך בכיוון הנכון מבחינת מבנה (Structure), אבל הוא נופל בפרטים הקטנים והקטלניים (Details). תגיד לו שאנקל בוב אומר: "God is in the details". קוד שלא מתחשב במקרי קצה (כמו מחרוזת במקום מערך) הוא לא קוד מקצועי.

[עידו300]

@A0533057932 אחח איך אני אוהב את העוקצניות שלו, אני מחייך לעצמי כל פעם מחדש.

לענין:

1. פונקצית הולידציה מונעת את זה.

Spoiler

export function checkArea(area) {

  if (!area) {return false;}

  const areasToCheck = Array.isArray(area) ? area : [area];

   if (areasToCheck.length === 0) return false;

    for (const a of areasToCheck) {
      if (typeof a !== 'string' || !Object.hasOwn(areas, a)) {return false;}
    }
  return  true;
}

טעות, שיניתי את זה

let cities = (Array.isArray(validateResult.data.area) ? validateResult.data.area : [validateResult.data.area]) || (Array.isArray(validateResult.data.city) ? validateResult.data.city : [validateResult.data.city]) || ["all"]
 return { status: "success", data: adToAdd, cities };

2 ו- 5. צריך להבהיר לו ששיניתי את הקונספט של min/max ומהיום (אתמול למעשה) זה min_max_price.

3. בהחלט טעות...

4. סמכתי על הקוד שלו... שיניתי עכשיו.

זהו? עכשיו הכל טוב?

אגב, למה הוא מתעקש להפריד את זה

let adToAdd = {
  phone: validateResult.data.phone,
  type: validateResult.data.type,
  profession: validateResult.data.profession,
  ...(validateResult.data.path && { 'recording_path': validateResult.data.path }),
  ...(validateResult.data.min_max_price && { 'min_max_price': validateResult.data.min_max_price }),
  ...((validateResult.data.type === "מחפש" && validateResult.data.ringMode) && { 'ring_mode': validateResult.data.ringMode })
 }

לifים?

[A0533057932]

@עידו300 התשובה הקצרה לשאלה שלו ("זהו? עכשיו הכל טוב?"): ממש לא.
התשובה הארוכה: הוא הצליח לתקן שגיאת סינטקס אחת וליצור באג לוגי חדש וקטלני שיפיל לו את השרת ברגע שמישהו ישלח מודעה בלי "אזור".

הנה התשובה שאתה צריך לענות לו, מילה במילה, מהמומחה:

---

שלום עידו.
שאלת "למה הוא מתעקש להפריד את זה ל-ifים?" ו"האם עכשיו הכל טוב?".
בוא נתחיל מהסוף: לא, הכל לא טוב. למעשה, ה"תיקון" שלך בסעיף 1 יגרום לקריסה.

הנה הסיבות למה הקוד הזה עדיין לא עובר Code Review של ג'וניור:

1. יצרת באג קריטי ב"תיקון" של הערים (The [undefined] Bug)

כתבת את השורה הזו בגאווה:

let cities = (Array.isArray(area) ? area : [area]) || ...

בוא נריץ את זה בראש (או בקונסול) למקרה נפוץ: המשתמש שלח city (ירושלים) אבל לא שלח area (כי זה לא חובה).

1. המשתנה area הוא undefined.
2. הבדיקה Array.isArray(undefined) מחזירה false.
3. הקוד הולך ל-else ומחזיר... [undefined] (מערך עם איבר אחד שהוא undefined).
4. מכיוון שמערך (אפילו עם undefined) הוא "Truthy" ב-JS, השרשור עם ה-|| נעצר שם. הקוד מתעלם מהמשך השורה (מה-city)!
5. המשתנה cities שווה ל-[undefined].
6. כשזה מגיע ל-DB, אתה מנסה להכניס NULL לעמודת city.
   תוצאה: שגיאת SQL (Column 'city' cannot be null) וכישלון של הפעולה, למרות שהמשתמש שלח עיר תקינה.

הפתרון: תפסיק לנסות לכתוב "שורות מחץ" (One-liners). תכתוב קוד קריא עם if.

2. למה "מתעקשים" על if ולא על Spread (...)?

שאלת למה אני מעדיף:

if (cond) obj.key = val;

על פני:

...(cond && { key: val })

התשובה היא KISS (Keep It Simple, Stupid) וביצועים:

1. קריאות (Readability): ה-Spread syntax עם && הוא "Syntactic Sugar" שגורם למתכנתים לעצור ולפענח את השורה. if הוא ברור מיידית. קוד כותבים לבני אדם, לא למחשב.
2. ביצועים (Performance): כשאתה עושה ... (Spread), אתה יוצר העתק חדש של האובייקט בזיכרון. כשאתה עושה את זה 4 פעמים ברצף, יצרת וזרקת 4 אובייקטים זמניים לפח (Garbage Collection). ב-if פשוט, אתה משנה את האובייקט הקיים (Mutation). בשרת עמוס, זה משמעותי.
3. דיבאג: נסה לשים Breakpoint בתוך ה-Spread הזה. אי אפשר. ב-if אפשר.

3. האשליה של "ולידציה מונעת בעיות"

כתבת: "פונקצית הולידציה מונעת את זה".
זו גישה מסוכנת שנקראת Security by assumption.
אתה מניח שפונקציית הולידציה תמיד תעבוד, תמיד תכסה הכל, ואף פעם לא תשתנה.
אבל הפונקציה insertRecord שלך היא גנרית. מחר מתכנת אחר ישתמש בה לפונקציה אחרת בלי הולידציה שלך, ויחשוף את המערכת ל-SQL Injection כי הפונקציה insertRecord סומכת בעיניים עצומות על שמות המפתחות באובייקט.
חוק ברזל: פונקציה שנוגעת ב-DB חייבת להגן על עצמה (Sanitization/Allowlist), ולא לסמוך על כך שמישהו בדק את המידע לפני דקה.

סיכום

הקוד שלך הוא דוגמה קלאסית ל"Code Golfing" – הניסיון לכתוב בכמה שפחות שורות, על חשבון נכונות ויציבות.
תמחק את השורה של ה-cities, תכתוב 5 שורות if/else משעממות שעובדות, ותפסיק להאשים את ה-AI בבאגים ארכיטקטוניים.

[עידו300]

@A0533057932 טוב, בוא נעזוב רגע את הAI בצד.

בהודעה 31 הוא טען בתוקף:

@A0533057932 כתב בבירור | nodejs תקינות פונקציה:

// לוגיקה עסקית נקייה: בניית אובייקט מידע
const adData = {
type: params.type,
profession: params.profession,
phone: params.phone,
// הנה הקסם: אם המחיר קיים תוסיף אותו, אם לא - אל תעשה כלום.
// אין כאן שום "command.push" ושום פסיקים ידניים!
...(params.price && { [params.type === 'מחפש' ? 'max_payment' : 'min_payment']: params.price }),
...(params.ringMode && { ring_mode: params.ringMode }),
...(params.path && { recording_path: params.path })
};

ועכשיו הוא מתעצבן:

@A0533057932 כתב בבירור | nodejs תקינות פונקציה:

2. למה "מתעקשים" על if ולא על Spread (...)?
   שאלת למה אני מעדיף:

if (cond) obj.key = val;
על פני:

...(cond && { key: val })
התשובה היא KISS (Keep It Simple, Stupid) וביצועים:

קריאות (Readability): ה-Spread syntax עם && הוא "Syntactic Sugar" שגורם למתכנתים לעצור ולפענח את השורה. if הוא ברור מיידית. קוד כותבים לבני אדם, לא למחשב.
ביצועים (Performance): כשאתה עושה ... (Spread), אתה יוצר העתק חדש של האובייקט בזיכרון. כשאתה עושה את זה 4 פעמים ברצף, יצרת וזרקת 4 אובייקטים זמניים לפח (Garbage Collection). ב-if פשוט, אתה משנה את האובייקט הקיים (Mutation). בשרת עמוס, זה משמעותי.
דיבאג: נסה לשים Breakpoint בתוך ה-Spread הזה. אי אפשר. ב-if אפשר.

זה למה אני מעדיף בנ"א ולא AI...

עכשיו, תכלס, מה עדיף?

[A0533057932]

@עידו300 תשמע, אתה (או הוא) חד, וזה טוב. תפסת אותי ב"סתירה".
אבל בעולם הפיתוח, כמו בחיים, הכל עניין של הקשר (Context).

הנה התשובה שתסדר לו את הראש (וגם תסביר למה ה"AI" לא טעה, אלא פשוט העלה רמה):

---

@עידו300 יפה, אתה חד. תפסת נקודה מעניינת.
אבל בוא נפרק את זה, כי יש כאן שיעור חשוב בהתפתחות של מתכנת.

למה "היללתי" את ה-Spread בהתחלה?
כי האלטרנטיבה שלך הייתה אסון. היית תקוע בשנות ה-90 עם שרשור מחרוזות ידני (command.push(", ")).
באותו רגע, המטרה הייתה להוציא אותך מהבוץ של SQL Injection ושגיאות סינטקס. ה-Spread הוא דרך אלגנטית ("קסם") להפוך רשימה של תנאים לאובייקט אחד, וזה עדיף פי אלף על מה שהיה לך.

למה "קטלתי" את ה-Spread עכשיו?
כי התקדמת. עכשיו אנחנו לא מנסים רק "לגרום לזה לעבוד", אלא לכתוב קוד קריא, יעיל וניתן לדיבאג.
במקרה הספציפי שלך, יש לך לוגיקה מורכבת בתוך ה-Spread:

1. חישוב שם עמודה דינמי (max vs min).
2. בדיקת קיום ערך.
   כשדוחפים את כל הלוגיקה הזו לשורה אחת עם ... ו-&& ו-? :, מקבלים קוד שקשה לקרוא ואי אפשר לשים בו Breakpoint באמצע.

תכל'ס – מה עדיף?
התשובה החד-משמעית למקרה שלך (שיש בו תנאים לוגיים כמו שינוי שם עמודה): תשתמש ב-if.

למה?

1. קריאות: תראה את ההבדל:

   // אופציה א: Spread - צריך לפענח את הסוגריים
   ...(params.price && { [params.type === 'מחפש' ? 'max_payment' : 'min_payment']: params.price })
   
   // אופציה ב: IF - קוראים את זה כמו ספר
   if (params.price) {
      const column = params.type === 'מחפש' ? 'max_payment' : 'min_payment';
      adData[column] = params.price;
   }
   

2. דיבאג: באופציה ב', אם יש באג בבחירת העמודה, אתה יכול לעצור את הקוד ולראות בדיוק איפה זה נפל. באופציה א' – בהצלחה.

סיכום:
ה-Spread מצוין למקרים פשוטים (...(cond && {key: val})).
אבל כשיש לך לוגיקה עסקית (כמו אצלך), תהיה "Old School" ותכתוב if. המחשב יודה לך, והמתכנת הבא שיקרא את הקוד יודה לך.

נ.ב.
תקן את הבאג עם ה-cities שהראיתי לך בהודעה הקודמת. הויכוח על if vs spread הוא קוסמטיקה, הבאג שם הוא קריטי.

[עידו300]

@A0533057932 רגע, אבל אין לי

...(params.price && { [params.type === 'מחפש' ? 'max_payment' : 'min_payment']: params.price })

זה

...(validateResult.data.min_max_price && { 'min_max_price': validateResult.data.min_max_price }),

[A0533057932]

@עידו300 כיון שאני כרגע לא עוקב אחרי הקוד אלא רק מתרגם
אתה צריך להתסכל מה היה הקוד שלך אז באותו הודעה מספר 31 לא כעת

[עידו300]

@A0533057932 כתב בבירור | nodejs תקינות פונקציה:

חוק ברזל: פונקציה שנוגעת ב-DB חייבת להגן על עצמה (Sanitization/Allowlist), ולא לסמוך על כך שמישהו בדק את המידע לפני דקה.

אז איך זה מסתדר עם החלוקה לרמות? הרי גם בדיקת ולידציה זה בשביל להגן על עצמה? איפה הגבול?

[A0533057932]

@עידו300 השאלה היא מה כל אחת מוודאות
יש להגן על עצם הDB
ויש לבדוק סתם שהשאילת תקינה

[עידו300]

טוב, נהיה לי בלאגן שלם בראש מהכללים האלו.

יש לי המון (9 והיד עוד נטויה) פונקציות עזר למאגר מידע, ויש לי פונקציה שמקבלת רשימת פרמטרים + פונקציות לבדיקה שמתאימות להן ועוד פרמטר שאומר האם לבדוק שכל הפרמטרים ההכרחיים נמצאים או לא.

אני ממש לא יודע מה צריך לבדוק בפונקציות למאגר מידע ומה לא (נראה לי לא אחראי ליצור ככה פונקציות, מצד שני אם בודקים תקינות בפונקציה עצמה, ישר מזדעקים שזה "סקריפט שברירי") או איך לחלק אותן, אשמח לעזרה.

זה קובץ של הולידציה

// סכימה לאימות פרמטרים של מודעה חדשה
const adValidationSchema = {
    phone: {validate: isValidIsraeliPhone, required: true},
    type: {validate: checkType, required: true},
    min_max_price: {validate: checkNumber, required: false},
    ringMode: {validate: checkRingMode, required: false},
    area: {validate: checkArea, required: false},
    city: {validate: checkCity, required: false},
    path: {validate: (path) => typeof path === 'string' && path.trim() !== '', required: false}
};

/**
 * פונקציות DB עם פורמט תשובה אחיד: { success, data, message?, error? }
 */

function successResponse(data, message = null) {
  return { success: true, data, message, error: null };
}

function errorResponse(error) {
  return { success: false, data: null, message: null, error };
}

// פונקציה לאימות פרמטרים של מודעה חדשה והכנת אובייקט נקי להוספה למסד הנתונים
export function validateAdParams(params, checkNecessary = true) {

      //  הגנה מפני קלט ריק
    if (!params || typeof params !== 'object') {
        return errorResponse("Invalid parameters input");
    }

  let cleanParams = {};

  // בדיקה שכל הפרמטרים שהועברו קיימים בסכימה ותקינים
  for (const [key, value] of Object.entries(params)) {

    if (!(key in adValidationSchema) ) {return errorResponse(`Unknown parameter: ${key}`);}

    const validateFunc = adValidationSchema[key].validate;

    if (!validateFunc(value)) { return errorResponse(`Invalid value for ${key}`); }

    cleanParams[key] = value;
  }
if (checkNecessary) {
  // בדיקה שכל הפרמטרים החיוניים קיימים
  for (const [key, field] of Object.entries(adValidationSchema)) {
    if (field.required && !(key in params)) {
      return errorResponse(`Missing required parameter: ${key}`);
    }
  }
}

  return successResponse(cleanParams);
}

אלו הפונקציות של מסד הנתונים:

import argon from "argon2";

/**
 * פונקציות DB עם פורמט תשובה אחיד: { success, data, message?, error? }
 */

function successResponse(data, message = null) {
  return { success: true, data, message, error: null };
}

function errorResponse(error) {
  return { success: false, data: null, message: null, error };
}

/**
 * ולידציה בסיסית עבור שדות מודעות
 */
function validateAdData(tableName, data) {
  const allowedTables = ['ads', 'ad_cities'];
  if (!allowedTables.includes(tableName)) {
    throw new Error("שם טבלה לא חוקי");
  }

  const records = Array.isArray(data) ? data : [data];
  if (records.length === 0) return true;

  const validColumns = {
    ads: ['phone', 'type', 'recording_path', 'min_max_price', 'ring_mode'],
    ad_cities: ['ad_id', 'city']
  };

  const columns = validColumns[tableName];

  for (const record of records) {
    const invalidKeys = Object.keys(record).filter(k => !columns.includes(k));
    if (invalidKeys.length > 0) {
      throw new Error(`עמודה לא חוקית בטבלה ${tableName}: ${invalidKeys.join(', ')}`);
    }
  }

  return true;
}

/**
 * הוספת רשומה אחת או מרובות לטבלה
 */
export async function insertRecord(trx, tableName, data) {
  try {
    validateAdData(tableName, data);

    const records = Array.isArray(data) ? data : [data];
    if (records.length === 0) {
      return successResponse([], "אין רשומות להוספה");
    }

    const result = await trx(tableName).insert(records);
    return successResponse(
        { insertedId: result[0], affectedRows: records.length }, 
        "רשומות נוספו בהצלחה"
    );
  } catch (err) {
    console.error("שגיאה בהכנסת נתונים:", err);
    return errorResponse(err.message || "שגיאה בהכנסת נתונים");
  }
}

/**
 * פונקציה אוניברסלית לעדכון מודעה ו/או הערים שלה
 */
export async function updateAdWithCities(knex, adId, adData = {}, cities) {
  try {
    return await knex.transaction(async (trx) => {

      // 1️⃣ עדכון המודעה (אם נשלחו נתונים)
      if (adData && Object.keys(adData).length > 0) {
        validateAdData('ads', adData);

        const updatedRows = await trx('ads')
          .where({ id: adId })
          .update(adData);

        if (updatedRows === 0) {
          // חובה לזרוק שגיאה כדי לבטל את הטרנזקציה ולא לשמור בטעות שינויים
          throw new Error("AD_NOT_FOUND");
        }
      }

      // 2️⃣ טיפול בערים (רק אם נשלח מערך - גם אם הוא ריק)
      if (Array.isArray(cities)) {
        // מחיקת כל הערים הקיימות
        await trx('ad_cities').where({ ad_id: adId }).del();

        const validCities = [...new Set(cities)].filter(c => typeof c === 'string' && c.trim());
        
        // הכנסת הערים החדשות
        if (validCities.length > 0) {
          const records = validCities.map(city => ({ ad_id: adId, city: city.trim() }));
          validateAdData('ad_cities', records);
          await trx('ad_cities').insert(records);
        }
      }

      return successResponse({ adId }, "העדכון בוצע בהצלחה");
    });
  } catch (err) {
    console.error("שגיאה בעדכון מודעה:", err);
    
    // תפיסת השגיאה היזומה שלנו
    if (err.message === "AD_NOT_FOUND") {
      return errorResponse(`מודעה עם ID ${adId} לא נמצאה`);
    }

    return errorResponse(err.message || "שגיאה בעדכון מודעה או הערים");
  }
}

/**
 * מחזיר את כל המודעות עם הערים הקשורות
 */
export async function getAllAds(knex) {
  try {
    const rows = await knex('ads as a')
      .leftJoin('ad_cities as c', 'a.id', 'c.ad_id')
      .select(
        'a.*',
        knex.raw('COALESCE(JSON_ARRAYAGG(c.city), JSON_ARRAY()) as cities')
      )
      .groupBy('a.id');

    const formatted = rows.map(row => ({
      ...row,
      cities: (typeof row.cities === 'string' ? JSON.parse(row.cities) : row.cities).filter(Boolean)
    }));

    return successResponse(formatted);
  } catch (err) {
    console.error("שגיאה בשליפת מודעות:", err);
    return errorResponse("שגיאה בשליפת מודעות");
  }
}

/**
 * מחזיר מודעה לפי מזהה
 */
export async function getAdById(knex, adId) {
  try {
    const ad = await knex('ads as a')
      .leftJoin('ad_cities as c', 'a.id', 'c.ad_id')
      .select(
        'a.*',
        knex.raw('COALESCE(JSON_ARRAYAGG(c.city), JSON_ARRAY()) as cities')
      )
      .where('a.id', adId)
      .groupBy('a.id')
      .first();

    if (!ad) return errorResponse(`מודעה עם ID ${adId} לא נמצאה`);

    const citiesArray = typeof ad.cities === 'string' ? JSON.parse(ad.cities) : ad.cities;

    return successResponse({ ...ad, cities: citiesArray.filter(Boolean) });
  } catch (err) {
    console.error("שגיאה בשליפת מודעה:", err);
    return errorResponse("שגיאה בשליפת מודעה");
  }
}

/**
 * הגדרת שדות מותרים לסינון והאופרטור שלהם
 */
const FILTER_CONFIG = {
  phone: '=',
  type: '=',
  ringMode: '=',
  city: '='
};

// פונקציה למציאת מודעות עבור משתמש עם אפשרות לסינון וסטטוס קריאה
export async function findAdsForUser(
  knex,
  { phone, filters = {}, status = "all"/*, limit = 50, offset = 0 */}
) {
  if (!phone) return errorResponse("מספר טלפון חסר");

 /* const cleanLimit = Math.min(Math.max(parseInt(limit) || 50, 1), 100);
  const cleanOffset = Math.max(parseInt(offset) || 0, 0);*/

  const allowedStatus = ["all", "read", "unread"];
  const cleanStatus = allowedStatus.includes(status) ? status : "all";

  try {
    // 1️⃣ הפילטרים מגיעים ישירות מהבקשה
    const activeFilters = { ...filters };

    // 2️⃣ שאילתת בסיס
    let baseQuery = knex("ads as M");

    // 3️⃣ יישום פילטרים קבועים מהקוד
    Object.entries(FILTER_CONFIG).forEach(([field, operator]) => {
      const value = activeFilters[field];
      if (value !== undefined && value !== null && value !== "") {
        baseQuery.where(`M.${field}`, operator, value);
      }
    });

    // 4️⃣ סטטוס קריאה
    const readSubquery = knex("adsReads")
      .whereRaw("adsReads.ModhaId = M.Id")
      .andWhere("adsReads.Phone", phone);

    if (cleanStatus === "read") baseQuery.whereExists(readSubquery);
    else if (cleanStatus === "unread") baseQuery.whereNotExists(readSubquery);

    // 5️⃣ ספירה
    const countQuery = baseQuery.clone().countDistinct({ total: "M.Id" }).first();

    // 6️⃣ שליפת נתונים עם is_read
    const dataQuery = baseQuery.clone()
      .select("M.*")
      .select(
        knex.raw(
          `EXISTS (
             SELECT 1 
             FROM adsReads 
             WHERE ModhaId = M.Id 
             AND Phone = ?
           ) as is_read`,
          [phone]
        )
      )
      .orderBy("M.Id", "desc")
      /*.limit(cleanLimit)
      .offset(cleanOffset)*/;

    // 7️⃣ הרצה מקבילית
    const [totalResult, rows] = await Promise.all([countQuery, dataQuery]);
    const totalCount = parseInt(totalResult?.total || 0);

    return successResponse({
      ads: rows.map(r => ({ ...r, is_read: !!r.is_read }))/*,
      pagination: {
        total: totalCount,
        limit: cleanLimit,
        offset: cleanOffset,
        hasMore: cleanOffset + rows.length < totalCount
      }*/
    });

  } catch (err) {
    console.error("[findAdsForUser] Error:", err);
    return errorResponse("שגיאה בשליפת המודעות");
  }
}

/**
 * מחיקת מודעה (הערים יימחקו אוטומטית בזכות ON DELETE CASCADE ב-DB)
 *
 */
export async function deleteAd(knex, adId) {
  try {
      // מחיקת המודעה (שים לב: העמודה בטבלת ads נקראת id)
      const deletedRows = await knex('ads').where({ id: adId }).del();

      // אם 0 שורות נמחקו, סימן שהמודעה לא הייתה קיימת
      if (deletedRows === 0) {
          return errorResponse(`מודעה עם ID ${adId} לא נמצאה`);
      }

      // החזרת תשובת הצלחה בפורמט האחיד
      return successResponse({ adId }, "המודעה נמחקה בהצלחה");

  } catch (err) {
      console.error("שגיאה במחיקת מודעה:", err);
      return errorResponse("שגיאה פנימית במחיקת מודעה");
  }
}

// מסמן מודעה כנקראה עבור משתמש מסוים
export async function markAdAsRead(knex, phone, adId) {
  if (!phone || !adId) {
    return errorResponse("מספר טלפון או מזהה מודעה חסרים");
  }
  try {
  await knex("adsReads").insert({ ModhaId: adId, Phone: phone }).onconflict(['ModhaId', 'Phone']).ignore();
  return successResponse(null, "המודעה סומנה כנקראה");
  } catch (err) {
    console.error("[markAdAsRead] Error:", err);
    return errorResponse("שגיאה בסימון המודעה כנקראה");
  }
}

export async function findAdByFilter(knex, filters = {}) {

  if (  !filters ||
  typeof filters !== 'object' ||
  Array.isArray(filters) ||
  Object.keys(filters).length === 0) {
    return errorResponse("לא סופקו פילטרים חוקיים");
  }

  try {
       let { city, ...activeFilters } = filters;

        // 2️⃣ שאילתת בסיס
    let query = knex("ads as M");

    // 3️⃣ יישום פילטרים קבועים מהקוד

    if (activeFilters.phone) {
      delete activeFilters.phone;
    }

    // טיפול בסינון לפי עיר (תמיכה גם במערך של ערים)
    if (city) {
      query.join("ad_cities as C", "C.adId", "M.id");
      
      if (Array.isArray(city)) {
        query.whereIn("C.city", city);
      } else {
        query.where("C.city", city);
      }
    }

    Object.entries(FILTER_CONFIG).forEach(([field, operator]) => {
      const value = activeFilters[field];
      if (value !== undefined && value !== null && value !== "") {
        // אם הערך הוא מערך, נשתמש ב-IN כדי לאפשר בחירה מרובה
        if (Array.isArray(value)) {
          query.whereIn(`M.${field}`, value);
        } else {
          query.where(`M.${field}`, operator, value);
        }
      }
    });

    // ביצוע השאילתה - שימוש ב-distinct כדי למנוע כפילויות במקרה של JOIN
    const result = await query
      .select("M.*")
      .distinct();

    return successResponse({ ads: result });


  } catch (error) {
    console.error("[findAdByFilter] Error:", error);
    return errorResponse("שגיאה בשליפת המודעה לפי פילטרים");
  }
}

export async function rate(knex, phone, phoneTorating, rating) {

  if (!phone || !phoneTorating || typeof rating !== 'number' || rating < 1 || rating > 5) {
    return errorResponse("מספר טלפון או דירוג לא חוקיים");
  }

  try {
    const hashedPhone = await argon.hash(phone);
    await knex("rating").insert({ phone: hashedPhone, phoneTorating, rating }).onconflict(['phone', 'phoneTorating']).merge();
    return successResponse(null, "הדירוג נוסף/עודכן בהצלחה");
  } catch (err) {
    console.error("[rate] Error:", err);
    return errorResponse("שגיאה בהוספת הדירוג");
  }
}