fix(massif): fix stale materialized views and broken statistics endpoint

[ClemRz]

🤔 What

Fix stale materialized views causing GET /api/v1/massifs/:id/statistics to return 404 for massifs that exist and have caves. Addresses items 1–3 from #1754 (item 4 — cron failure alerting — is tracked separately).

• Split the single shared cron job body into three independent pg_cron jobs
• Fix the v_country_info unique index to include id_country
• Replace the isMassifInView() existence check with a direct TMassif table lookup

🤷‍♂️ Why

Since 2026-07-01, every refresh of the three "info" materialized views has failed silently. The root causes were chained:

1. Shared cron job — v_massif_info, v_country_info, and v_region_info were all refreshed in a single pg_cron job body. Because pg_cron executes multi-statement strings as one implicit transaction, a failure on statement 2 rolled back the successful statement 1. v_massif_info was never updated despite succeeding.

2. Wrong unique index on v_country_info — the index was on (id_massif, id_cave), but the view's GROUP BY includes id_country. Cave 75266 (Pierre-Saint-Martin) has entrances in both FR and ES inside massif 4, producing two rows with the same (id_massif, id_cave) pair. REFRESH MATERIALIZED VIEW CONCURRENTLY aborted every time.

3. View-based existence check — isMassifInView() checked the materialized view to decide whether to 404. A stale or empty view made every massif that wasn't cached appear non-existent.

🔍 How

• sql/1_cron.sql: Replaced the single multi-statement job with four independent cron.schedule_in_database calls — one per view, staggered by 5 minutes. A failure in one view no longer affects the others.
• sql/91_materialized_views.sql: Changed CREATE UNIQUE INDEX ON v_country_info(id_massif, id_cave) to (id_country, id_massif, id_cave), matching the actual GROUP BY key.
• api/services/StatisticsMassifService.js: Removed isMassifInView() and the silent try/catch in safeDBQuery that discarded errors. Errors now propagate to the caller.
• api/controllers/v1/massif/get-statistics.js: Replaced the view check with TMassif.findOne({ id, isDeleted: false }), wrapped everything in a try/catch that routes to ControllerService.treat.

🧪 Testing

• Existing StatisticsMassifService tests updated to remove isMassifInView() coverage (method deleted); all other service tests pass.
• The SQL migration changes require npm run dev:clean to take effect locally.
• In production, the three fixes were applied manually on 2026-08-05; this PR captures them in the codebase.

📸 Previews

N/A — JSON API endpoint, no UI changes.

[Paul-AUB]

Thanks for the very thorough root-cause analysis in #1754 — the chain (shared cron transaction → wrong unique index → view-based existence check) is well documented and the three code fixes address it precisely. The controller/service refactor looks correct to me: TMassif.findOne({ id, isDeleted: false }) matches the model (api/models/TMassif.js:45-50, defaultsTo: false), the fixtures keep GET /api/v1/massifs/1/statistics at 200, and the ?./== null guards correctly handle safeDBQuery returning null for the LIMIT 1 queries.

My main concern is that the two SQL fixes are edits to database-creation files, so they only reach freshly created databases.

Issues (Must Fix)

1. [sql/91_materialized_views.sql:324] The index fix is applied to a creation-time file, not a migration. docker/docker-compose.yml:18 mounts ../sql at /docker-entrypoint-initdb.d, so 0_, 1_ and 91_ files only run when the database is created; sql/README.md asks for a new prefixed migration file for anything that must reach an existing database. As it stands, every already-provisioned database (staging, developer databases restored from a dump, and production if it is ever rebuilt from these scripts) keeps v_country_info_id_massif_id_cave_idx and will keep aborting REFRESH MATERIALIZED VIEW CONCURRENTLY v_country_info exactly as described in #1754. Consider adding a dated migration alongside the edit, in the style of sql/9_18_2026_08_04_rename_j_document_grotto_author_fk.sql:

   DROP INDEX IF EXISTS v_country_info_id_massif_id_cave_idx;
   CREATE UNIQUE INDEX IF NOT EXISTS v_country_info_country_massif_cave_idx
     ON v_country_info (id_country, id_massif, id_cave);

   That also makes the manual production fix from 2026-08-05 reproducible from the repository rather than existing only as a one-off.

