New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup SMPP Message ID correlation for messages waiting for DLR #2965
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we lack coverage in this PR. we can add a unit test to verify that on reception of dlr query is working as expected similar to what we did for submit_sm_response case
private List<String> accountSidSet; // if not-null we need the cdrs that belong to several accounts | ||
private String recipient; | ||
private String sender; | ||
private String startTime; // to initialize it pass string arguments with yyyy-MM-dd format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the reason to change the type from Date to String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as the filter is concerned, the date is a String with some date-like pattern.
If you check the new builder, it accepts a Date which is internally parsed to the desired format. IMO much cleaner and avoids parsing exception when building these objects.
AND "date_created" <= DATE_ADD(#{endTime},INTERVAL 1 DAY) | ||
AND "date_created" <= DATE_ADD(#{endTime},INTERVAL 1 DAY) | ||
</if> | ||
<if test="smppMessageId != null"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to make this change for sms-messages.xml
files in restcomm.application
module as they are the ones that are packaged and used on production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, agree
… in getSmsMessagesByUsingFilters query
@@ -53,4 +54,6 @@ | |||
// Support for filtering of message list result, Issue 1395 | |||
Integer getTotalSmsMessage(SmsMessageFilter filter); | |||
List<SmsMessage> getSmsMessages(SmsMessageFilter filter); | |||
|
|||
List<SmsMessage> findBySmppMessageIdAndDateCreatedGreaterOrEqualThanOrderedByDateCreatedDesc(String smppMessageId, DateTime startDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would prefer a Filter based API approach as in https://github.com/RestComm/Restcomm-Connect/blob/master/restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/IncomingPhoneNumbersDao.java#L48
youll see it supports sorting as well https://github.com/RestComm/Restcomm-Connect/blob/master/restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/entities/IncomingPhoneNumberFilter.java#L34
the long name of this method is already advising a Filter
} | ||
|
||
// Find all messages correlated with SMPP Message ID in last three days | ||
final List<SmsMessage> smsMessages = this.storage.getSmsMessagesDao().findBySmppMessageIdAndDateCreatedGreaterOrEqualThanOrderedByDateCreatedDesc(smppMessageId, DateTime.now().minusDays(3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we agreed this query was actually for SubmitResponse part, not DLR
} else { | ||
// Clean correlation to SMPP Message ID because SMPP identifiers are may repeat after a given time frame | ||
smsMessagesDao.updateSmsMessage(smsMessage.setSmppMessageId(null).setStatus(dLRPayload.getStat())); | ||
for (int index = 0; index < smsCount; index++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this consistency check, i thought it was for submit Resp
What this PR does / why we need it:
Enforces uniqueness in such way that no more than one SMS Message becomes correlated to the same SMPP Message ID.
Which issue(s) this PR fixes:
Fixes BS-232
Special notes for your reviewer:
none