Sy Protocol Implementation by aferreiraguido · Pull Request #57 · RestComm/jdiameter

aferreiraguido · 2016-09-08T16:08:54Z

No description provided.

+ jdiameter-api/sy interfaces

+ jdiameter-parent/examples/resources/dictionary.xml + defined OC-Supported-Features codes + added AVP definitions

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP

FerUy · 2016-09-08T16:24:58Z

Related to issue #44

deruelle · 2016-09-08T16:31:20Z

Thanks @aferreiraguido !

@brainslog @FerUy can you review ?

FerUy · 2016-09-08T16:40:31Z

Hi @deruelle, work is not finished yet, right @aferreiraguido ?

aferreiraguido · 2016-09-08T21:46:03Z

actually not. there was added api interfaces and AVP definitions to dictionary.xml.- Yet to be implemented is Common, Client and Server java code.

FerUy · 2016-09-08T21:58:23Z

Understood. Thanks for the feedback @aferreiraguido

aferreiraguido · 2016-09-08T22:20:57Z

Please see this is not a request for a merge, the Sy implementation is missing, I thought this pull will allow @brainslog to review the work done until now, as I need some feedback to be sure about the implementation on Server, Client and Common classes as well.

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP issue #44

brainslog · 2016-09-19T03:11:04Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySession.java

@@ -0,0 +1,78 @@
+ /*


Please use TeleStax only license for new files:

/* * TeleStax, Open Source Cloud Communications * Copyright 2011-2016, Telestax Inc and individual contributors * by the @authors tag. * * This program is free software: you can redistribute it and/or modify * under the terms of the GNU Affero General Public License as * published by the Free Software Foundation; either version 3 of * the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU Affero General Public License for more details. * * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see <http://www.gnu.org/licenses/> */

brainslog · 2016-09-19T03:22:45Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySession.java

+import org.jdiameter.api.app.StateMachine;
+
+import org.jdiameter.api.sy.events.SpendingLimitRequest;
+import org.jdiameter.api.sy.events.SpendingTerminationRequest;


This class doesn't seem to be present in the commits..

brainslog · 2016-09-19T03:23:09Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySession.java

+  void sendIntermediateSpendingLimitReportRequest(SpendingLimitRequest request)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+  void sendFinalSpendingLimitReportRequest(SpendingTerminationRequest request)


Do we need a different event type for the Termination ? Why can't we just use the generic SpendingLimitRequest ?

nop, you're right. I will reuse org.jdiameter.api.auth.events.SessionTermRequest on method sendFinalSpendingLimitRequest(), as this is not a SpendingLimitRequest but a termination session request.

brainslog · 2016-09-19T03:25:29Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySession.java

+  void sendFinalSpendingLimitReportRequest(SpendingTerminationRequest request)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+  void sendSpendingLimitReportAnswer(SpendingNotificationAnswer answer)


Shouldn't this method be sendSpendingStatusNotificationAnswer ?
Also, the event class name should also be SpendingStatusNotificationAnswer instead of SpendingNotificationAnswer, IMO, to be according with the spec message name.

yes, my fault.

brainslog · 2016-09-19T03:26:30Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySessionListener.java

+import org.jdiameter.api.sy.events.SpendingLimitRequest;
+import org.jdiameter.api.sy.events.SpendingLimitAnswer;
+import org.jdiameter.api.sy.events.SpendingTerminationRequest;
+import org.jdiameter.api.sy.events.SpendingTerminationAnswer;


Missing class.

brainslog · 2016-09-19T03:27:57Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySessionListener.java

+  void doIntermediateSpendingLimitReportAnswer(ClientSySession session, SpendingLimitRequest request, SpendingLimitAnswer answer)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+  void doFinalSpendingLimitReportAnswer(ClientSySession session, SpendingTerminationRequest request, SpendingTerminationAnswer answer)


Same comment as above, regarding using generic SpendingLimitRequest/SpendingLimitAnswer.

noted and fixed as well.

brainslog · 2016-09-19T03:33:56Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySessionListener.java

+  void doFinalSpendingLimitReportAnswer(ClientSySession session, SpendingTerminationRequest request, SpendingTerminationAnswer answer)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+  void doSpendingLimitReportRequest(ClientSySession session, SpendingNotificationAnswer answer)


void doSpendingLimitReportRequest(ClientSySession session, SpendingStatusNotificationRequest request)?

method name should be doSpendingLimitReportRequest;

2nd parameter should be a Request and not an Answer;

event name should be SpendingStatusNotificationRequest;

Right ?

my fault, should be doSpendingStatusNotificationRequest()

brainslog · 2016-09-19T03:35:14Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ServerSySession.java

+  void sendSpendingLimitReportRequest(SpendingNotificationRequest request)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+}


Same comments as in Client.

brainslog · 2016-09-19T03:41:25Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ClientSySession.java

+ */
+public interface ClientSySession extends AppSession, StateMachine {
+
+  void sendInitialSpendingLimitReportRequest(SpendingLimitRequest request)


Not sure if the method name should not follow the event name, ie sendInitialSpendingLimitRequest.. I see that the spec often refers to it as you've used:

(...) uses the Initial Spending Limit Report Request procedure.
(...) trigger the PCRF sending the Initial/Intermediate/Final Spending Limit Report Request to the OCS (...)

WDYT ?

right! INITIAL and INTERMEDIATE comes in AVP.SL-Request-Type

brainslog · 2016-09-19T03:43:57Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ServerSySessionListener.java

+*/
+public interface ServerSySessionListener {
+
+  void doInitialSpendingLimitReportRequest(ClientSySession session, SpendingLimitRequest request, SpendingLimitAnswer answer)


do*Request methods only take the Request object as parameter, since those are the methods that will handle a received request, where we'll have no answer yet.

brainslog · 2016-09-19T03:45:13Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ServerSySessionListener.java

+  void doFinalSpendingLimitReportRequest(ClientSySession session, SpendingTerminationRequest request, SpendingTerminationAnswer answer)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+  void doSpendingLimitReportAnswer(ClientSySession session, SpendingNotificationAnswer answer)


.. and do*Answer methods take the Request and Answer objects as parameters, as we have both at this time and can pass them to the listener.

brainslog · 2016-09-19T03:45:35Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/ServerSySessionListener.java

+  void doSpendingLimitReportAnswer(ClientSySession session, SpendingNotificationAnswer answer)
+      throws InternalException, IllegalDiameterStateException, RouteException, OverloadException;
+
+}


same comments as in Client.

brainslog · 2016-09-19T03:47:36Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/events/SessionTerminationAnswer.java

@@ -0,0 +1,62 @@
+/*


Why not reuse https://github.com/RestComm/jdiameter/blob/master/core/jdiameter/api/src/main/java/org/jdiameter/api/auth/events/SessionTermAnswer.java ?

brainslog · 2016-09-19T03:48:00Z

core/jdiameter/api/src/main/java/org/jdiameter/api/sy/events/SessionTerminationRequest.java

@@ -0,0 +1,62 @@
+/*


Why not reuse https://github.com/RestComm/jdiameter/blob/master/core/jdiameter/api/src/main/java/org/jdiameter/api/auth/events/SessionTermRequest.java ?

brainslog · 2016-09-19T03:48:56Z

core/jdiameter/impl/pom.xml

        <filtering>true</filtering>
      </resource>
    </resources>
+      <plugins>


Please don't enforce this here, we have added similar @ https://github.com/RestComm/jdiameter/blob/master/pom.xml#L194-L201

brainslog · 2016-09-19T03:52:12Z

examples/charging-server-simulator/src/main/resources/dictionary.xml


  <!-- ************************ END 3GPP S13/S13' AVPS ******************* -->

+  <!-- ***************************** 3GPP Sy AVPS ************************ -->


This should be modified in all the instances of the dictionary.xml file, I understand it's not worth to propagate while not finished, but we should not forget about it. Main file is https://github.com/RestComm/jdiameter/blob/master/core/mux/common/config/dictionary.xml

brainslog · 2016-09-19T03:54:59Z

@aferreiraguido, good job so far! Thanks for the hard work.

I have made some inline comments in the changes, please review them and let me know your thoughts and/or fix where applicable.

@brainslog

+ fixed according fo @brainslog comments #issue 44

issue #44

+renamed jdiameter/client/api/ISessionFactory.registerAppFac<t>ory() issue #44

added testsuite/tests/sy/... revert example/charging-server-simulator issue #44 added AVP's for Ro messages issue #43

brainslog · 2017-03-20T19:25:24Z

Hi @aferreiraguido!

Sorry for the late review, but here are some of my notes. I thought about doing them inline, but I think it's easier to follow on a single comment, let me know if you have any doubts regarding what I am referring to!

@ Session and Listeners, [send/do]FinalSpendingLimit[Request/Answer] should be [send/do]SessionTermination[Request/Answer], to follow the name of the actual Diameter message.
Replace getSLRequestTypeAVPValue with getSLRequestType, as this simplifies the API.. we are not strict on this, as we have some getAVPValue, but I prefer if we don't add the suffix.
Don't change (un)registerAppFacory to (un)registerAppFactory (we know about this typo, but changing it will break many apps).
Also, please don't make unrelated changes in other places (eg, https://github.com/RestComm/jdiameter/pull/57/files#diff-2ea999abd5aad58a6283ef3a04cbe3dc, https://github.com/RestComm/jdiameter/pull/57/files#diff-ea50086ddc217fd882f3bc48efbd9bbe, several dictionary changes.. when you find these unrelated issues, please address them in a separate PR, so it's easier to review and merge)
ISyMessageFactory misses the methods for creating answers ?
getSLRequestTypeAVPValue always return 0 ?
Missing checks for null/invalid sessionData, listener, appIds values at ServerSySessionImpl constructor
ServerSySessionImpl is overriding methods from parent implementations with empty bodies (addStateChangeNotification, removeStateChangeNotification, getSessions, getSessionAppId, etc..)
ServerSySessionImpl : handleEvent : case SENT_INITIAL_RESPONSE is sending the message again instead of calling dispatchEvent(..) ?
ServerSySessionImpl is missing proper AnswerDelivery implementation
Is the ClientSySessionImpl missing (among other things?)

Overall it looks like this is still unfinished work, are you still open to finish this work ? I will be more active now on Diameter and able to provide feedback and help you quicker.

added testsuite/tests/sy/... revert example/charging-server-simulator issue #44 added AVP's for Ro messages issue #43

Alex added 6 commits August 25, 2016 17:32

Sy interface implementation - first commit

087435f

+ jdiameter-api/sy interfaces

Sy interface implementation - first commit

cf1861b

+ jdiameter-api/sy interfaces

Sy interface implementation - fixed some AVP definitions

89614b5

+ jdiameter-parent/examples/resources/dictionary.xml + defined OC-Supported-Features codes + added AVP definitions

Sy interface implementation - fixed some AVP definitions

18131d8

+ jdiameter-parent/examples/resources/dictionary.xml + defined OC-Supported-Features codes + added AVP definitions

Sy interface implementation - api & defs - Impl missing

b265d07

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP

Sy interface implementation - api & defs - Impl missing

5da6b3b

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP

FerUy added the enhancement label Sep 8, 2016

deruelle assigned brainslog Sep 8, 2016

Alex added 2 commits September 9, 2016 08:11

Sy interface implementation - api & defs - Impl missing

67bae63

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP issue #44

Sy interface implementation - api & defs - Impl missing

16f5125

+ jdiameter-parent/examples/resources/dictionary.xml + fixed OC-Supported-Features codes + added AVP definitions for Sy + added DRMP issue #44

brainslog reviewed Sep 19, 2016

View reviewed changes

aferreiraguido changed the title ~~Sy Protocol Implementation - Pull for API Interface & AVP's~~ Sy Protocol Implementation Sep 24, 2016

Alex added 8 commits September 24, 2016 21:34

Sy interface implementation - api & defs - Impl missing

cafeaac

+ fixed according fo @brainslog comments #issue 44

Sy Impl - first release

c832b63

issue #44

Sy Impl - night build #1

6abef3c

issue #44

Sy Impl - night build #2

c3b5124

issue #44

Sy Impl - night build #3

207e692

issue #44

Sy Impl - night build #3

ab0e144

issue #44

Sy Impl - night build #4

17401cd

+renamed jdiameter/client/api/ISessionFactory.registerAppFac<t>ory() issue #44

Sy Impl - night build #6

4346890

added testsuite/tests/sy/... revert example/charging-server-simulator issue #44 added AVP's for Ro messages issue #43

Sy Impl - night build #6

920b07f

added testsuite/tests/sy/... revert example/charging-server-simulator issue #44 added AVP's for Ro messages issue #43

brainslog modified the milestones: 1.7.0.FINAL, 1.7.0.FINAL-Sprint2 Mar 28, 2017

deruelle modified the milestones: 1.7.0-FINAL-Sprint 3, 1.7.0.FINAL-Sprint2 May 3, 2017

ammendonca modified the milestone: 1.7.0-FINAL-Sprint 3 Jul 13, 2017


		<!-- ********************** END 3GPP S13/S13' AVPS ***************** -->

		<!-- *************************** 3GPP Sy AVPS ********************** -->

Conversation

aferreiraguido commented Sep 8, 2016

Uh oh!

FerUy commented Sep 8, 2016

Uh oh!

deruelle commented Sep 8, 2016

Uh oh!

FerUy commented Sep 8, 2016

Uh oh!

aferreiraguido commented Sep 8, 2016

Uh oh!

FerUy commented Sep 8, 2016

Uh oh!

aferreiraguido commented Sep 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brainslog Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brainslog commented Sep 19, 2016

Uh oh!

brainslog commented Mar 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brainslog Sep 19, 2016 •

edited

Loading