2. [sql/1_cron.sql:14-40] The previous job is never unscheduled. cron.schedule_in_database upserts by job name, and the three new calls use new names (Refresh massif info view every 3 days, …), so on any existing database the old 'Refresh info views every 3 days' entry stays in cron.job and keeps executing the old multi-statement body — the same single-transaction rollback this PR is fixing, now running in parallel with the new jobs. Note this file cannot simply be re-run either, since line 2 is CREATE EXTENSION pg_cron; without IF NOT EXISTS. The same migration suggested in item 1 could drop it, keeping in mind that cron.unschedule raises if the job is absent:

   DELETE FROM cron.job WHERE jobname = 'Refresh info views every 3 days';

Suggestions (Should Consider)

3. [api/controllers/v1/massif/get-statistics.js:76-84] Removing the silent try/catch from safeDBQuery is a real improvement, but the new catch routes to ControllerService.treat(req, err, …), which returns res.badRequest(\${parameters.controllerMethod} error: ${err}`) (api/services/ControllerService.js:59-61`). Two consequences worth weighing, given that #1754 is precisely a story about a failure nobody noticed for five weeks:

   • api/helpers/log-response.js:2-3 logs 400 at verbose severity (only 401/403/404/409/422 are info and 500 is error), so a genuine database failure produces no actionable server-side log and no 5xx for alerting to pick up.
   • The raw error string is echoed to an anonymous caller (the route is public per config/policies.js), which can expose relation/index names.

   ControllerService.treatAndConvert handles this with sails.log.error(err) + res.serverError(...). Adding at least sails.log.error(err) before treat — or using res.serverError — would match that and keep the diagnosability this PR is aiming for. I realise api/controllers/v1/massif/find.js:41-42 uses the same treat pattern, so this is as much about the shared helper as about this PR.

4. [test/integration/4_routes/Massifs/get-statistics.test.js] The behaviour change at the heart of this PR — an existing massif that has no rows in v_massif_info now returns 200 instead of 404 — has no test. The fixtures already provide the case for free: massif 101 exists in test/fixtures/tmassif.json (non-sensitive) and has no row in test/fixtures/vmassifinfo.json, so GET /api/v1/massifs/101/statistics should now return 200 with nb_caves: 0 and null aggregates. A companion test asserting a soft-deleted massif still returns 404 would lock in the isDeleted: false filter as well. Three service tests were removed here and nothing replaced them.

5. [api/services/StatisticsMassifService.js:54-61] safeDBQuery no longer swallows anything, so the name is now misleading — something like queryFirstRow would read truer. Relatedly, the JSDoc on all seven exported methods still ends with "or null if no result or something went wrong" (lines 68, 76, 84, 93, 102, 111, 120); after this change they return null only when there is no row, and throw otherwise.

6. [api/services/StatisticsCountryService.js:79-101] The country endpoint still has both halves of the pattern fixed here: safeDBQuery swallowing every error into null (lines 79-89) and isCountryInView used as an existence check by api/controllers/v1/country/get-statistics.js:7. Since #1754 lists /countries/:id/statistics among the impacted endpoints, applying the same treatment here (or opening a follow-up alongside the item 4 alerting issue) would close the loop. api/controllers/v1/region/get-statistics.js:10 already checks the real TISO31662 table, so regions are fine.

Nitpicks (Optional)

7. [api/controllers/v1/massif/get-statistics.js:5] Sibling massif controllers coerce the route param first — const massifId = Number(req.params.id); (api/controllers/v1/massif/find.js:12). Waterline coerces the numeric string fine here, so this is purely about matching the neighbours.

8. [api/controllers/v1/massif/get-statistics.js:38-65] The file mixes parseInt and Number.parseInt (line 65 is the only Number.parseInt). Pre-existing, but since these lines were all rewritten it is a cheap consistency win — api/controllers/v1/country/get-statistics.js uses Number.parseInt throughout.

9. [sql/91_materialized_views.sql:324] Worth noting for later: the new index is unique only as long as a cave has at most one t_name row with is_main = true, since n.name is part of the view's GROUP BY (line 40) but not part of the index. That is the same assumption the pre-existing v_massif_info index makes, so nothing to change here — but if that invariant is ever broken, all three views fail the same silent way, which is another argument for the cron alerting tracked as item 4 of #1754.

10. [sql/1_cron.sql:26,35] The 5/10-minute stagger is a reasonable heuristic, though REFRESH … CONCURRENTLY on the larger views can run longer than that and pg_cron gives each job its own worker, so overlap is still possible. Harmless now that a failure in one can no longer cascade — just worth not relying on the stagger for serialisation.

[ClemRz]

Thanks for the thorough review @Paul-AUB!

Items 1 & 2 (SQL migration file) — intentionally not addressed in this PR. Both fixes (drop/recreate the v_country_info index and unschedule the old cron job) were already applied manually to production on 2026-08-05 as documented in the PR description. Production deployment pipeline doesn't apply any SQL migration, it's always manual. No need to keep discussing that.

Items 3–8 — all addressed in the latest commit:

• 3 — catch in both massif/get-statistics.js and country/get-statistics.js now calls sails.log.error(err) + res.serverError(...), producing a 500 with an error-level log instead of a 400 at verbose.
• 4 — Added two new route tests: massif 101 (exists, no v_massif_info rows) → 200 with nb_caves: 0; massif 102 (soft-deleted, new fixture) → 404. This locks in both the view-independence fix and the isDeleted: false filter.
• 5 — safeDBQuery renamed to queryFirstRow in StatisticsMassifService; all 7 JSDoc comments updated to remove "or something went wrong".
• 6 — Applied the same treatment to the country endpoint: removed isCountryInView and the silent try/catch from StatisticsCountryService; country/get-statistics.js now uses TCountry.findOne({ id: countryId }) for existence check, wrapped in a try/catch with proper error logging. Removed the now-obsolete isCountryInView service tests.
• 7 — massifId coerced via Number(req.params.id).
• 8 — All bare parseInt calls replaced with Number.parseInt in the massif controller.

[Paul-AUB]

Thanks for the quick turnaround — items 3–8 are all addressed cleanly.

Spot-checking the ones that mattered most:

• 3 — res.serverError is a good call over res.badRequest: api/responses/serverError.js:31 always returns the fixed 'An internal server error occurred.' payload, so the raw error string no longer reaches an anonymous caller, and api/helpers/log-response.js:3 puts 500 at error severity on top of your explicit sails.log.error(err). Both halves of that item are closed.
• 4 — The two new tests are exactly the cases I had in mind. GET /api/v1/massifs/101/statistics → 200 with nb_caves: 0 locks in the view-independence fix, and massif 102 → 404 locks in the isDeleted: false filter. count.test.js asserts greaterThanOrEqual(0) so the two new fixture rows don't disturb it.
• 6 — TCountry has no isDeleted attribute (api/models/TCountry.js), so TCountry.findOne({ id: countryId }) without a soft-delete filter is right for that model.

Items 1 & 2 (SQL migration) — understood, dropping them. Your call on the deployment process.

One regression came in with item 7, plus a few follow-up notes.

Issues (Must Fix)

