Skip to content
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

URLService proposed improvements #179

Merged
merged 1 commit into from Nov 7, 2012
Merged

Conversation

apicolet
Copy link
Contributor

@apicolet apicolet commented Oct 5, 2012

Change

Two changes to the URLService framework, needed for interaction with RESTFul services :

  • now modules can pass a full service specification structure, instead of a raw actionName string, to be used by the URLService to determine the target request
  • an URLService can now return a structured request details instead of a raw URL, which can for now contain URL and HTTP method; this mechanism could later be used for URLService to define needed HTTP headers as well (Range:, Accept:, ...)

Example use

New way to call sendJsonRequest from a module :

this.submitJsonRequest(
  {name:"fm.flightleg.get",
   params:{flight:"AF226",departurePort:"CDG"}},
   null,
   {
     fn : this._onRequest,
     scope : this,
     args:{}
  });
},

Usage in an URLService implementation :

    $actionMap: {
        "fm.flightleg.get" : {
            method : "GET", url: "/flight/{flight}/leg/{departurePort}"
        },
        "fm.flightleg.update" : {
            method : "POST", url: "/flight/{flight}/leg/{departurePort}"
        }
    },

    createServiceUrl : function (moduleName, serviceSpec, sessionId) {
        var actionName = serviceSpec.name;
        var actionParams = serviceSpec.params;
        var actionDef = this.$actionMap[actionName];
        if (!actionDef) {
            return '';
        }
        //TODO : do a generic URL template engine
        var url = actionDef.url;
        url = url.replace(/\{flight\}/,actionParams.flight);
        url = url.replace(/\{departurePort\}/,actionParams.departurePort);

        if (sessionId) {
            url = aria.modules.RequestMgr.__appendActionParameters(url,"SITK="+sessionId);
        }

        return {
            url: this.baseUrl+url,
            method: actionDef.method
        };
       }

* Generate a 'service' URL
* This implementation understands a serviceSpec in the form
* {actionName: string}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing 3 @param

@piuccio
Copy link
Contributor

piuccio commented Oct 6, 2012

Hello Antoine, the code seems to be ok, but the travis build is broken.

JShint found 15 errors in the code! Please check build/at-lint/lint-log.txt for the complete logs

Do you happen to have any unit test?

@apicolet
Copy link
Contributor Author

apicolet commented Oct 8, 2012

I tried to take into account all comments. Two issues I have :

  • now that I created a new one (RequestDetails), there now seems to be a lot of different Beans in aria.modules.RequestBeans, whose purposes might not always be very clear to user
  • I do not have unit tests for this chenge; I planned to adapt the existing unit tests on ModuleCtrl and RequestMgr to account for the new input/output format, but could not find them in the the test directory. Would you have an advice on how to progress ?

@apicolet apicolet closed this Oct 8, 2012
@apicolet apicolet reopened this Oct 8, 2012
@piuccio
Copy link
Contributor

piuccio commented Oct 13, 2012

Hello, if you want to experiment with unit testing you can now have a look at CONTRIBUTING and test/README.

@vratojr
Copy link
Contributor

vratojr commented Nov 7, 2012

I wrote the unit test and fixed some bugs. Waiting for apicolet to merge in the changes..

- now modules can pass a full 'service specification' structure, instead of a
raw actionName string, to be used by the URLService to determine the request
- URLService can now return a structured 'request details' instead of a raw
URL, which can contain URL and HTTP method; this could be used for HTTP headers as well

- Updated the existing tests for RequestMgrTest
- Added test for RequestMgr::createRequestDetails
- Added tests for PatternURLCreationImpl::createServiceUrl( a lot of formatting,
modified only the method: testOriginalCreation)
@vratojr
Copy link
Contributor

vratojr commented Nov 7, 2012

Almost ready for merging. Checking if more tests are needed.

@vratojr vratojr closed this Nov 7, 2012
@vratojr vratojr reopened this Nov 7, 2012
vratojr added a commit that referenced this pull request Nov 7, 2012
URLService proposed improvements
@vratojr vratojr merged commit 6898eaf into ariatemplates:master Nov 7, 2012
@vratojr
Copy link
Contributor

vratojr commented Nov 7, 2012

I cancelled the merge.
There's a minor bug that's better to fix before merging.
In this code:

testCreateRequestDetailsForService : function () {
        var requestMgr = aria.modules.RequestMgr, url;

        urlAction = requestMgr.createRequestDetails({
            moduleName : "tpls",
            actionName : "doTest1"
        }, {
            id : "x"
        });

        urlService = requestMgr.createRequestDetails({
            moduleName : "tpls",
            serviceSpec : {
                actionName : "doTest1"
            }
        }, {
            id : "x"
        });

        this.assertEquals(urlAction.url, urlService.url);
    }

urlAction and serviceAction are declared globally. Let's make them local.
Do the modification, please, then use the command:
git commit -a --amend
and leave the commit message unchanged ( type :wq ) to exit from vi.

@vratojr
Copy link
Contributor

vratojr commented Nov 7, 2012

then push again to you branch and reopen the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants