1344043 - taskcluster-lib-api: provide context to parameter and query validation functions

Status: RESOLVED FIXED

(Taskcluster :: Services, enhancement)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

In https://github.com/taskcluster/taskcluster-lib-api each API method gets a "context" containing useful values, as described in the README:

 * `context` - a list of context entries that must be passed to `api.setup`.  
   Each will be available as properties of `this` within the implementation
   of each API method.

Users can also define functions to validate params or queries, also described in the README:

```js
  params: {
    thingId: /[a-z.]+/,
    filter: v => {
      if (!validFilterExpression(v)) {
        return "invalid filter expression";
      }   
    },  
  }
```

However, you can't access context from within the param or query functions.  This won't work:

```js
  params: {
    filter: v => {
      if (!this.filterConfig[v]) {
        return `no filter named ${v} defined`;
      }   
    },  
  }
```

To solve this bug, make filter functions like the above, and similarly query functions, get passed the context as "this".

This will require learning a little bit about how JavaScript handles "this" specially.  Please also write unit tests, and update the README to indicate that context is available for these functions.

Comment 1

[yuktikirtani]

Hey,
I would like to work on this issue

Comment 2

[yuktikirtani]

Could you please guide me to the aforementioned code?
Thanks.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

I'd suggest to start by getting the tests for taskcluster-lib-api running.

The next step would be to modify the tests to also use filter and query validator functions that reference `this`, and see that those tests fail.  Then fix the implementation to make those tests pass, and update the README.

Comment 4

[yuktikirtani]

Okay, will try this.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

yuktikirtani -- are you still working on this?

Comment 6

[Prachi Manchanda]

Hi Dustin,
If this is available, I'd like to give this a try.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

I suspect it is available - it has been several months with no comment from yukti.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Also, good to see you back! :)

Comment 9

[Prachi Manchanda]

I did get the tests running and was now trying to write the tests referencing context inside the params and queries in tests/context_test.js
I had a couple of queries
1. The api/publish tests get skipped every time I run npm test
2. For some reason, the test I wrote passes both with and without this parameter.. which clearly, should not happen. I think the reason for this could be that I do not completely understand how context and params work with each other for taskcluster-lib-api and was simply using the example in the README (using filter as a parameter) to write the test for this case. Are there any tests written which use both context and params? I tried looking but couldn't find any!
Thanks

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Skipping the publish tests is OK -- you don't have the credentials necessary to run them, but you're not modifying anything that affects publishing anyway.

When I say "context" here I mean the Javascript value with the special name "this".  So to take the example from the README:

  params: {
    region: r => {
      return this.cfg.app.allowedRegions.includes(r);
    },
  }

So here `this` is the context, just like for API handlers (https://github.com/taskcluster/taskcluster-lib-api#api-server-setup).  You might be able to add something like this to https://github.com/taskcluster/taskcluster-lib-api/blob/master/test/context_test.js#L13

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

Prachi, how is this going?

Comment 12

[Prachi Manchanda]

I am still working on writing the tests. I expect to be done by Monday. If not, I'll unassign myself.

Comment 13

[Jonas Finnemann Jensen (:jonasfj)]

For the record, this can never work:
>  params: {
>    region: r => {
>      return this.cfg.app.allowedRegions.includes(r);
>    },
>  }

arrow functions cannot have a `this`, instead I suggest:


>  params: {
>    region: function(r) {
>      return this.cfg.app.allowedRegions.includes(r);
>    },
>  }

for the few cases where you actually want to use the context.

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Sorry, yes, I should have used function().  But if you try that, you'll see you don't get the right context, which is what the bug is here.

Comment 15

[Yashvardhan Didwania [:ydidwania]]

Prachi, are you still working on this? If not I would like to take it up

Comment 16

[Yashvardhan Didwania [:ydidwania]]

I started off with this bug a few days back. I was facing a similar problem as Prachi, using params in the API Setup - 

var api = new subject({
      ...,
      context:      ['myProp'],
      name:         'test',
      params: {
        filter: function()  {
          console.log(this);
          console.log('just testing');
        },
      },
    });
    api.declare({
      method:   'get',
      route:    '/context/:filter',
      name:     'getContext',
      title:    'Test End-Point',
      description:  'Place we can call to test something',
    }, function(req, res) {
      console.log(req.params);
      res.status(200).json({myProp: this.myProp});
    });


I had to add `:filter` to the url route in order to get `filter` in `req.params`, without it `req.params` is just {} , even when `params` is not empty. I suppose our task here is that `console.log(this)` in `params` should no longer be `undefined`.

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Sorry for the delay yd -- yes, that's all exactly right.  Note that you can write the 

params: {
  filter: function() { .. },
}

part either in the `api = new API(..)` or in the `api.declare(..)` -- it should work the same either way.

And yes, the param is only used if it appears in the route.

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

I'll finish this up.

Comment 19

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/taskcluster-lib-api/pull/105

Prachi, YD, if you want to have a look at the PR I'd love to hear any feedback :)

Comment 20

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-lib-api

https://github.com/taskcluster/taskcluster-lib-api/commit/7bfeaa0151f5743cbb303bd24d34d501a7198e2d
Bug 1344043 - provide context to query, param validation functions

https://github.com/taskcluster/taskcluster-lib-api/commit/c21aaa5748160616d0433d28b7287f2909735095
Merge pull request #105 from djmitche/bug1344043

Bug 1344043 - provide context to query, param validation functions