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

Adding some missing XML serializations to the CAP stack #103

Closed
AlerantAppNGIN opened this Issue May 30, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@AlerantAppNGIN
Collaborator

AlerantAppNGIN commented May 30, 2016

We should add XML serialization for:
PlayAnnouncementRequestImpl
SpecializedResourceReportRequestImpl
ResetTimerRequestImpl

@deruelle deruelle added this to the 3.1.0 milestone May 30, 2016

@vetss

This comment has been minimized.

Collaborator

vetss commented Jun 2, 2016

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:
8d00113
8621f5d

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).
132f1ea

@AlerantAppNGIN

This comment has been minimized.

Collaborator

AlerantAppNGIN commented Jun 3, 2016

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

This comment has been minimized.

Collaborator

vetss commented Jun 3, 2016

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.

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