Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implemented IstCommandRequest/Response management. Also some MAP callhandling responses and requests modified to extend of CallHandlingMessage #76

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Collaborator

SummaNetworks commented May 6, 2016

Updated 6 MAP call handling request and responses to extend of CallHandlingMessage.
#75

@deruelle deruelle added the Peer Review label May 6, 2016

@deruelle deruelle added this to the 3.0.0 milestone May 6, 2016

@deruelle deruelle assigned vetss and unassigned brainslog May 6, 2016

Collaborator

vetss commented May 11, 2016 edited

Hello,

thanks for provided updates, I have just checked them.

Here are my remarks:

  1. I have found interface updates for setReportingState-73, istCommand - 88 and releaseResources - 20. May I ask which MAP operations are you going to cover ? (I just want to mark them as in implementation)

  2. you have added only MAPMessageType.istCommand_Request but we need to have also istCommand_Response diffwerently for resquest and response. Also response's getMessageType() must refer to a proper value

  3. Interface definition files like IstCommandRequest have some header info from MAP specification. Their formatting was broken for some reasons and we fix of formatting when we implement a primitive. An example for a proper header you can find in https://github.com/RestComm/jss7/blob/master/map/map-api/src/main/java/org/mobicents/protocols/ss7/map/api/service/callhandling/SendRoutingInformationRequest.java - see a section between and ("" is needed for the format will not broken again in the future). May I ask you to fix it ?

  4. "imsi [0] IMSI, extensionContainer [1] ExtensionContainer OPTIONAL" parameters in IstCommandRequest (unlike IstCommandResponse) are context-specific. Also imsi parameter is NOT optional so we need to check at decoding / encoding steps (if it is not provided we need to throw MAPException). See as an example SendRoutingInformationRequestImpl (msisdn parameter)
    IstCommandResponseImpl encoding / descoding is correct

Collaborator

vetss commented May 11, 2016

  1. we need for each implemented method a unit test for testing on encoding / decoding. You can find examples there:
    https://github.com/RestComm/jss7/blob/master/map/map-impl/src/test/java/org/mobicents/protocols/ss7/map/service/callhandling/SendRoutingInformationRequestTest.java
    https://github.com/RestComm/jss7/blob/master/map/map-impl/src/test/java/org/mobicents/protocols/ss7/map/service/callhandling/SendRoutingInformationResponseTest.java

  2. MAPDialogCallHandlingImpl.addIstCommandRequest() - this operation can be used only in MAP version 3. So you need to leave only version 3 in the code block:
    if ((this.appCntx.getApplicationContextName() != MAPApplicationContextName.ServiceTerminationContext)
    || (vers != MAPApplicationContextVersion.version1 && vers != MAPApplicationContextVersion.version2 && vers != MAPApplicationContextVersion.version3))
    throw new MAPException(
    "Bad application context name for addIstCommandRequest: must be ServiceTerminationContext _V1, V2 or V3");

The same this is also in addIstCommandResponse()

  1. MAPDialogCallHandlingImpl.addIstCommandResponse() - the parameter is optional. This means that if no extensionContainer is provided by a sender we need to skip a parameter body. See an example for it in https://github.com/RestComm/jss7/blob/master/map/map-impl/src/main/java/org/mobicents/protocols/ss7/map/service/sms/MAPDialogSmsImpl.java - addMoForwardShortMessageResponse()

  2. MAPServiceCallHandlingImpl.isServingService() -
    case ServiceTerminationContext: - we need to accept only version 3 here

  3. MAPServiceCallHandlingImpl.getMAPv1ApplicationContext() - we need to remove MAPOperationCode.istCommand from here. This section is only for MAP operations that supports MAP version 1 (this one supports only version 3)

  4. MAPServiceCallHandlingImpl.istCommandResponse() must take into account that a parameter is optional. See as an example MAPServiceSmsImpl.moForwardShortMessageResponse().

Collaborator

vetss commented May 11, 2016

  1. for MAPFunctionalTest we have one hint - you can add (temporary for testing locally !!) the folloing code in the begin of your test:
    saveTrafficInFile();
    This leads that MAPFunctionalTest writes a pcap file with the content which you are transferring. You can open this file in WireShark and check if it decodes what you are going to send.

Please fix mentioned by me remarks and then provide a new pull request for ONE operation, I will check and commit and only then we will be able to go forward.

Collaborator

SummaNetworks commented May 11, 2016

Hello,

I am newbie using GIT, so first of all, sorry for the inconveniences.
I created the PR just after the first commit, and I supposed that this timing should be respected when committing it. I have not asked yet for a new PR of the second commit and I thought that you could commit first PR with its previous commits, and second PR with the remaining commits. As I read you, I understand that is not possible, isn't it? You have to commit all the changes in the fork when committing a PR.

In order to make several PRs and avoid overlapping my PRs with the commits, should be better to create a branch in our fork for each PR?

Thanks in advance.

Owner

deruelle commented May 11, 2016

In order to make several PRs and avoid overlapping my PRs with the commits, should be better to create a branch in our fork for each PR?

Yes please.

@vetss vetss modified the milestones: 3.1.0, 3.0.0 May 13, 2016

Collaborator

SummaNetworks commented May 19, 2016

Hi,
I have committed all changes requested. I only have a doubt about this point:

  1. "imsi [0] IMSI, extensionContainer [1] ExtensionContainer OPTIONAL" parameters in IstCommandRequest (unlike IstCommandResponse) are context-specific.

How do I know that the parameters are context-specific or another class? I could not find this information in the documentation...

Thank you.

@SummaNetworks SummaNetworks changed the title from Some MAP callhandling responses and requests modified to extend of CallHandlingMessage to Implemented IstCommandRequest/Response management. Also some MAP callhandling responses and requests modified to extend of CallHandlingMessage May 19, 2016

Collaborator

vetss commented May 20, 2016

How do I know that the parameters are context-specific or another class?
I could not find this information in the documentation...

  1. The record
    imsi IMSI
    means that ASN Tag class is CLASS_UNIVERSAL (0) and Tag depends on IMSI element. You can find in spec that "IMSI ::= TBCD-STRING (SIZE (3..8))" and "TBCD-STRING ::= OCTET STRING". So tag in this case is "STRING_OCTET" (4).

  2. THE RECORD
    imsi [0] IMSI
    means that ASN Tag class is CLASS_CONTEXT_SPECIFIC (2) and Tag==0 (a value from "[0]")

@vetss vetss modified the milestones: 3.0.0, 3.1.0 May 20, 2016

Collaborator

vetss commented May 20, 2016

I have committed your update into both master and netty-2 branches. Thans for your work.

Also I have committed some little mostly tuning update "Minor updates after IstCommand MAP operation introducing". Before you continue please synch with master branch and check it.

I am closing this pull request now.

@vetss vetss closed this May 20, 2016

@deruelle deruelle removed the Peer Review label May 20, 2016

@vetss vetss added the enhancement label May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment