Skip to content

Commit ac3b905

Browse files
committed
fix(caver): address PR review feedback
- Replace TOCTOU SELECT+INSERT with atomic ON CONFLICT in add-explored-entrance - Add unique index to test DB for ON CONFLICT support - Add missing DELETE test for non-existing caverId - Move legacy j_caver_cave_explorer cleanup into Promise.all in delete - Add defensive junction row cleanup in model test outer after hook
1 parent 4e549b7 commit ac3b905

5 files changed

Lines changed: 28 additions & 19 deletions

File tree

api/controllers/v1/caver/add-explored-entrance.js

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,28 +31,22 @@ module.exports = async (req, res) => {
3131
return res.notFound(`Caver with id ${caverId} not found.`);
3232
}
3333

34-
// Check if a relationship already exists
35-
const existingQuery = `
36-
SELECT 1 FROM j_caver_entrance_explorer
37-
WHERE id_entrance = $1 AND id_caver = $2
34+
// Atomic upsert: INSERT with ON CONFLICT eliminates the TOCTOU race
35+
const insertQuery = `
36+
INSERT INTO j_caver_entrance_explorer (id_entrance, id_caver)
37+
VALUES ($1, $2)
38+
ON CONFLICT (id_entrance, id_caver) DO NOTHING
3839
`;
39-
const existingResult = await sails.sendNativeQuery(existingQuery, [
40+
const result = await sails.sendNativeQuery(insertQuery, [
4041
entranceId,
4142
caverId,
4243
]);
4344

44-
if (existingResult.rows.length > 0) {
45+
if (result.rowCount === 0) {
4546
return res.conflict(
4647
'Caver is already registered as an explorer of this entrance.'
4748
);
4849
}
4950

50-
// Create the relationship
51-
const insertQuery = `
52-
INSERT INTO j_caver_entrance_explorer (id_entrance, id_caver)
53-
VALUES ($1, $2)
54-
`;
55-
await sails.sendNativeQuery(insertQuery, [entranceId, caverId]);
56-
5751
return res.status(204).send();
5852
};

api/controllers/v1/caver/delete.js

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,14 +158,13 @@ module.exports = async (req, res) => {
158158
linkedEntitiesDeleteOrMerge('subscribedToRegions'),
159159
linkedEntitiesDeleteOrMerge('grottos'),
160160
linkedEntitiesDeleteOrMerge('documents', allDocumentIds),
161+
// Clean up legacy j_caver_cave_explorer rows (table preserved, no Waterline association)
162+
CommonService.query(
163+
'DELETE FROM j_caver_cave_explorer WHERE id_caver = $1',
164+
[caverId]
165+
),
161166
]);
162167

163-
// Clean up legacy j_caver_cave_explorer rows (table preserved, no Waterline association)
164-
await CommonService.query(
165-
'DELETE FROM j_caver_cave_explorer WHERE id_caver = $1',
166-
[caverId]
167-
);
168-
169168
await TCaver.destroyOne({ id: caverId });
170169

171170
const action = shouldMergeInto ? 'delete+merge' : 'delete';

test/customSQL.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ CREATE INDEX IF NOT EXISTS j_caver_cave_explorer_id_caver_idx ON j_caver_cave_ex
9393
CREATE INDEX IF NOT EXISTS j_caver_cave_explorer_id_cave_idx ON j_caver_cave_explorer (id_cave);
9494
CREATE INDEX IF NOT EXISTS j_caver_entrance_explorer_id_caver_idx ON j_caver_entrance_explorer (id_caver);
9595
CREATE INDEX IF NOT EXISTS j_caver_entrance_explorer_id_entrance_idx ON j_caver_entrance_explorer (id_entrance);
96+
CREATE UNIQUE INDEX IF NOT EXISTS j_caver_entrance_explorer_unique ON j_caver_entrance_explorer (id_entrance, id_caver);
9697
CREATE INDEX IF NOT EXISTS idx_document_id_parent ON t_document(id_parent) WHERE id_parent IS NOT NULL;
9798
CREATE INDEX IF NOT EXISTS idx_document_hierarchy ON t_document(id, id_parent);
9899
CREATE INDEX IF NOT EXISTS idx_t_document_type ON t_document(id_type);

test/integration/0_models/JCaverEntranceExplorer.test.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,14 @@ describe('JCaverEntranceExplorer model associations', () => {
3030
});
3131

3232
after(async () => {
33+
// Clear any lingering junction rows first (idempotent — safe if already gone)
34+
if (testCaver && testEntrance) {
35+
await TCaver.removeFromCollection(
36+
testCaver.id,
37+
'exploredEntrances',
38+
testEntrance.id
39+
);
40+
}
3341
if (testEntrance) await TEntrance.destroyOne({ id: testEntrance.id });
3442
if (testCave) await TCave.destroyOne({ id: testCave.id });
3543
if (testCaver) await TCaver.destroyOne({ id: testCaver.id });

test/integration/4_routes/Cavers/exploredEntrances.test.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,13 @@ describe('Caver explored entrances features', () => {
155155
.expect(404, done);
156156
});
157157

158+
it('should return 404 on non-existing caver', (done) => {
159+
supertest(sails.hooks.http.app)
160+
.delete(`/api/v1/entrances/${testEntrance.id}/cavers/987654321`)
161+
.set('Authorization', moderatorToken)
162+
.expect(404, done);
163+
});
164+
158165
it('should return 404 when relationship does not exist', async () => {
159166
// Remove the relationship first
160167
await sails.sendNativeQuery(

0 commit comments

Comments
 (0)