RestComm / jss7

RestComm Java SS7 Stack and Services
http://www.restcomm.com/
GNU Affero General Public License v3.0
190 stars 221 forks source link

Adding some missing XML serializations to the CAP stack #103

Closed AlerantAppNGIN closed 8 years ago

AlerantAppNGIN commented 8 years ago

We should add XML serialization for: PlayAnnouncementRequestImpl SpecializedResourceReportRequestImpl ResetTimerRequestImpl

vetss commented 8 years ago

Hello @AlerantAppNGIN,

I have committed your updates into netty-2 branch. We have stopped to add new updates into master branch because we will release it soon. Then we will rename betty-2 to master.

Fixed by: https://github.com/RestComm/jss7/commit/8d0011306d048f1ebf0885dc4a440ff37e7dbc76 https://github.com/RestComm/jss7/commit/8621f5d48382b6ab1c35d63a32e01a91acebd7aa

Also I added an extra fix. I think that SpecializedResourceReportRequestImpl was not made in the best way. Please check my update. (I also made some extra code changes for code style). https://github.com/RestComm/jss7/commit/132f1ea47918edb4e30fb245a2da095d3034d8e8

AlerantAppNGIN commented 8 years ago

Hello @vetss,

Thank you for fixing and merging our mods. I have some PRs ready to send but from now on I'll rebase them to the netty-2 branch.

I'd like to discuss the handling of default values in general.

When decoding ASN.1 data, a missing optional parameter with a default value must always be parsed as if the default value was there in the data. We think that it is a good idea to use the same logic in XML parsing as well. For this reason, we always assign the default value to the parameter first, and only update it if a contradicting value is present in the input data. For disconnectFromIPForbidden that means that in the decode and read method a Boolean(true) value will be assigned to it initially and the value of this field should only be changed if disconnectFromIPForbidden is present in the input data.

I'd like to hear your opinion about this topic because the next updates I am preparing contain default value handling too.

Thank you. Regards, Gabor

vetss commented 8 years ago

Hello @AlerantAppNGIN

I have some PRs ready to send but from now on I'll rebase them to the netty-2 branch.

fill free to provide PR even for a master branch if they are ready. I will check and add commits into netty-2 branch. But please add new PR to code based on netty-2 branch.

As for treating of absense of XML parameters (that have a default value) as a default value (and not null) as it occurs at SS7 part: we always added into the XML part parameters even they have default values and decode they also without caring of default values. Your approach also makes sense of cause, but the issue that we followed previously in another style and this means behaviour changing that we need to avoid in code update as much as possible.

So let's encode all non-null parameters even they have default values.