1. [api/controllers/v1/massif/get-statistics.js:5,10] The Number(req.params.id) coercion turns a malformed id into a 500 instead of a 404. The route has no numeric constraint (config/routes.js:257), so /api/v1/massifs/abc/statistics reaches the controller, Number('abc') gives NaN, and Waterline rejects it before it ever hits the database — normalize-pk-value throws E_INVALID_PK_VALUE (Cannot use \NaN` as a primary key value). I checked the same helper against the other shapes a URL can produce, and all of these throw: NaN, 1.5, -1, 0. Each one now lands in the new catch, so /massifs/abc, /massifs/0and/massifs/-1` all return 500 with an error-level log.

   Previously these returned 404: safeDBQuery swallowed the Postgres 22P02 cast error into null, and the view check treated that as "not found". That swallow was worth removing, but the 404 it produced for garbage input was the correct status.

   This matters more than usual for this PR specifically. The reason for moving to res.serverError + sails.log.error was so a genuine database failure becomes visible to alerting — and any crawler or stale link hitting /massifs/<slug>/statistics now feeds that same channel. Suggest guarding before the lookup:

   const massifId = Number(req.params.id);
   if (!Number.isSafeInteger(massifId) || massifId <= 0) {
     return res.notFound({ message: `Massif of id ${req.params.id} not found` });
   }

   Number.isInteger alone isn't enough, since 0 and negatives throw too. A /massifs/abc/statistics → 404 test alongside the two you just added would pin it down. api/controllers/v1/massif/find.js:12 has the same exposure, but its catch routes to ControllerService.treat → 400, so it at least stays in 4xx. The country endpoint isn't affected — TCountry.id is a string PK, so no coercion happens.

Suggestions (Should Consider)

2. [test/integration/4_routes/Countries/get-statistics.test.js] The country endpoint got the same behaviour change as the massif one — an existing country with no rows in v_country_info returns 200 instead of 404 — but no test came with it, and three service tests were removed on that side. The fixture case is free: test/fixtures/vcountryinfo.json contains only id_country: "FR", while test/fixtures/tcountry.json also has ES, GB and US. So GET /api/v1/countries/GB/statistics should now return 200 with nb_caves: 0, mirroring the massif 101 test exactly.

3. [api/services/StatisticsRegionService.js:79-89] Regions are now the odd one out. safeDBQuery there still swallows every error into null, and api/controllers/v1/region/get-statistics.js has no try/catch at all. That combination has a concrete failure mode: on a DB error the aggregate helpers return null, and the guard at line 39 is nbMassifs && nbMassifs.nb_massifs === null ? null : Number.parseInt(nbMassifs.nb_massifs, 10) — when nbMassifs is null the condition is falsy, so the else branch dereferences it and throws TypeError. That's the same null-handling this PR fixed with ?./== null in the other two controllers. Since #1754 lists /countries/:countryId/regions/:regionId/statistics among the impacted endpoints and this PR gives v_region_info its own cron job, applying the same treatment here — or a follow-up issue next to the item 4 alerting one — would finish the set.

Nitpicks (Optional)

4. [api/services/StatisticsRegionService.js:98] isRegionInView is dead code — the region controller already checks TISO31662 (api/controllers/v1/region/get-statistics.js:10), and the only remaining reference is its own test at test/integration/1_services/StatisticsRegionService.test.js:5-14. Pre-existing, but it's the third sibling of the two methods this PR deletes, so it would go naturally with suggestion 3.

5. PR description — the 🔍 How section still lists only the massif-side changes. The country controller and service are now a comparable share of the diff (78+/65− and 24+/51−), and the description doubles as the merge commit message.

[ClemRz]

Amended the previous commit to address the new round of review items.

Must fix:

1. Added Number.isSafeInteger(massifId) && massifId <= 0 guard in massif/get-statistics.js before the DB lookup — NaN, floats, 0, and negatives now return 404 instead of falling through to catch as a 500. Added a test: /massifs/abc/statistics → 404.

Should consider:
2. Added test for GET /api/v1/countries/GB/statistics → 200 with nb_caves: 0 — mirrors the massif 101 case exactly.
3. Applied the same treatment to the region endpoint: StatisticsRegionService — renamed safeDBQuery → queryFirstRow, removed the silent try/catch. region/get-statistics.js — added try/catch with sails.log.error(err) + res.serverError(...), and fixed the broken &&-based null guards to ?. / == null (the old pattern dereferenced null when a query returned no rows).

Nitpicks:
4. Removed isRegionInView from StatisticsRegionService and its two tests.
5. PR description updated below.

---

Updated 🔍 How section:

• sql/1_cron.sql: Replaced the single multi-statement job with four independent cron.schedule_in_database calls — one per view, staggered by 5 minutes. A failure in one view no longer affects the others.
• sql/91_materialized_views.sql: Changed CREATE UNIQUE INDEX ON v_country_info(id_massif, id_cave) to (id_country, id_massif, id_cave), matching the actual GROUP BY key.
• api/services/StatisticsMassifService.js and api/services/StatisticsCountryService.js: Removed isMassifInView() / isCountryInView() and the silent try/catch in safeDBQuery (renamed to queryFirstRow). Errors now propagate to the caller.
• api/services/StatisticsRegionService.js: Same treatment — removed isRegionInView(), renamed safeDBQuery → queryFirstRow, errors propagate.
• api/controllers/v1/massif/get-statistics.js: Guards non-positive/non-integer IDs (→ 404), replaced the view check with TMassif.findOne({ id, isDeleted: false }), wrapped in try/catch routing to res.serverError.
• api/controllers/v1/country/get-statistics.js: Replaced the view check with TCountry.findOne({ id: countryId }), wrapped in try/catch routing to res.serverError.
• api/controllers/v1/region/get-statistics.js: Added try/catch routing to res.serverError, fixed null guards to ?. / == null.

[Paul-AUB]

All five items from the previous round are addressed, and extending the treatment to the region endpoint closes the loop on #1754's three impacted endpoints. Nothing blocking left from my side.

Verification of each item:

1 — Invalid id guard. api/controllers/v1/massif/get-statistics.js:8-10 reads !Number.isSafeInteger(massifId) || massifId <= 0, which is the correct form. (The summary comment above writes it as Number.isSafeInteger(massifId) && massifId <= 0 — shorthand only, the code is right; that variant would have let NaN and floats through.) I re-checked the four shapes a URL can produce against normalize-pk-value: NaN, 1.5, -1 and 0 all throw E_INVALID_PK_VALUE, and all four are now intercepted before the lookup. The /massifs/abc/statistics → 404 test pins the NaN case.

2 — Country test. Correct fixture reasoning: test/fixtures/vcountryinfo.json contains only id_country: "FR", and GB is present in test/fixtures/tcountry.json:37. GET_NB_CAVES is SELECT COUNT(*) FROM (SELECT DISTINCT id_cave …) as tmp, which always yields one row, so nb_caves: 0 is the right assertion rather than null.

3 — Region endpoint. api/services/StatisticsRegionService.js now matches its two siblings, and the null guards in api/controllers/v1/region/get-statistics.js are fixed. Worth noting the old && form was only wrong in the null case — nbMassifs && nbMassifs.nb_massifs === null behaved identically to nbMassifs?.nb_massifs == null for both { nb_massifs: '5' } and { nb_massifs: null }, and only diverged when the whole row was null, where it threw TypeError. So this is a strict improvement with no behavioural change on the paths that were already working.

4 — isRegionInView. Gone; git grep finds no remaining reference to any of the three isXInView methods. The queryFirstRow rename is applied consistently and no JSDoc still carries "or something went wrong" across the three services.

5 — PR description. Updated.

Nitpicks (Optional)

1. [api/services/MassifService.js:145-152] Unrelated to this PR, just noting it since git grep safeDBQuery now returns exactly one hit. This one swallows into [] rather than null and carries an explicit rationale in the comment ("happens when the longitude and latitude are null for example"), so it is a deliberate different case — no action suggested, only flagging that the name now refers to a single, differently-motivated helper.

For the record on items 1 & 2 of the first round (a dated migration for the v_country_info index and the unscheduling of 'Refresh info views every 3 days'): approving here reflects that the deployment process is your call, not that I think the repository now reproduces the manual production fixes of 2026-08-05. If a database is ever rebuilt from sql/, the corrected index and the split jobs will be there — it is only existing databases that still depend on the manual steps.

CI was still running at the time of this review; the reasoning above is from reading the code and fixtures rather than from a green build.

[ClemRz]

Addressed the nitpick from the third review round: renamed safeDBQuery → querySpatialRows in api/services/MassifService.js, with a comment explaining the rationale (spatial queries using ST_Contains throw rather than return empty when point_geom is null). Both call sites (getCaves, getNetworks) updated accordingly.