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

javax.sip.Transaction - addition of applicationData methods #6

Closed
tzieleniewski opened this Issue Feb 25, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@tzieleniewski
Collaborator

tzieleniewski commented Feb 25, 2016

The javax.sip.Transaction interface defines applicationData methods, used to store transaction specific data:

The current base wrapper implemetation throws SecurityException.

    /**
     * Operation forbidden in the sip interfaces implemented by concrete wrappers.
     * @return
     */
    public Object getApplicationData() {
        throw new SecurityException();
    }

    /**
     * Operation forbidden in the sip interfaces implemented by concrete wrappers.
     * @param arg0
     */
    public void setApplicationData(Object arg0) {
        throw new SecurityException();
    }

Methods are not overriden in wrapper classes for client and server transactions.
Only the ACKDummyTransaction overrides applicationData methods
https://github.com/RestComm/jain-slee.sip/blob/master/resources/sip11/ra/src/main/java/org/mobicents/slee/resource/sip11/wrappers/ACKDummyTransaction.java

I recommend an enhancement through the implementation of those methods for Transaction extended interfaces.

@abhayani

This comment has been minimized.

abhayani commented Feb 25, 2016

Comments from Jaime Casero

To allow user defined transaction data (which is what the customer is aking for), and save the private tx wrapper that only the container should access, we could save a simple object with private reference to tx, and a generic object reference to what the app logic saved.

That way we could potentially exposed the setApplicationData method to the SBB, but set the proper reference inside our new compound object, since we know when the internal container TX wrapper data is saved, and when is the app logic setting/retrieving custom data. Of course, someone should review impact on replicated environments.

Just mentioning as suggestion, maybe someone wants to contribute based on this idea...

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Feb 26, 2016

The ClientTransactionWrapperAppData is set in the wrapped transaction application data in the ClientTransactionWrapper public constructor.

    public ClientTransactionWrapper(SIPClientTransaction wrappedTransaction, SipResourceAdaptor ra) {
        super(new ClientTransactionActivityHandle(wrappedTransaction.getTransactionId()),ra);
        this.wrappedTransaction = wrappedTransaction;
        this.wrappedTransaction.setApplicationData(new ClientTransactionWrapperAppData(this));
        if (tracer == null) {
            tracer = ra.getTracer(ClientTransactionWrapper.class.getSimpleName());
        }
    }

An addition of the private field, referencing application data object, to the ClientTransactionWrapperAppData class would assure, as Jaime Casero commented, that application data is properly saved.

Contribution
Presented for the ClientTransaction, in analogous way would apply to the ServerTransaction.

Extension of the TransactionWrapperAppData interface

public interface TransactionWrapperAppData {

    Object getWrappedApplicationData();

    void setWrappedApplicationData(Object wrappedApplicationData);

    public TransactionWrapper getTransactionWrapper(Transaction t, SipResourceAdaptor ra);

}

Implementation of added and extension of Externalizable interface methods i.e. ClientTransactionWrapperAppData

public class ClientTransactionWrapperAppData implements TransactionWrapperAppData, Externalizable {

    private ClientTransactionWrapper transactionWrapper;
    private String associatedServerTransactionId;
    private Object wrappedApplicationData = null;

    ..

    @Override
    public void writeExternal(ObjectOutput out) throws IOException {
        if (transactionWrapper != null) {
            associatedServerTransactionId = transactionWrapper.getAssociatedServerTransaction();
        }
        if (associatedServerTransactionId != null) {
            out.writeBoolean(true);
            out.writeUTF(associatedServerTransactionId);
        } else {
            out.writeBoolean(false);
        }

        writeExternalWrappedApplicationData(out);
    }

    private void writeExternalWrappedApplicationData(ObjectOutput out) throws IOException {
        if (wrappedApplicationData != null) {
            out.writeBoolean(true);
            out.writeObject(wrappedApplicationData);
        } else {
            out.writeBoolean(false);
        }
    }

    @Override
    public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
        if (in.readBoolean()) {
            associatedServerTransactionId = in.readUTF();
        }

        readExternalWrappedApplicationData(in);
    }

    private void readExternalWrappedApplicationData(ObjectInput in) throws IOException, ClassNotFoundException {
        if (in.readBoolean()) {
            wrappedApplicationData = in.readObject();
        }
    }

    @Override
    public Object getWrappedApplicationData() {
        return wrappedApplicationData;
    }

    @Override
    public void setWrappedApplicationData(final Object wrappedApplicationData) {
        this.wrappedApplicationData = wrappedApplicationData;
    }

}

Implementation of applicationData methods in ClientTransactionWrapper

public class ClientTransactionWrapper extends TransactionWrapper implements ClientTransaction {

    ..
    @Override
    public Object getApplicationData() {
        return clientTransactionWrapperAppData().getWrappedApplicationData();
    }

    @Override
    public void setApplicationData(final Object applicationData) {
        clientTransactionWrapperAppData().setWrappedApplicationData(applicationData);
    }

    private ClientTransactionWrapperAppData clientTransactionWrapperAppData() {
        return (ClientTransactionWrapperAppData) wrappedTransaction.getApplicationData();
    }

}

Jean Deruelle gave me rights to the repository to contribute. I would like to move forward if my proposition is ok.

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Feb 29, 2016

I write this code with adding some test:

Implementation of setApplicationData methods in ClientTransactionWrapper

    @Override
    public void setApplicationData(final Object applicationData) {
        ClientTransactionWrapperAppData appDataWrapper = clientTransactionWrapperAppData();
        if (appDataWrapper == null) {
            throw new IllegalStateException();
        } else {
            appDataWrapper.setWrappedApplicationData(applicationData);
        }
    }

I write analogous code for ServerTransaction.

SergeyLee added a commit to SergeyLee/jain-slee.sip that referenced this issue Feb 29, 2016

Fixes issue RestComm#6. Implementation of applicationData methods for…
… ClientTransactionWrapper and ServerTransactionWrapper.
@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Feb 29, 2016

@tzieleniewski check changes in my commit: #7.

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Feb 29, 2016

Lee I've already prepared patch with more test and generics.
May You grant me a branch where I can introduce more/better changes.

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Feb 29, 2016

That was on paper proposition.
After digging into the source code I've introduced more optimal changes.

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Feb 29, 2016

@tzieleniewski this question to @deruelle.
Or you can fork and create your pull request.

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Feb 29, 2016

@tzieleniewski you need sign telestax opensource contributor agrement.
http://www.telestax.com/open-source/

@deruelle

This comment has been minimized.

Member

deruelle commented Feb 29, 2016

@SergeyLee @tzieleniewski already signed the contributor license agreement. @tzieleniewski you need to follow the open source playbook process from https://docs.google.com/document/d/1RZz2nd2ivCK_rg1vKX9ansgNF6NpK_PZl81GxZ2MSnM/edit#heading=h.mcjitemt6ng1. Please fork the project, work in your branch, push to your fork and then do a pull request for review.

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Mar 1, 2016

Is there a code formatting convention, i.e. tab sizes?

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Mar 2, 2016

Code pushed.
I'm not creating pull request until tests confirm correct behavior.
https://github.com/tzieleniewski/jain-slee.sip/commit/2ba5121dee5c743d513beb3748727f1f8b6f9bd4

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Mar 3, 2016

Thanks, @tzieleniewski!
As I can see tab size was 8 spaces. It is strange because in SublimeText and Gedit it shows only 4 spaces...
4 spaces for tab size sounds good.
I'll try it as soon as possible.

SergeyLee added a commit to SergeyLee/jain-slee.sip that referenced this issue Mar 16, 2016

SergeyLee added a commit to SergeyLee/jain-slee.sip that referenced this issue Mar 16, 2016

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Mar 16, 2016

@tzieleniewski Thanks!
I will see it ASAP.

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Mar 17, 2016

I merged your pull request (see last comment on it).
How I can test it on my local machine?
PS: I think that JUnit tests is enough for this.

@tzieleniewski

This comment has been minimized.

Collaborator

tzieleniewski commented Mar 29, 2016

Thx,
For instance set the application data inside the IES method and then get it inside event processing methods, during initial and following events processing, when new SBB object is assigned.
TKZ

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