Skip to content

Commit 35bd18b

Browse files
MartiiMartii
authored
Tag possible error prone code points (#1564)
* These are mostly comments in this PR * Normalized a few more identifiers to their standards for easier grep'ing. I'm sure there's more. * Some of these may be silent as intended... they should be denoted as such... like `fallthrough` for `switch` should be normally * Some of these assume that AWS S3 and MongoLabs are perfectly connected and no issues... bad idea. This is what I was describing. We need [#37](Graceful failure) whether it's a comment, a console message, a page, or some combination of these. * This doesn't include MongoDB calls that just use `save()` for example... which is not recommended at all. Error traps are good when connecting to third parties whether they are local or remote. I'm not perfect either but I can get easily burned out on fixing these which is why I work in stages instead of all at once. Don't get paid for that! ;) :) Always fill out the arguments if known especially with `aErr`. That's a reason why it's done first in this coding style. * This was a very brief skim over the code... hopefully it wasn't overzealous or underzealous in code points... but they are comments to be addressed eventually. Living on four hours of sleep in 3 days isn't good so I'm taking a break not to mention a few days away from holiday days. Applies to #430 and indirectly to #1548 Signed-off-by: Martii <[email protected]> Auto-merge
1 parent 0d61c78 commit 35bd18b

18 files changed

+90
-8
lines changed

‎controllers/admin.js

+10
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ exports.adminUserUpdate = function (aReq, aRes, aNext) {
190190

191191
// Make sure the change is reflected in the session store
192192
updateSessions(aReq, aUser, function (aErr, aSess) {
193+
// WARNING: No err handling
194+
193195
aRes.redirect(user.userPageUri);
194196
});
195197
});
@@ -509,6 +511,8 @@ exports.adminApiKeysPage = function (aReq, aRes, aNext) {
509511
// strategyListQuery
510512
tasks.push(function (aCallback) {
511513
Strategy.find({}, function (aErr, aStrats) {
514+
// WARNING: No err handling
515+
512516
var stored = nil();
513517
var strategies = null;
514518

@@ -556,6 +560,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) {
556560
});
557561

558562
Strategy.find({}, function (aErr, aStrats) {
563+
// WARNING: No err handling
564+
559565
var stored = nil();
560566

561567
aStrats.forEach(function (aStrat) {
@@ -590,6 +596,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) {
590596
}
591597

592598
strategy.save(function (aErr, aStrategy) {
599+
// WARNING: No err handling
600+
593601
loadPassport(aStrategy);
594602
aCallback();
595603
});
@@ -599,6 +607,8 @@ exports.apiAdminUpdate = function (aReq, aRes, aNext) {
599607

600608
aCallback();
601609
}, function (aErr) {
610+
// WARNING: No err handling
611+
602612
aRes.redirect('/admin/api');
603613
});
604614
});

‎controllers/auth.js

+5
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ passport.serializeUser(function (aUser, aDone) {
4343
// Setup all the auth strategies
4444
var openIdStrategies = {};
4545
Strategy.find({}, function (aErr, aStrategies) {
46+
// WARNING: No err handling
4647

4748
// Get OpenId strategies
4849
for (var name in allStrategies) {
@@ -95,6 +96,8 @@ exports.auth = function (aReq, aRes, aNext) {
9596
if (aReq.session.cookie.sameSite !== 'lax') {
9697
aReq.session.cookie.sameSite = 'lax';
9798
aReq.session.save(function (aErr) {
99+
// WARNING: No err handling
100+
98101
authenticate(aReq, aRes, aNext);
99102
});
100103
} else {
@@ -342,6 +345,8 @@ exports.callback = function (aReq, aRes, aNext) {
342345

343346
if (!aReq.session.passport.oujsOptions.authAttach) {
344347
expandSession(aReq, aUser, function (aErr) {
348+
// WARNING: No err handling
349+
345350
aRes.redirect(doneUri);
346351
});
347352
} else {

‎controllers/discussion.js

+6
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,8 @@ function postComment(aUser, aDiscussion, aContent, aCreator, aUserAgent, aCallba
427427
});
428428

429429
comment.save(function (aErr, aComment) {
430+
// WARNING: No err handling
431+
430432
++aDiscussion.comments;
431433
aDiscussion.lastCommentor = aUser.name;
432434
aDiscussion.updated = new Date();
@@ -479,6 +481,8 @@ function postTopic(aUser, aCategory, aTopic, aContent, aIssue, aUserAgent, aCall
479481
newDiscussion = new Discussion(props);
480482

481483
newDiscussion.save(function (aErr, aDiscussion) {
484+
// WARNING: No err handling
485+
482486
// Now post the first comment
483487
postComment(aUser, aDiscussion, aContent, true, aUserAgent, function (aErr, aDiscussion) {
484488
aCallback(aDiscussion);
@@ -566,6 +570,8 @@ exports.createComment = function (aReq, aRes, aNext) {
566570

567571

568572
postComment(authedUser, aDiscussion, content, false, userAgent, function (aErr, aDiscussion) {
573+
// WARNING: No err handling
574+
569575
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
570576
return encodeURIComponent(aStr);
571577
}).join('/') +

‎controllers/flag.js

+2
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ exports.flag = function (aReq, aRes, aNext) {
4343

4444
form = new formidable.IncomingForm();
4545
form.parse(aReq, function (aErr, aFields) {
46+
// WARNING: No err handling
47+
4648
var flag = aFields.flag === 'false' ? false : true;
4749
var reason = null;
4850

‎controllers/group.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ exports.addScriptToGroups = function (aScript, aGroupNames, aCallback) {
129129
});
130130

131131
group.save(function (aErr, aGroup) {
132+
// WARNING: No err handling
133+
132134
aScript._groupId = aGroup._id;
133135
aCallback();
134136
});
@@ -149,7 +151,9 @@ exports.addScriptToGroups = function (aScript, aGroupNames, aCallback) {
149151
aGroup.size = aScripts.length;
150152
aGroup.rating = getRating(aScripts);
151153
aGroup.updated = new Date();
152-
aGroup.save(function () {
154+
aGroup.save(function (aErr, aGroup) {
155+
// WARNING: No err handling
156+
153157
// NOTE: This is a callback that does nothing
154158
});
155159
}

‎controllers/index.js

+4
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ exports.home = function (aReq, aRes) {
107107
getFlaggedListForContent('Script', options, aCallback);
108108
}
109109
], function (aErr) {
110+
// WARNING: No err handling
111+
110112
preRender();
111113
render();
112114
});
@@ -295,6 +297,8 @@ exports.logout = function (aReq, aRes) {
295297
}
296298

297299
User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
300+
// WARNING: No err handling
301+
298302
removeSession(aReq, aUser, function () {
299303
aRes.redirect(redirectUri);
300304
});

‎controllers/issue.js

+4
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,8 @@ exports.comment = function (aReq, aRes, aNext) {
443443

444444
discussionLib.postComment(authedUser, aIssue, content, false, userAgent,
445445
function (aErr, aDiscussion) {
446+
// WARNING: No err handling
447+
446448
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
447449
return encodeURIComponent(aStr);
448450
}).join('/') +
@@ -493,6 +495,8 @@ exports.changeStatus = function (aReq, aRes, aNext) {
493495

494496
if (changed) {
495497
aIssue.save(function (aErr, aDiscussion) {
498+
// WARNING: No err handling
499+
496500
aRes.redirect(aDiscussion.path.split('/').map(function (aStr) {
497501
return encodeURIComponent(aStr);
498502
}).join('/') +

‎controllers/remove.js

+6
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ exports.rm = function (aReq, aRes, aNext) {
4242

4343
form = new formidable.IncomingForm();
4444
form.parse(aReq, function (aErr, aFields) {
45+
// WARNING: No err handling
46+
4547
var reason = aFields.reason;
4648

4749
var type = aReq.params[0];
@@ -87,6 +89,8 @@ exports.rm = function (aReq, aRes, aNext) {
8789
installName: scriptStorage.caseSensitive(installNameBase +
8890
(isLib ? '.js' : '.user.js'))
8991
}, function (aErr, aScript) {
92+
// WARNING: No err handling
93+
9094
removeLib.remove(Script, aScript, authedUser, reason, function (aRemoved) {
9195
if (!aRemoved) {
9296
aNext();
@@ -101,6 +105,8 @@ exports.rm = function (aReq, aRes, aNext) {
101105

102106
User.findOne({ name: { $regex: new RegExp('^' + username + '$', "i") } },
103107
function (aErr, aUser) {
108+
// WARNING: No err handling
109+
104110
removeLib.remove(User, aUser, authedUser, reason, function (aRemoved) {
105111
if (!aRemoved) {
106112
aNext();

‎controllers/script.js

+8
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,8 @@ var getScriptPageTasks = function (aOptions) {
247247
_scriptId: script._id,
248248
_userId: authedUser._id
249249
}, function (aErr, aVoteModel) {
250+
// WARNING: No err handling
251+
250252
aOptions.voteable = !script.isOwner;
251253

252254
if (aVoteModel) {
@@ -359,6 +361,8 @@ exports.view = function (aReq, aRes, aNext) {
359361
getFlaggedListForContent('Script', options, aCallback);
360362
}
361363
], function (aErr) {
364+
// WARNING: No err handling
365+
362366
preRender();
363367
render();
364368
});
@@ -547,13 +551,17 @@ exports.vote = function (aReq, aRes, aNext) {
547551

548552
Vote.findOne({ _scriptId: aScript._id, _userId: authedUser._id },
549553
function (aErr, aVoteModel) {
554+
// WARNING: No err handling
555+
550556
var votes = aScript.votes || 0;
551557
var flags = 0;
552558
var oldVote = null;
553559

554560
function saveScript() {
555561
if (!flags) {
556562
aScript.save(function (aErr, aScript) {
563+
// WARNING: No err handling
564+
557565
var script = null;
558566

559567
if (vote === false) {

‎controllers/scriptStorage.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,7 @@ exports.sendScript = function (aReq, aRes, aNext) {
853853

854854
// Resave affected properties
855855
aScript.save(function (aErr, aScript) {
856-
// WARNING: No error handling at this stage
856+
// WARNING: No err handling
857857
});
858858
}
859859
});
@@ -885,6 +885,8 @@ exports.sendMeta = function (aReq, aRes, aNext) {
885885

886886
Script.findOne({ installName: caseSensitive(installNameBase + '.user.js') },
887887
function (aErr, aScript) {
888+
// WARNING: No err handling
889+
888890
var script = null;
889891
var scriptOpenIssueCountQuery = null;
890892
var whitespace = '\u0020\u0020\u0020\u0020';
@@ -1942,7 +1944,7 @@ exports.deleteScript = function (aInstallName, aCallback) {
19421944
function (aErr, aScript) {
19431945
var s3 = new AWS.S3();
19441946

1945-
// WARNING: No error handling at this stage
1947+
// WARNING: No err handling
19461948

19471949
s3.deleteObject({ Bucket : bucketName, Key : aScript.installName},
19481950
function (aErr) {

‎controllers/user.js

+15
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ exports.extend = function (aReq, aRes, aNext) {
124124
_id: authedUser._id,
125125
sessionIds: { "$in": [ aReq.sessionID ] }
126126
}, function (aErr, aUser) {
127+
// WARNING: No err handling
128+
127129
extendSession(aReq, aUser, function (aErr) {
128130
if (aErr) {
129131
if (aErr === 'Already extended') {
@@ -469,6 +471,8 @@ exports.view = function (aReq, aRes, aNext) {
469471
getFlaggedListForContent('User', options, aCallback);
470472
}
471473
], function (aErr) {
474+
// WARNING: No err handling
475+
472476
preRender();
473477
render();
474478
});
@@ -731,6 +735,8 @@ exports.userScriptListPage = function (aReq, aRes, aNext) {
731735
}
732736
}
733737
], function (aErr) {
738+
// WARNING: No err handling
739+
734740
preRender();
735741
render();
736742
});
@@ -969,6 +975,8 @@ exports.userEditPreferencesPage = function (aReq, aRes, aNext) {
969975
tasks.push(function (aCallback) {
970976
var userStrats = user.strategies.slice(0);
971977
Strategy.find({}, function (aErr, aStrats) {
978+
// WARNING: No err handling
979+
972980
var defaultStrategy = userStrats[userStrats.length - 1];
973981
var name = null;
974982
var strategy = null;
@@ -1652,6 +1660,8 @@ exports.uploadScript = function (aReq, aRes, aNext) {
16521660

16531661
form = new formidable.IncomingForm();
16541662
form.parse(aReq, function (aErr, aFields, aFiles) {
1663+
// WARNING: No err handling
1664+
16551665
//
16561666
var isLib = aReq.params.isLib;
16571667
var script = aFiles.script;
@@ -1695,6 +1705,7 @@ exports.uploadScript = function (aReq, aRes, aNext) {
16951705

16961706
stream.on('end', function () {
16971707
User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
1708+
// WARNING: No err handling
16981709

16991710
var bufferConcat = Buffer.concat(bufs);
17001711

@@ -1764,6 +1775,8 @@ exports.submitSource = function (aReq, aRes, aNext) {
17641775
function storeScript(aMeta, aSource) {
17651776

17661777
User.findOne({ _id: authedUser._id }, function (aErr, aUser) {
1778+
// WARNING: No err handling
1779+
17671780
scriptStorage.storeScript(aUser, aMeta, aSource, false, function (aErr, aScript) {
17681781
var redirectUri = aScript
17691782
? ((aScript.isLib ? '/libs/' : '/scripts/') +
@@ -1815,6 +1828,8 @@ exports.submitSource = function (aReq, aRes, aNext) {
18151828
aScript.fork = fork;
18161829

18171830
aScript.save(function (aErr, aScript) {
1831+
// WARNING: No err handling
1832+
18181833
aRes.redirect(redirectUri);
18191834
});
18201835
});

‎libs/flag.js

+2
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,8 @@ exports.unflag = function (aModel, aContent, aUser, aReason, aCallback) {
210210

211211
function removeFlag(aAuthor) {
212212
aFlag.remove(function (aErr) {
213+
// WARNING: No err handling
214+
213215
saveContent(aModel, aContent, aAuthor, aUser.role < 4 ? -2 : -1, true, aCallback);
214216
});
215217
}

‎libs/markdown.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -238,11 +238,11 @@ marked.setOptions({
238238
if (aLang && hljs.getLanguage(aLang)) {
239239
try {
240240
return hljs.highlight(aLang, aCode).value;
241-
} catch (aErr) {
241+
} catch (aE) {
242242
if (isDev) {
243243
console.error([
244244
colors.red('Dependency named highlighting failed with:'),
245-
aErr
245+
aE
246246

247247
].join('\n'));
248248
}
@@ -264,11 +264,11 @@ marked.setOptions({
264264
}
265265
return hljs.highlightAuto(aCode, lang).value;
266266
}
267-
} catch (aErr) {
267+
} catch (aE) {
268268
if (isDev) {
269269
console.error([
270270
colors.red('Dependency automatic named highlighting failed with:'),
271-
aErr
271+
aE
272272

273273
].join('\n'));
274274
}

‎libs/modelQuery.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ var parseSearchConditions = function (aQ, aPrefixSearchFields, aFullSearchFields
7373
var fullStr = '';
7474
var prefixRegex = null;
7575
var fullRegex = null;
76-
var terms = aQ.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, '\\$1').split(/\s+/).map(function (aE) { return aE.trim(); });
76+
var terms = aQ.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, '\\$1').split(/\s+/).map(function (aEl) { return aEl.trim(); });
7777

7878
// Match all the terms but in any order
7979
terms.forEach(function (aTerm) {

‎libs/modifySessions.js

+4
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ exports.add = function (aReq, aUser, aCallback) {
4747
var store = aReq.sessionStore;
4848

4949
function finish(aErr, aUser) {
50+
// WARNING: No err handling
51+
5052
aReq.session.user = serializeUser(aUser);
5153
aCallback();
5254
}
@@ -268,6 +270,8 @@ exports.getSessionDataList = function (aReq, aOptions, aCallback) {
268270

269271
aInnerCallback();
270272
}], function (aErr) {
273+
// WARNING: No err handling
274+
271275
aCallback(null);
272276
}
273277
);

‎libs/passportVerify.js

+2
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ exports.verify = function (aId, aStrategy, aUsername, aLoggedIn, aDone) {
7272

7373
if (!aUser) {
7474
User.findOne({ 'name': aUsername }, function (aErr, aUser) {
75+
// WARNING: No err handling
76+
7577
if (aUser && aLoggedIn) {
7678
if (allStrategies[aStrategy].readonly) {
7779
aDone(null, false, 'readonly strategy');

0 commit comments

Comments
 (0)