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

How to proxy params from request to ApiMethod #20

Closed
tarmolov opened this issue Aug 21, 2014 · 12 comments
Closed

How to proxy params from request to ApiMethod #20

tarmolov opened this issue Aug 21, 2014 · 12 comments

Comments

@tarmolov
Copy link
Member

Sometimes you need to pass some parameters from the express request. For example, hostname, IP address, or a header.

It's impossible to pass these parameters from a browser; so, you need another way to get access to them. Currently, the express middleware tries to find method params using req.param(name) and req.session. Thus, you don't have a way to proxy custom request parameters to your api method.

I have only one suggestion. Now Api class knows nothing about express and environment. It would be great to save this.

There are several possible solutions of this issue:
1. Add an option to the middleware.

apiMiddleware(pathPattern, {
    getParameterValueByName: function (name, req) {
        // if the req param is defined in a method, it'll pass this param to it
        return name === 'req' ? req : req.param(name);
    })
});

prons:

  • the most simplest solution to implement

cons:

  • some parameters, which are not passed from a browser, appear in a magic way

2. Mark some parameters as an environment variables.
In a api method declaration add a special flag to the parameter.

.addParam({name: 'req', environment: true})

It shows a special label in doc page and it could be ignored in server side.

Anyway we have to add a special option to the middleware.

apiMiddleware(pathPattern, {
    getEnvironmentParameterValue: function (name, req) {
        // if the req param is defined in a method, it'll pass this param to it
        return name === 'req' ? req : req.param(name);
    })
});

prons:

  • simple solution
  • you can choose which parameters are magic in more explicit way

cons:

  • there are two types of parameters

3. Pass environment as a third parameter to the method

api.exec(methodName, params, environment);

In the middleware you should build the environment object

apiMiddleware(pathPattern, {
    getEnvironmentObject: function (req) {
        return {
            hostname: req.get('host'),
            ...
        }
    })
});

prons:

  • passing environment parameters in explicit way

cons:

  • method execution becomes more complex

4. Always proxy express request as a third parameter.
We also can enable this mechanism with a method option:

.setOptions('proxyRequest', true);

prons:

  • freedom of using express request

cons:

  • api methods depend on express environment
  • it's little tricky to execute method on the server side

@dodev, @lemmy-, if I forget something to add, let me know.

Guys, we need your opinions.
//cc @mdevils, @razetdinov, @dfilatov

@dodev
Copy link
Contributor

dodev commented Aug 25, 2014

I came up with another solution. It is based on the fact that API methods will be part of applications that use one framework and the method can have a small piece of isolated, framework-dependent code.

The solution uses the approach with separating the method parameters in two classes - passed by the user and environment provided. So the param definition in the API method file will require a way to distinguish the environment-provided ones:

.addParam({'name': 'foo', 'env-provided': true});

The framework-dependent part in the API method definition should look something like this:

...
// framework-dependent code starts here
.processEnvironmentParams(function (environment) {
    return {
        path: environment.pathname,
        userCookie: environment.cookies.yp
    };
})
// framework-dependent code ends here
...

And the main code can remain framework-agnostic:

.setAction(function (params) {
    logger.notice('request was made for ' + params.pathname + ' from user with cookie ' + params.userCookie);
})

In the same time there will be no need for adding a global param-mapping function and each method can extract only environment data-fields that it will actually use.

Pros:

  • Data-access restriction. The API method's execution code has access only to those params that are used in it.
  • The params will be declared in the same file that they are used in.
  • The main API execution code remains unaware of the environment.

Cons:

  • The framework-dependent code partially limits the portability of the api-method.
  • A mechanism is needed for calling API methods on the server-side.

@mdevils
Copy link

mdevils commented Aug 25, 2014

@dodev, looks good to me.

@tarmolov
Copy link
Member Author

Good approach.

We can consider environment as a source of default params.

.setOption('setDefaultParamsFromEnvironment', function (environment) {
    return {
        path: environment.pathname,
        userCookie: environment.cookies.yp
    };
});

It means that your don't mark parameters with a special flag and the method use a environment to fill default values for the parameters.

Furthermore, we can allow the pass environment as a second parameter for execution on the server side:

api.exec('method', params, environment);

In this case setDefaultParamsFromEnvironment will be used for the server side, too.

@dodev
Copy link
Contributor

dodev commented Aug 26, 2014

I think that parameters that can be set by the environment should be marked never the less. They can fall in the category 'optional', but that name can be deceiving for example a user can call a method from the server-side without passing the environment and not setting those params, because they are marked as 'optional'. This is not critical, but can lead to some nasty bugs.
In short, my point is that we should have a mechanism for marking parameters as 'optional-env-provided'

@tarmolov
Copy link
Member Author

Yeah, actually you're right. Let's think about parameter name.
I prefer

.addParam({name: 'param', useEnvironment: true})

@dodev
Copy link
Contributor

dodev commented Aug 26, 2014

What about environmentProvided or fromEnvironment

@dmikis
Copy link
Contributor

dmikis commented Aug 26, 2014

I think, we can achieve both method portability and powerful mechanism for obtaining some params from a context of an application or a request.

Here is my suggestion. Let's add two new features:

  1. a new characteristic of a parameter, let's call it inferable;
  2. an ability to provide a callback for modifying method params as an intermediate step between a caller and the code of the method itself, let's call it inferParamsCallback.

If any parameter of the method is declared as an inferable one, a caller may omit this parameter. If a call occurs, and some inferable parameters are omitted, the module calls method's inferParamsCallback with parameters passed by the caller and some context-like wrapper as arguments.

The inferParamsCallback should examine obtained parameters and, if necessary, get (or even compute) missed inferable parameters from the context. The context may be a some sandbox object, an express request object or anything else. Plainly, the inferParamsCallback is in power to do whatever the hell it wants to do to obtain all inferables.

When inferParamsCallback has done what it could and some inferables are still missing, the module should raise an error. This makes inferables not quite as optional parameters. Omitting them means you don't know how (or don't want) to get parameters value and suggest the method to do it itself. But you always have an option to provide them explicitly.

@tarmolov
Copy link
Member Author

Solutions presented above look very complex for me but the issues is not a big deal actually.

@lemmy- and I talked about this a little today and we agreed that we should always proxy express request as a second parameter to the method.

From the server side you can pass express request as well:

api.exec('method', params, req);

If you decide to use express req, make sure that it's not null (an extra check in the api method). Also it makes it possible to define undefined optional parameters if you want to. Full freedom, guys 😈

If you work in the nmaps team, then you love to divide all methods into two groups: the server side and the frontend side. This approach will work, too, if we proxy request to a method.

Anyway we need a special optional to mark parameters which will be provided by request (for docpage).

thoughts?

@watch-the-stars
Copy link

The motivation here is that this is the simplest solution we've come up with so far, it's easy to implement and does exactly what it's supposed to. The downsides are some new usage conventions.

@dodev
Copy link
Contributor

dodev commented Aug 27, 2014

If an ApiMethod doesn't need environmental objects can I omit the req parameter in the exec method?

@watch-the-stars
Copy link

I guess you can ignore it if you don't need it.
If you do, make sure to pass the req when calling the method directly (from the server side, as we usually say).

@dfilatov
Copy link
Contributor

By the way, "guys from nmaps team" have already been using such approach )

tarmolov added a commit that referenced this issue Aug 29, 2014
Proxy express request to a method. fixed #20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants