fix: message_id in submit_sm_resp + SMPP spec compliant DLR stat codes#68
fix: message_id in submit_sm_resp + SMPP spec compliant DLR stat codes#68imidevops wants to merge 4 commits into
Conversation
Updated README to reflect critical fixes for Kannel SMS Gateway and Redis DLR compatibility, including detailed problem descriptions and solutions.
📝 WalkthroughWalkthroughThis pull request refactors the SMPP library's logging infrastructure from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/utils.js (1)
645-665:⚠️ Potential issue | 🟠 Major
splitMsg()now drops one character from every full segment.When
i2 === partCharLimit, the code pushesmsgPart.slice(0, -1)and then reprocesses the removed character. That turns each “full” part intopartCharLimit - 1chars, so a 305-char GSM message becomes 152/152/1 instead of 153/152.🛠️ Suggested fix
if (i2 === partCharLimit) { - msgs.push(defs.encodings[encoding].encode(msgPart.slice(0, - 1))); + msgs.push(defs.encodings[encoding].encode(msgPart)); msgPart = ''; - i --; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils.js` around lines 645 - 665, The splitMsg() loop drops one character from every full segment because when i2 === partCharLimit it pushes defs.encodings[encoding].encode(msgPart.slice(0, -1)) and then decrements i to reprocess the removed char; change this so it pushes the entire msgPart (no slice) for full segments and do not decrement i—i.e., remove the .slice(0, -1) and the i-- adjustment so msgs receives partCharLimit characters and the loop proceeds correctly using msgPart, i2, partCharLimit, msgs and defs.encodings[encoding].encode.lib/defs.js (1)
631-639:⚠️ Potential issue | 🟠 MajorNormalize the input before reading its fields.
These encoders allocate
resultfromvalue.data/value.number/value.displaybefore theBuffer.isBuffer()and string-normalization branches run.broadcast_area_identifier.encode('foo')and any Buffer pass-through will currently throw before they hit the intended fast path.🛠️ Suggested pattern
encode: function(value) { - var result = new Buffer(value.data.length + 1); if (Buffer.isBuffer(value)) { return value; } if (typeof value === 'string') { value = { format: consts.BROADCAST_AREA_FORMAT.NAME, data: value }; } + var result = new Buffer(value.data.length + 1);Apply the same reordering in
billing_identification.encode(),callback_num.encode(), andcallback_num_atag.encode().Also applies to: 657-673, 752-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/defs.js` around lines 631 - 639, The encoders (e.g., broadcast_area_identifier.encode, billing_identification.encode, callback_num.encode, callback_num_atag.encode) currently allocate a result Buffer using value.data/number/display before checking Buffer.isBuffer(value) or normalizing string inputs, which causes throws for raw Buffers or string inputs; fix each encode function by first normalizing the input (handle string -> object with .data/.number/.display as needed) and performing the Buffer.isBuffer(value) pass-through check before allocating result, then allocate result and write format/other fields and copy value.data/number/display into it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/session.js`:
- Around line 385-386: The multipart submit flow currently returns two
conflicting responses per part: the inline sendReturn call in the submit path
(use of sendReturn with generated msgId) and the downstream checkLongSmses path
which emits an sms object containing sendResp/smsResp and pduObjs; update the
code so only one response path is used—remove or guard the per-part sendReturn
(the sendReturn call that sends 'ESME_ROK' with message_id) when
checkLongSmses/longSms handling will emit the sms event, and instead ensure
checkLongSmses emits the correct UUIDs (or returns the pdu-level ids) so
handlers calling sms.sendResp() receive the same IDs; specifically modify the
logic around sendReturn in the submit handler and the emission in checkLongSmses
(and related smsResp/pduObjs formation) so multipart messages send a single
consistent submit_sm_resp per part with matching IDs.
- Around line 349-357: In sendLongSms's inner that.send callback, guard against
err before dereferencing retPduObj by short-circuiting on error: if (err) { if
(typeof callback === 'function') callback(err); return; } — only then access
retPduObj.params.message_id and push to smsIds/retPduObjs and continue to call
the parent callback when smsIds.length === msgs.length; update the callback
invocation to ensure errors are propagated once and no undefined retPduObj is
accessed.
- Around line 496-507: session currently assumes its argument is a raw socket
and immediately reads sock.remoteAddress and registers sock.on, breaking callers
that pass an options wrapper (e.g., { sock, log }). Add a compatibility shim at
the start of the session function: detect if the argument is an object with a
.sock property and, if so, extract sock = arg.sock and optionally log = arg.log
(or preserve existing log variable), otherwise keep the existing behavior;
ensure you guard uses of sock (remoteAddress/remotePort) and event registration
against undefined to maintain backward compatibility with callers still passing
a plain socket and with existing callers that pass { sock, log } and expect
session() to behave the same.
In `@lib/utils.js`:
- Around line 714-750: The status parsing in smsDlr is collapsing legacy aliases
(e.g. "UNDELIVERABLE") into numeric 2 and then hardcoding the receipt body;
update smsDlr's status normalization to (1) accept legacy string aliases by
mapping known old names to the canonical text/ID (use defs.consts.MESSAGE_STATE
and defs.constsById.MESSAGE_STATE to resolve both directions), (2) keep status
as either a numeric id or canonical text as needed, and (3) when composing
shortMessage use the canonical status text (e.g. compute a serializedStat
variable from defs.constsById.MESSAGE_STATE[status] or
defs.consts.MESSAGE_STATE[status] and append ' stat:'+serializedStat+' err:...'
instead of always using DELIVRD/UNDELIV) while still defaulting to the
UNDELIVERABLE value if resolution fails; update the stat/err branches to use
that serializedStat rather than hardcoded values.
In `@test/app.js`:
- Around line 14-15: Add redis to package.json dependencies and update the
client initialization in test/app.js to use the v4 API: replace the legacy
redis.createClient('6379','host-IP') pattern with creating the client via
redis.createClient({ host: 'host-IP', port: 6379 }) and call await
client.connect() after creation; ensure the module import variable name (redis)
remains used and handle the returned client (client) lifetime and errors
appropriately.
- Around line 95-111: The request callback currently only calls
sms.sendDlr('DELIVRD') on a 200 body match and logs errors otherwise; when
sms.dlr is true you must also send a terminal DLR for failures. Update the
request(...) callback (the anonymous function handling error, response, body) so
that in the else branch (non-200 responses or request errors) you set
accessLog.apiResponse to the error/response info and, if sms.dlr === true, call
sms.sendDlr('UNDELIV') before log.info(accessLog) to ensure transport failures
trigger the UNDELIV delivery receipt.
---
Outside diff comments:
In `@lib/defs.js`:
- Around line 631-639: The encoders (e.g., broadcast_area_identifier.encode,
billing_identification.encode, callback_num.encode, callback_num_atag.encode)
currently allocate a result Buffer using value.data/number/display before
checking Buffer.isBuffer(value) or normalizing string inputs, which causes
throws for raw Buffers or string inputs; fix each encode function by first
normalizing the input (handle string -> object with .data/.number/.display as
needed) and performing the Buffer.isBuffer(value) pass-through check before
allocating result, then allocate result and write format/other fields and copy
value.data/number/display into it.
In `@lib/utils.js`:
- Around line 645-665: The splitMsg() loop drops one character from every full
segment because when i2 === partCharLimit it pushes
defs.encodings[encoding].encode(msgPart.slice(0, -1)) and then decrements i to
reprocess the removed char; change this so it pushes the entire msgPart (no
slice) for full segments and do not decrement i—i.e., remove the .slice(0, -1)
and the i-- adjustment so msgs receives partCharLimit characters and the loop
proceeds correctly using msgPart, i2, partCharLimit, msgs and
defs.encodings[encoding].encode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ea9321d-62a3-4755-a202-f6a0b26cccb5
📒 Files selected for processing (5)
README.mdlib/defs.jslib/session.jslib/utils.jstest/app.js
| that.send(pduObj, function(err, retPduObj) { | ||
| smsIds.push(retPduObj.params.message_id); | ||
| retPduObjs.push(retPduObj); | ||
|
|
||
| that.log.silly(logPrefix + 'Got cb from that.send()'); | ||
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - Got callback from that.send()'); | ||
|
|
||
| if (typeof cb === 'function' && smsIds.length === msgs.length) { | ||
| that.log.silly(logPrefix + 'All cbs returned, run the parent cb.'); | ||
| cb(err, smsIds, retPduObjs); | ||
| if (typeof callback === 'function' && smsIds.length === msgs.length) { | ||
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - All callbacks returned, run the parent callback.'); | ||
| callback(err, smsIds, retPduObjs); |
There was a problem hiding this comment.
Guard the multipart callback path before dereferencing retPduObj.
If one that.send() call fails, retPduObj is undefined here and this callback throws before the original error can propagate. Please short-circuit on err before touching retPduObj.params.message_id.
🛠️ Minimum fix
that.send(pduObj, function(err, retPduObj) {
+ if (err) {
+ if (typeof callback === 'function') {
+ callback(err);
+ }
+ return;
+ }
smsIds.push(retPduObj.params.message_id);
retPduObjs.push(retPduObj);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| that.send(pduObj, function(err, retPduObj) { | |
| smsIds.push(retPduObj.params.message_id); | |
| retPduObjs.push(retPduObj); | |
| that.log.silly(logPrefix + 'Got cb from that.send()'); | |
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - Got callback from that.send()'); | |
| if (typeof cb === 'function' && smsIds.length === msgs.length) { | |
| that.log.silly(logPrefix + 'All cbs returned, run the parent cb.'); | |
| cb(err, smsIds, retPduObjs); | |
| if (typeof callback === 'function' && smsIds.length === msgs.length) { | |
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - All callbacks returned, run the parent callback.'); | |
| callback(err, smsIds, retPduObjs); | |
| that.send(pduObj, function(err, retPduObj) { | |
| if (err) { | |
| if (typeof callback === 'function') { | |
| callback(err); | |
| } | |
| return; | |
| } | |
| smsIds.push(retPduObj.params.message_id); | |
| retPduObjs.push(retPduObj); | |
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - Got callback from that.send()'); | |
| if (typeof callback === 'function' && smsIds.length === msgs.length) { | |
| log.silly('larvitsmpp: lib/session.js: sendLongSms() - All callbacks returned, run the parent callback.'); | |
| callback(err, smsIds, retPduObjs); |
🧰 Tools
🪛 ESLint
[error] 349-349: Missing space before function parentheses.
(space-before-function-paren)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/session.js` around lines 349 - 357, In sendLongSms's inner that.send
callback, guard against err before dereferencing retPduObj by short-circuiting
on error: if (err) { if (typeof callback === 'function') callback(err); return;
} — only then access retPduObj.params.message_id and push to smsIds/retPduObjs
and continue to call the parent callback when smsIds.length === msgs.length;
update the callback invocation to ensure errors are propagated once and no
undefined retPduObj is accessed.
| var msgId = require('uuid').v4(); | ||
| this.sendReturn(pduObj, 'ESME_ROK', {'message_id': msgId}, false); |
There was a problem hiding this comment.
Multipart submits now have two conflicting response paths.
Line 386 already sends a submit_sm_resp for each part, but checkLongSmses() still emits an sms object with sendResp: smsResp and the original pduObjs. Existing handlers in README.md Line 185 and test/app.js Line 68 call sms.sendResp() on every sms event, so long messages will send a second response per part—and the later ids (smsId-2, smsId-3, …) will not match the UUIDs already returned from longSms().
Also applies to: 452-465
🧰 Tools
🪛 ESLint
[error] 385-385: Combine this with the previous 'var' statement.
(one-var)
[error] 385-385: Unexpected var, use let or const instead.
(no-var)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/session.js` around lines 385 - 386, The multipart submit flow currently
returns two conflicting responses per part: the inline sendReturn call in the
submit path (use of sendReturn with generated msgId) and the downstream
checkLongSmses path which emits an sms object containing sendResp/smsResp and
pduObjs; update the code so only one response path is used—remove or guard the
per-part sendReturn (the sendReturn call that sends 'ESME_ROK' with message_id)
when checkLongSmses/longSms handling will emit the sms event, and instead ensure
checkLongSmses emits the correct UUIDs (or returns the pdu-level ids) so
handlers calling sms.sendResp() receive the same IDs; specifically modify the
logic around sendReturn in the submit handler and the emission in checkLongSmses
(and related smsResp/pduObjs formation) so multipart messages send a single
consistent submit_sm_resp per part with matching IDs.
| function session(sock) { | ||
| var returnObj = new events.EventEmitter(); | ||
|
|
||
| if (! options.log) { | ||
| options.log = new lUtils.Log(); | ||
| } | ||
| utils.log = options.log; | ||
| log.silly('larvitsmpp: lib/session.js: session() - New session started from ' + sock.remoteAddress + ':' + sock.remotePort); | ||
|
|
||
| returnObj.log = options.log; | ||
| returnObj.loggedIn = false; | ||
|
|
||
| returnObj.log.silly(logPrefix + 'New session started'); | ||
| // Sequence number used for commands initiated from us | ||
| returnObj.ourSeqNr = 1; | ||
|
|
||
| returnObj.loggedIn = false; | ||
| returnObj.ourSeqNr = 1; // Sequence number used for commands initiated from us | ||
| returnObj.sock = options.sock; // Make the socket transparent via the returned emitter | ||
| returnObj.incOurSeqNr = incOurSeqNr; | ||
| returnObj.closeSocket = closeSocket; | ||
| returnObj.sockWrite = sockWrite; | ||
| returnObj.send = send; | ||
| returnObj.sendReturn = sendReturn; | ||
| returnObj.sendSms = sendSms; | ||
| returnObj.utils = utils; | ||
| // Make the socket transparent via the returned emitter | ||
| returnObj.sock = sock; |
There was a problem hiding this comment.
Keep session() compatible with the current callers.
lib/client.js Line 64 and lib/server.js Line 100 still pass session({ sock, log }). This implementation immediately reads sock.remoteAddress and then registers sock.on(...), so those callers will now hand in a plain wrapper object and fail during session setup.
🛠️ Suggested compatibility shim
function session(sock) {
- var returnObj = new events.EventEmitter();
+ var returnObj = new events.EventEmitter();
+ sock = sock.sock || sock;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function session(sock) { | |
| var returnObj = new events.EventEmitter(); | |
| if (! options.log) { | |
| options.log = new lUtils.Log(); | |
| } | |
| utils.log = options.log; | |
| log.silly('larvitsmpp: lib/session.js: session() - New session started from ' + sock.remoteAddress + ':' + sock.remotePort); | |
| returnObj.log = options.log; | |
| returnObj.loggedIn = false; | |
| returnObj.log.silly(logPrefix + 'New session started'); | |
| // Sequence number used for commands initiated from us | |
| returnObj.ourSeqNr = 1; | |
| returnObj.loggedIn = false; | |
| returnObj.ourSeqNr = 1; // Sequence number used for commands initiated from us | |
| returnObj.sock = options.sock; // Make the socket transparent via the returned emitter | |
| returnObj.incOurSeqNr = incOurSeqNr; | |
| returnObj.closeSocket = closeSocket; | |
| returnObj.sockWrite = sockWrite; | |
| returnObj.send = send; | |
| returnObj.sendReturn = sendReturn; | |
| returnObj.sendSms = sendSms; | |
| returnObj.utils = utils; | |
| // Make the socket transparent via the returned emitter | |
| returnObj.sock = sock; | |
| function session(sock) { | |
| var returnObj = new events.EventEmitter(); | |
| sock = sock.sock || sock; | |
| log.silly('larvitsmpp: lib/session.js: session() - New session started from ' + sock.remoteAddress + ':' + sock.remotePort); | |
| returnObj.loggedIn = false; | |
| // Sequence number used for commands initiated from us | |
| returnObj.ourSeqNr = 1; | |
| // Make the socket transparent via the returned emitter | |
| returnObj.sock = sock; |
🧰 Tools
🪛 ESLint
[error] 497-497: Unexpected var, use let or const instead.
(no-var)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/session.js` around lines 496 - 507, session currently assumes its
argument is a raw socket and immediately reads sock.remoteAddress and registers
sock.on, breaking callers that pass an options wrapper (e.g., { sock, log }).
Add a compatibility shim at the start of the session function: detect if the
argument is an object with a .sock property and, if so, extract sock = arg.sock
and optionally log = arg.log (or preserve existing log variable), otherwise keep
the existing behavior; ensure you guard uses of sock (remoteAddress/remotePort)
and event registration against undefined to maintain backward compatibility with
callers still passing a plain socket and with existing callers that pass { sock,
log } and expect session() to behave the same.
| if (status === undefined || status === true || status === 2 || status === 'true') { | ||
| status = 2; | ||
| status = 2; | ||
| } else if (defs.consts.MESSAGE_STATE[status] !== undefined) { | ||
| status = defs.consts.MESSAGE_STATE[status]; | ||
| status = defs.consts.MESSAGE_STATE[status]; | ||
| } else if (defs.constsById.MESSAGE_STATE[status]) { | ||
| status = parseInt(status); | ||
| status = parseInt(status); | ||
| } else { | ||
| status = 5; // UNDELIVERABLE | ||
| status = 2; // UNDELIVERABLE | ||
| } | ||
|
|
||
| if (typeof cb !== 'function') { | ||
| cb = function () {}; | ||
| if (typeof callback !== 'function') { | ||
| callback = function() {}; | ||
| } | ||
|
|
||
| if (sms.smsId === undefined) { | ||
| const err = new Error('Trying to send DLR with no smsId.'); | ||
| exports.log.warn(logPrefix + err.message); | ||
| return cb(err); | ||
| err = new Error('Trying to send DLR with no smsId.'); | ||
| log.warn('larvitsmpp: lib/utils.js: smsDlr() - ' + err.message); | ||
| callback(err); | ||
| return; | ||
| } | ||
|
|
||
| shortMessage = 'id:' + sms.smsId + ' sub:001 '; | ||
|
|
||
| if (status === 2) { | ||
| shortMessage += 'dlvrd:1 '; | ||
| shortMessage += 'dlvrd:1 '; | ||
| } else { | ||
| shortMessage += 'dlvrd:0 '; | ||
| shortMessage += 'dlvrd:0 '; | ||
| } | ||
|
|
||
| shortMessage += 'submit date:' + smppDate(sms.submitTime); | ||
| shortMessage += ' done date:' + smppDate(new Date()); | ||
| shortMessage += 'submit date:' + smppDate(sms.submitTime); | ||
| shortMessage += ' done date:' + smppDate(new Date()); | ||
|
|
||
| if (status === 2) { | ||
| shortMessage += ' stat:DELIVRD err:0 text:xxx'; | ||
| shortMessage += ' stat:DELIVRD err:000'; | ||
| } else { | ||
| shortMessage += ' stat:UNDELIVERABLE err:1 text:xxx'; | ||
| shortMessage += ' stat:UNDELIV err:001'; | ||
| } |
There was a problem hiding this comment.
Keep the old DLR aliases and serialize the actual status text.
After the MESSAGE_STATE.UNDELIV rename, sms.sendDlr('UNDELIVERABLE') now falls through to status = 2, so it emits a delivered receipt. The same block also hardcodes every non-2 body to stat:UNDELIV, which mis-serializes EXPIRED, REJECTED, ENROUTE, and other valid states. README.md Line 198 still documents UNDELIVERABLE, so this is also a backwards-compat break.
🧰 Tools
🪛 ESLint
[error] 725-725: Missing space before function parentheses.
(space-before-function-paren)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils.js` around lines 714 - 750, The status parsing in smsDlr is
collapsing legacy aliases (e.g. "UNDELIVERABLE") into numeric 2 and then
hardcoding the receipt body; update smsDlr's status normalization to (1) accept
legacy string aliases by mapping known old names to the canonical text/ID (use
defs.consts.MESSAGE_STATE and defs.constsById.MESSAGE_STATE to resolve both
directions), (2) keep status as either a numeric id or canonical text as needed,
and (3) when composing shortMessage use the canonical status text (e.g. compute
a serializedStat variable from defs.constsById.MESSAGE_STATE[status] or
defs.consts.MESSAGE_STATE[status] and append ' stat:'+serializedStat+' err:...'
instead of always using DELIVRD/UNDELIV) while still defaulting to the
UNDELIVERABLE value if resolution fails; update the stat/err branches to use
that serializedStat rather than hardcoded values.
| var redis = require("redis"), | ||
| client = redis.createClient('6379','host-IP'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^package\.json$' | while read -r file; do
printf '%s -> redis=%s\n' "$file" "$(jq -r '.dependencies.redis // .devDependencies.redis // "not declared"' "$file")"
done
echo
rg -n --type js '\bredis\.createClient\s*\('Repository: larvit/larvitsmpp
Length of output: 185
Add redis as a dependency and update the client initialization to use the current API.
redis is not declared in package.json, so the code won't run as-is. Additionally, redis.createClient('6379', 'host-IP') is the legacy v3 API. Modern redis versions (v4+) require a different approach:
const client = redis.createClient({
host: 'host-IP',
port: 6379
});
await client.connect();Add redis to dependencies in package.json and update the client initialization accordingly.
🧰 Tools
🪛 ESLint
[error] 14-15: Combine this with the previous 'var' statement.
(one-var)
[error] 14-15: All 'var' declarations must be at the top of the function scope.
(vars-on-top)
[error] 14-15: Unexpected var, use let or const instead.
(no-var)
[error] 15-15: Expected indentation of 1 tab but found 8 spaces.
(indent)
[error] 15-15: A space is required after ','.
(comma-spacing)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/app.js` around lines 14 - 15, Add redis to package.json dependencies and
update the client initialization in test/app.js to use the v4 API: replace the
legacy redis.createClient('6379','host-IP') pattern with creating the client via
redis.createClient({ host: 'host-IP', port: 6379 }) and call await
client.connect() after creation; ensure the module import variable name (redis)
remains used and handle the returned client (client) lifetime and errors
appropriately.
| request(url, {timeout: 60000} ,function (error, response, body) { | ||
| console.log('body:', body); | ||
| if (!error && response.statusCode == 200) { | ||
| accessLog.apiResponse = body; | ||
| if (sms.dlr === true) { | ||
| if (body && body.indexOf('Successful') !== -1) { | ||
| console.log('DELIVRD'); | ||
| sms.sendDlr('DELIVRD'); | ||
| } else { | ||
| sms.sendDlr('UNDELIV'); | ||
| console.log('UNDELIV'); | ||
| } | ||
| } | ||
| } else { | ||
| accessLog.apiResponse = error; | ||
| } | ||
| log.info(accessLog); |
There was a problem hiding this comment.
Send UNDELIV for transport failures too.
The else branch only logs the failed request. When the submitter asked for a DLR, non-200 responses and request errors currently leave the message without any terminal receipt.
🛠️ Suggested fix
} else {
- accessLog.apiResponse = error;
+ accessLog.apiResponse = error || body;
+ if (sms.dlr === true) {
+ sms.sendDlr('UNDELIV');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request(url, {timeout: 60000} ,function (error, response, body) { | |
| console.log('body:', body); | |
| if (!error && response.statusCode == 200) { | |
| accessLog.apiResponse = body; | |
| if (sms.dlr === true) { | |
| if (body && body.indexOf('Successful') !== -1) { | |
| console.log('DELIVRD'); | |
| sms.sendDlr('DELIVRD'); | |
| } else { | |
| sms.sendDlr('UNDELIV'); | |
| console.log('UNDELIV'); | |
| } | |
| } | |
| } else { | |
| accessLog.apiResponse = error; | |
| } | |
| log.info(accessLog); | |
| request(url, {timeout: 60000} ,function (error, response, body) { | |
| console.log('body:', body); | |
| if (!error && response.statusCode == 200) { | |
| accessLog.apiResponse = body; | |
| if (sms.dlr === true) { | |
| if (body && body.indexOf('Successful') !== -1) { | |
| console.log('DELIVRD'); | |
| sms.sendDlr('DELIVRD'); | |
| } else { | |
| sms.sendDlr('UNDELIV'); | |
| console.log('UNDELIV'); | |
| } | |
| } | |
| } else { | |
| accessLog.apiResponse = error || body; | |
| if (sms.dlr === true) { | |
| sms.sendDlr('UNDELIV'); | |
| } | |
| } | |
| log.info(accessLog); |
🧰 Tools
🪛 ESLint
[error] 95-95: Expected indentation of 2 tabs but found 16 spaces.
(indent)
[error] 95-95: There should be no space before ','.
(comma-spacing)
[error] 95-95: A space is required after ','.
(comma-spacing)
[error] 96-96: Expected indentation of 3 tabs but found 24 spaces.
(indent)
[error] 97-97: Expected indentation of 3 tabs but found 24 spaces.
(indent)
[error] 98-98: Expected indentation of 4 tabs but found 32 spaces.
(indent)
[error] 99-99: Expected indentation of 4 tabs but found 40 spaces.
(indent)
[error] 100-100: Expected indentation of 5 tabs but found 43 spaces.
(indent)
[error] 101-101: Expected indentation of 6 tabs but found 51 spaces.
(indent)
[error] 102-102: Expected indentation of 6 tabs but found 49 spaces.
(indent)
[error] 103-103: Expected indentation of 5 tabs but found 48 spaces.
(indent)
[error] 104-104: Expected indentation of 6 tabs but found 57 spaces.
(indent)
[error] 105-105: Expected indentation of 6 tabs but found 56 spaces.
(indent)
[error] 106-106: Expected indentation of 5 tabs but found 48 spaces.
(indent)
[error] 107-107: Expected indentation of 4 tabs but found 41 spaces.
(indent)
[error] 108-108: Expected indentation of 3 tabs but found 24 spaces.
(indent)
[error] 109-109: Expected indentation of 4 tabs but found 32 spaces.
(indent)
[error] 110-110: Expected indentation of 3 tabs but found 24 spaces.
(indent)
[error] 111-111: Expected indentation of 3 tabs but found 24 spaces.
(indent)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/app.js` around lines 95 - 111, The request callback currently only calls
sms.sendDlr('DELIVRD') on a 200 body match and logs errors otherwise; when
sms.dlr is true you must also send a terminal DLR for failures. Update the
request(...) callback (the anonymous function handling error, response, body) so
that in the else branch (non-200 responses or request errors) you set
accessLog.apiResponse to the error/response info and, if sms.dlr === true, call
sms.sendDlr('UNDELIV') before log.info(accessLog) to ensure transport failures
trigger the UNDELIV delivery receipt.
1 participant
Summary
This PR fixes DLR delivery when larvitsmpp is used as an SMPP server
connected to Kannel SMS Gateway with Redis DLR storage.
Problems & Fixes
1. submit_sm_resp missing message_id (single part)
Kannel requires a message_id in the submit_sm_resp to store the DLR
entry in Redis. Without it, DLRs can never be matched back.
Fix: generate UUID and return it in submit_sm_resp, store as smsId.
2. submit_sm_resp missing message_id (multipart)
longSms() returned early without ever sending submit_sm_resp.
Fix: generate UUID per part inside longSms(), send resp immediately,
store first part's ID on the group for the assembled smsObj.
3. smsId never set on server-side smsObj
smsDlr() silently bails out if smsId is undefined.
Fix: smsId now explicitly set on smsObj in both single and multipart paths.
4. stat:UNDELIVERABLE violates SMPP v3.4 spec
Spec requires stat field to be exactly 7 chars. UNDELIVERABLE is 13.
Kannel's sscanf parser rejects it and treats every DLR as DELIVRD.
Fix: changed to UNDELIV (7 chars) per spec.
Tested
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes
Tests