Issue #1981: WIP MultiProvider RC extension #2007

abdulazizali77 · 2017-03-29T08:30:59Z

@gvagenas @deruelle I thought about possible ways to make account specific mappings to RC configurations with the currect RC Extensions API.

Abstractly i believe any plugin/extension API should allow its consumers to modify aspects of the system at predefined levels/modules. In reference to SMSC MProc API, it allows Messages and anything related to the Message, network id etc to be customized.
I believe with RC we can abstract that out even more, from Message to Session to Transaction to SystemNode to System, which is why i decided to subclass the ExtensionsResponse as i did.

Implementation wise, we couldve chosen between two options

Pass in an/several Interface/s to the Extension handler interface and expose certain parts of our System/Node/Tx/Session/Message to be customized by the extension provider/implementor.
This is sort of how SMSC does it, it passes specific factories in specific handlers which expose specific configurations to be modified by the MProcRule implementations.
Return specific class/Interface/es to the calling method and handle appropriately. The class/interface would be subclasses/subinterfaces of the ExtensionResponse and RC would handle them all differently.

In the PR changes ive opted to go with 2) so we can leverage the existing ExtensionResponse.
Additionally we also need to pass in more information to the extension than just a message object, because in our example of the MultiProvider specification it needs to map Account to custom database configurations. So abstractly we also need to create a new class/interface to pass in to the handlers, maybe something like ExtensionRequest ?
What do you think? If you think this is in sort of the correct direction, i will proceed with defining an ExtensionRequest and writing the actual MultiProvider extension itself.

deruelle · 2017-03-29T12:58:24Z

@abdulazizali77 that seems to make sense to me for defining multiple layers of Response but I think we should really describe what each level means in the context of Restcomm so it's clear and documentation be updated on that as well.

Seems to Makes sense to provide an ExtensionRequest as well to me.

@gvagenas will take the final call on this as he has more insights.

scottbarstow · 2017-04-10T14:38:09Z

restcomm/restcomm.sms/src/main/java/org/restcomm/connect/sms/smpp/SmppMessageHandler.java

 import java.io.IOException;
 import java.net.URI;
 import java.util.List;
 import java.util.concurrent.TimeUnit;

+import javolution.util.FastList;


@abdulazizali77 this looks like a new dependency. Not sure what policy is, just want to raise the issue here.

@scottbarstow unsure myself, should find out. Smsc common-library is dependent on this library so license MIGHT be permissible.
However we can cast this to Collection<Tlv> .

scottbarstow · 2017-04-10T14:39:23Z

restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/entities/SmsMessage.java

@@ -47,13 +46,20 @@
    private final Direction direction;
    private final BigDecimal price;
    private final Currency priceUnit;
+    private final String tlvTag;


Should we support array here, like TlvSet, rather than just a single key/value pair? @abdulazizali77 what do you think?

@scottbarstow I do agree, currently as per your comment on Slack the SmsService only fetches tlv 1550 which is why its only adding one tlv. However the code youre pointing at is the for-test-only Sms REST API tlv modification which im going to clean out.

scottbarstow · 2017-04-10T14:41:14Z

restcomm/restcomm.application/src/main/webapp/WEB-INF/data/hsql/restcomm.script

 CREATE MEMORY TABLE "restcomm_geolocation"("sid" VARCHAR(34) NOT NULL PRIMARY KEY, "date_created" DATETIME NOT NULL, "date_updated" DATETIME NOT NULL, "date_executed" DATETIME NOT NULL, "account_sid" VARCHAR(34) NOT NULL, "source" VARCHAR(30), "device_identifier" VARCHAR(30) NOT NULL, "geolocation_type" VARCHAR(15) NOT NULL, "response_status" VARCHAR(30), "cell_id" VARCHAR(10), "location_area_code" VARCHAR(10), "mobile_country_code" INTEGER, "mobile_network_code" VARCHAR(3), "network_entity_address" BIGINT, "age_of_location_info" INTEGER, "device_latitude" VARCHAR(15), "device_longitude" VARCHAR(15), "accuracy" BIGINT, "physical_address" VARCHAR(50), "internet_address" VARCHAR(50), "formatted_address" VARCHAR(200), "location_timestamp" DATETIME, "event_geofence_latitude" VARCHAR(15), "event_geofence_longitude" VARCHAR(15), "radius" BIGINT, "geolocation_positioning_type" VARCHAR(15), "last_geolocation_response" VARCHAR(10), "cause" VARCHAR(150), "api_version" VARCHAR(10) NOT NULL, "uri" LONGVARCHAR NOT NULL)
 CREATE USER SA PASSWORD ""
 GRANT DBA TO SA
 SET WRITE_DELAY 10
 SET SCHEMA PUBLIC
 INSERT INTO "restcomm_accounts" VALUES('ACae6e420f425248d6a26948c17a9e2acf','2012-04-24 00:00:00.000000000','2012-04-24 00:00:00.000000000','[email protected]','Default Administrator Account',NULL,'Full','uninitialized','77f8c12cc7b8f8423e5c38b035249166','Administrator','/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf')
+INSERT INTO "restcomm_accounts" VALUES('ACae6e420f425248d6a26948c17a9e2acg','2012-04-24 00:00:00.000000000','2012-04-24 00:00:00.000000000','[email protected]','Second Administrator Account',NULL,'Full','uninitialized','77f8c12cc7b8f8423e5c38b035249166','Administrator','/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acg')


Doubt we want to add new default admin accounts just to demonstrate the capabilities of this particular feature. Should have George weigh in on this for sure.

@scottbarstow agreed, this was only for test purposes

Agree with @scottbarstow , we shouldn't add new accounts here

scottbarstow · 2017-04-12T13:15:13Z

@abdulazizali77 are we ready for George to review this PR yet?

abdulazizali77 · 2017-04-13T07:45:05Z

@gvagenas This PR is ready. Please have a look,

maria-farooq · 2017-04-18T08:25:22Z

restcomm/restcomm.application/src/main/webapp/WEB-INF/scripts/mariadb/init.sql

+CREATE TABLE restcomm_accounts_extensions (
+account_sid VARCHAR(34) NOT NULL,
+extension_sid VARCHAR(34) NOT NULL,
+extension_blob CLOB,


@abdulazizali77 CLOB data type, did you chcked is it is supported in mySql

maria-farooq · 2017-04-18T08:30:58Z

@abdulazizali77 we need to add upgrade scripts in bitbucket RC to accomodate db schema changes. please check https://bitbucket.org/telestax/telscale-restcomm/src/master/restcomm/configuration/config-scripts/as7-config-scripts/restcomm/upgrade_scripts/?at=master

deruelle · 2017-05-01T12:05:31Z

@gvagenas @abdulazizali77 can this be merged ?

gvagenas

@abdulazizali77 please check my comments for this PR.

Also, since we have issue for all pending tasks/TODO/FIXME and other comments, lets remove them before we merge to master.

Thanks
George

gvagenas · 2017-05-04T08:45:50Z

restcomm/restcomm.application/src/main/webapp/WEB-INF/data/hsql/restcomm.script

 CREATE MEMORY TABLE "restcomm_geolocation"("sid" VARCHAR(34) NOT NULL PRIMARY KEY, "date_created" DATETIME NOT NULL, "date_updated" DATETIME NOT NULL, "date_executed" DATETIME NOT NULL, "account_sid" VARCHAR(34) NOT NULL, "source" VARCHAR(30), "device_identifier" VARCHAR(30) NOT NULL, "geolocation_type" VARCHAR(15) NOT NULL, "response_status" VARCHAR(30), "cell_id" VARCHAR(10), "location_area_code" VARCHAR(10), "mobile_country_code" INTEGER, "mobile_network_code" VARCHAR(3), "network_entity_address" BIGINT, "age_of_location_info" INTEGER, "device_latitude" VARCHAR(15), "device_longitude" VARCHAR(15), "accuracy" BIGINT, "physical_address" VARCHAR(50), "internet_address" VARCHAR(50), "formatted_address" VARCHAR(200), "location_timestamp" DATETIME, "event_geofence_latitude" VARCHAR(15), "event_geofence_longitude" VARCHAR(15), "radius" BIGINT, "geolocation_positioning_type" VARCHAR(15), "last_geolocation_response" VARCHAR(10), "cause" VARCHAR(150), "api_version" VARCHAR(10) NOT NULL, "uri" LONGVARCHAR NOT NULL)
 CREATE USER SA PASSWORD ""
 GRANT DBA TO SA
 SET WRITE_DELAY 10
 SET SCHEMA PUBLIC
 INSERT INTO "restcomm_accounts" VALUES('ACae6e420f425248d6a26948c17a9e2acf','2012-04-24 00:00:00.000000000','2012-04-24 00:00:00.000000000','[email protected]','Default Administrator Account',NULL,'Full','uninitialized','77f8c12cc7b8f8423e5c38b035249166','Administrator','/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf')
+INSERT INTO "restcomm_accounts" VALUES('ACae6e420f425248d6a26948c17a9e2acg','2012-04-24 00:00:00.000000000','2012-04-24 00:00:00.000000000','[email protected]','Second Administrator Account',NULL,'Full','uninitialized','77f8c12cc7b8f8423e5c38b035249166','Administrator','/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acg')


Agree with @scottbarstow , we shouldn't add new accounts here

gvagenas · 2017-05-04T08:48:26Z

restcomm/restcomm.application/src/main/webapp/WEB-INF/data/hsql/restcomm.script

@@ -42,3 +45,7 @@ INSERT INTO "restcomm_incoming_phone_numbers" VALUES('PN46678e5b01d44973bf184f65
 INSERT INTO "restcomm_incoming_phone_numbers" VALUES('PNb43ed9e641364277b6432547ff1109e9','2014-02-17 22:37:19.392000000','2014-02-17 22:37:19.392000000','RVD external services app, customer ID 1 or 2 ','ACae6e420f425248d6a26948c17a9e2acf','+1241','2012-04-24',FALSE,NULL,'POST',NULL,'POST',NULL,'POST','AP81cf45088cba4abcac1261385916d582',NULL,'POST',NULL,'POST',NULL,'/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/IncomingPhoneNumbers/PNb43ed9e641364277b6432547ff1109e9',NULL,NULL,NULL,NULL, TRUE,'0.0', NULL, NULL)
 INSERT INTO "restcomm_clients" VALUES('CLa2b99142e111427fbb489c3de357f60a','2013-11-04 12:52:44.144000000','2013-11-04 12:52:44.144000000','ACae6e420f425248d6a26948c17a9e2acf','2012-04-24','alice','alice','1234',1,NULL,'POST',NULL,'POST',NULL,'/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Clients/CLa2b99142e111427fbb489c3de357f60a')
 INSERT INTO "restcomm_clients" VALUES('CL3003328d0de04ba68f38de85b732ed56','2013-11-04 16:33:39.248000000','2013-11-04 16:33:39.248000000','ACae6e420f425248d6a26948c17a9e2acf','2012-04-24','bob','bob','1234',1,NULL,'POST',NULL,'POST',NULL,'/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Clients/CL3003328d0de04ba68f38de85b732ed56')
+INSERT INTO "restcomm_accounts_extensions" VALUES('ACae6e420f425248d6a26948c17a9e2acf','EX00000000000000000000000000000005','{"outbound-sms": [{"destination_network_id": "1000"}]}')


@abdulazizali77 we shouldn't add any configuration specific to the extension here. The restcomm_accounts_extensions and restcomm_extensions_configuration records should be removed.

@gvagenas removed

gvagenas · 2017-05-04T08:50:19Z

restcomm/restcomm.application/pom.xml

-
+        <dependency>
+            <groupId>org.apache.maven</groupId>
+            <artifactId>maven-artifact</artifactId>


@abdulazizali77 why we need this new dependency?

@gvagenas removed, it was initially because the ExtensionConfiguration requires it

gvagenas · 2017-05-04T08:55:03Z

restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/ExtensionsConfigurationDao.java

@@ -97,4 +97,16 @@
     * @return
     */
    boolean validate(ExtensionConfiguration extensionConfiguration);
+
+    /**/


@abdulazizali77 please add javadoc comments for the new methods, a short description will be fine

@gvagenas added

gvagenas · 2017-05-04T09:34:17Z

...extension.api/src/main/java/org/restcomm/connect/extension/api/MessageExtensionResponse.java

@@ -0,0 +1,3 @@
+package org.restcomm.connect.extension.api;
+public class MessageExtensionResponse extends ExtensionResponse {


@abdulazizali77 please add short javadoc description here

@gvagenas added

gvagenas · 2017-05-04T19:04:41Z

restcomm/restcomm.sms/src/main/java/org/restcomm/connect/sms/SmsService.java

@@ -208,8 +218,8 @@ private void message(final Object message) throws IOException {

                final SipServletResponse trying = request.createResponse(SipServletResponse.SC_TRYING);
                trying.send();
-
-                ActorRef session = session();
+                //TODO:do extensions check here too?


@abdulazizali77 yes we need extension check here also. Those requests are MESSAGE requests from registered clients that Restcomm will proxy out to.

@gvagenas I want to address this in a new Issue/PR because this Use Case was not actually covered in our testing.

gvagenas · 2017-05-04T19:06:53Z

restcomm/restcomm.sms/src/main/java/org/restcomm/connect/sms/SmsSession.java

+                //FIXME:fetch the tag value from the arbitrary key
+                this.tlvSet.addOptionalParameter(new Tlv(SmppConstants.TAG_DEST_NETWORK_ID,ByteArrayUtil.toByteArray(Integer.parseInt(valStr))));
+            } catch (Exception e) {
+                // TODO: handle exception


@abdulazizali77 please add a logger.error(...) here

@gvagenas added.

gvagenas · 2017-05-04T19:27:22Z

...m/restcomm.http/src/main/java/org/restcomm/connect/http/ExtensionsConfigurationEndpoint.java

+            }
+        }
+
+        if (accountSid!=null) {


@abdulazizali77 I don't think this is proper here. As far as I understand, an extension can have a global configuration but also can have a configuration specific to an account. Thus we need to different methods, one to get the global configuration and one to get the configuration for a specific account

@gvagenas I want to address the REST design + implementation issues in #2127

gvagenas · 2017-05-04T19:28:40Z

restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/ExtensionsConfigurationDao.java

@@ -97,4 +97,16 @@
     * @return
     */
    boolean validate(ExtensionConfiguration extensionConfiguration);
+
+    /**/
+    ExtensionConfiguration getAccountExtensionConfiguration(String accountSid, String extensionSid);


@abdulazizali77 please add short description as javadoc comment to each one of the new methods here.

@gvagenas added

gvagenas · 2017-05-04T19:34:27Z

...mm.dao/src/main/java/org/restcomm/connect/dao/mybatis/MybatisExtensionsConfigurationDao.java

+                    map.put("account_sid", DaoUtils.writeSid(accountSid));
+                    map.put("extension_sid", DaoUtils.writeSid(extensionConfiguration.getSid()));
+
+                    if (extensionConfiguration.getConfigurationData() != null)


@abdulazizali77 I believe that first thing we should do is to check that configuration data is not null and then proceed, if data is null we shouldn't attempt to do an insert in the DB.
Also, the check should be done at ExtensionsConfigurationEndpoint for add and update operations

@gvagenas I want to address the REST design + implementation issues in #2127

… to getAccountExtensionConfiguration Issue RestComm#1981: Revert ExtensionsBootStrapper xml parsing Issue RestComm#1981: modify mysql extensions sql Issue RestComm#1981: Fix poms Issue RestComm#1981: Add proper tlvs to outbound and sip request Issue RestComm#1981: WIP. Fix Accounts Extensions DAO. Fix ExtensionConfig to json to XMLConfig mapping. Add example account mappings Issue RestComm#1981: WIP. Dao changes. Table changes. SMSService changes. Issue RestComm#1981: Make changes to SmsSession objects Issue RestComm#1981: Add ExtensionResponse subclasses. Handle Session specific Extension customization by modifying the SmsSession configuration.

…onfiguration

abdulazizali77 · 2017-05-06T12:03:09Z

@gvagenas Please have another look.
-I have addressed most of the comments
-I have added the common Extension handler functions to ExtensionController but have not done this for CallManager.
-I have moved also DefaultExtensionConfiguration from telscale to community
-I have deferred the REST design + implementation issues to be addressed in #2127. I think we should discuss about the design at first.
-I have deferred from adding the extension hook for "MESSAGE" use case as this seems to be a new scenario which was not even encountered during testing. Will open an Issue for this

…Change RestcommExtensionGeneric preOutboundAction method signature. Add getName,getVersion. Fix dependencies. Fix checkstyle. Remove mistaken CallResponse add.

abdulazizali77 · 2017-05-10T02:04:08Z

thanks @gvagenas

otsakir added the Peer Review label Mar 29, 2017

abdulazizali77 requested a review from gvagenas March 29, 2017 08:31

abdulazizali77 self-assigned this Mar 29, 2017

scottbarstow reviewed Apr 10, 2017

View reviewed changes

abdulazizali77 force-pushed the MPROV_1981 branch from 86a2f06 to c163904 Compare April 13, 2017 07:44

maria-farooq reviewed Apr 18, 2017

View reviewed changes

abdulazizali77 force-pushed the MPROV_1981 branch from a68275d to 485af00 Compare April 27, 2017 05:41

gvagenas added this to the George 8.2.0 Sprint 2 milestone May 3, 2017

gvagenas suggested changes May 4, 2017

View reviewed changes

abdulazizali77 added 3 commits May 6, 2017 15:10

Issue RestComm#1981: Add extensions check to SMS inbound hosted app

a023fc5

Issue RestComm#1981: Modify Extensions REST API for acount specific c…

bbef17c

…onfiguration

abdulazizali77 force-pushed the MPROV_1981 branch from 485af00 to 09b166d Compare May 6, 2017 11:48

Issue#1981: Address PR comments

09b166d

Issue#1981: Address PR comments: Move DefaultExtensionConfiguration. …

771d389

…Change RestcommExtensionGeneric preOutboundAction method signature. Add getName,getVersion. Fix dependencies. Fix checkstyle. Remove mistaken CallResponse add.

gvagenas merged commit 771d389 into RestComm:master May 9, 2017

gvagenas removed the Peer Review label May 9, 2017

		@@ -0,0 +1,3 @@
		package org.restcomm.connect.extension.api;
		public class MessageExtensionResponse extends ExtensionResponse {

Issue #1981: WIP MultiProvider RC extension #2007

Issue #1981: WIP MultiProvider RC extension #2007

Conversation

abdulazizali77 commented Mar 29, 2017

deruelle commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbarstow commented Apr 12, 2017

abdulazizali77 commented Apr 13, 2017

maria-farooq Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

maria-farooq commented Apr 18, 2017

deruelle commented May 1, 2017

gvagenas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulazizali77 commented May 6, 2017

abdulazizali77 commented May 10, 2017

maria-farooq Apr 18, 2017 •

edited

Loading