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

Allow for using python code from a separate Python module inside the specification #301

Closed
camilamaia opened this issue Oct 8, 2020 · 10 comments
Assignees
Labels
Feature New feature or request

Comments

@camilamaia
Copy link
Member

Allow for using python code from a separate Python module inside the ScanAPI specification.

Example:

File tests.py

def status_is_200(**kwargs):
    response = kwargs.get(response)
    return response.status_code == 20

File scanapi.yaml

...
tests:
    - name: status_code_is_200
      assert: ${{ tests.status_is_200 }}
@camilamaia camilamaia added Feature New feature or request Hacktoberfest http://hacktoberfest.digitalocean.com/ labels Oct 8, 2020
@zebralt
Copy link

zebralt commented Oct 14, 2020

Hi, I would like to work on this.
Maybe we could have some sort of Context (to which you explicitly or implicitly feed modules to look into) object that would be passed along as the spec gets traversed, which could also be used for #265, in which case I may be able to cover both.

I'm also seeing the discussion happening on #51, so should the solution just expect to be parsing a reference to a function (e.g. ${{ module.a.function }}) instead of actual python code in a ${{ expression }} ?

Looking at the examples, accessing some of the response fields seems to be a key feature, so just a name wouldn't cut it.
The program needs to be able to both call out-of-scope methods and retain some ability to manipulate the response within a single expression.

If the goal is only to be able to switch from python code to 'I want to call this function, no real python code involved', maybe there needs to be a symbol that tells the evaluator that this needs to be processed differently, such as a prefix or a different syntax, e.g. ${{ !tests.status_is(200) }} (with some partial binding on the cake too).

@camilamaia
Copy link
Member Author

@zebralt thanks! I've assigned you to the ticket.

About your point, I agree with you, the python code should have a context. I believe it should have the same context as it would have if it was only an expression as it is today. I mean, the context should be the same, using {{ expression }} or using a separate module. What do you think?

About the #51, I believe we still want the ability to have only an expression. This is specially good for people that are not fluent in Python. Just the way we validate it could be different, and not an eval.

It would be nice to hear more thoughts here: @barbosa @flaviotruzzi @kelvins @marcuxyz, any comments? What do you think is the best way to have this feature?

@flaviotruzzi
Copy link
Contributor

I'll give it a thought, but I think @zebralt is going in the right direction, I think Jinja use something similar. You should be able to have a context and patch the globals/locals when executing that code so it is sandboxed. It all depend on how much control and safety one would want.
You can be inspired on how jinja2 do its sandboxing and control its environment:

https://github.com/pallets/jinja/blob/9b718ed3d26fc2f80ab58f3249b517a65db9cc7b/src/jinja2/environment.py#L132
https://github.com/pallets/jinja/blob/master/src/jinja2/sandbox.py

@kelvins
Copy link
Contributor

kelvins commented Oct 18, 2020

Although ScanAPI uses Python eval to evaluate Python expressions, it is a general tool that can be used for any web API, regardless of the programming language (as far as I understand it). With this feature, the tool will be more "coupled" to Python projects, since the Python support will be extended outside the yaml specification. I have no opinion against that, I just think it is important to keep that in mind.


One suggestion I can think of is to use the same pattern Gunicorn uses to define the app module, for example: src.my_module:status_is_200(response). It seems they use this function to dinamically import the module.

In this case, we probably need to change the way the evaluation is performed.


Another idea that I had, actually a workaround to avoid changing the way we perform evaluation, is to create a new modules key, for example:

modules:
  test_module: testing.test  # alias: module path
endpoints:
  - name: pokemon
    path: pokemon
    requests:
      - name: list_all
        method: get
        tests:
          - name: status_code_is_200
            assert: ${{ test_module.status_is_200(response) }}

Then, internally we would need to dynamically import these modules using the alias and perform the Python eval normally, for example:

>>> import importlib
>>> modules = {'test_module': 'testing.test'}
>>> for module_alias, module_path in modules.items():
>>>     setattr(self, module_alias, importlib.import_module(module_path)) # inside a class
>>> eval('test_module.status_is_200(response)')

Probably we would need to include a mechanism to avoid accidentally overriding attributes from the class itself.

@zebralt
Copy link

zebralt commented Oct 20, 2020

Another idea that I had, actually a workaround to avoid changing the way we perform evaluation, is to create a new modules key, for example:

modules:
    test_module: testing.test  # alias: module path

This is what I was thinking of when I spoke of explicitly importing modules: as part of the spec. You would then propagate this modules dict all the way down to CodeEvaluator, rather than having to do module discovery on the spot.

For now, I kept my work self-contained as a new scanapi/evaluators module, and so I went the implicit import way instead so I wouldn't have to depend on the spec aside from what is exposed to CodeEvaluator.

One suggestion I can think of is to use the same pattern Gunicorn uses to define the app module, for example: src.my_module:status_is_200(response).

Yes, a separate syntax/feature might be the way to go for this current issue, especially if you want to retain backward compatibility with existing specs. So far I've been working on the custom syntax I proposed (exprs starting with a !) but I don't mind using this one instead :)

It seems they use this function to dinamically import the module.

Thanks for sharing that :D I wrote a similar function to fetch a module by name, but the parsing is interesting too; I was doing my own parsing of a ast.Call-like expression (<name>(<args>) and literal_evaling comma-separated arg strings, but that seems like a cleaner approach, so I'll probably move to ast.parse instead.

In summary

Sandboxing seems like the optimal solution to the problem, but I'm not sure I can build an efficient solution for a problem of this magnitude in the span of time left for Hacktoberfest.

Still I see there are off-the-shelf solutions like https://github.com/zopefoundation/RestrictedPython, so it may be worth giving a shot. This might just be a drop-in replacement for eval with some tweaking involved. Other links:

Or I can stay within the scope of the issue (some of this I have already done):

  • create a new syntax (either !module.f(args) or module:f(args) allowing only calling functions found outside custom modules or scanapi off-the-shelf;
  • this syntax would exist alongside the current implementation making use of eval;
  • parse this new syntax with ast.parse like gunicorn does, e.g. expecting a ast.Call node;
  • import the function object (by importlibing the module, or looking in a set list of usable modules);
  • bind the function using the literal arguments in the expression;
  • feed the response to any function called this way and return its result.

@camilamaia camilamaia removed the Hacktoberfest http://hacktoberfest.digitalocean.com/ label Jul 30, 2021
@jlugao
Copy link

jlugao commented Aug 25, 2021

Hey, first of all, thank you for the awesome project. I'd like to know what is the status on this issue?
I think that this might be useful to test some side-effects in the app being tested (I could do things like directly check the value of something in the db using sqlalchemy).

Can I help with this issue? I might be able to contribute with some code, but I'd need some guidance.

@camilamaia
Copy link
Member Author

Hi @jlugao! Thank you very much 💜

This is not a simple issue, but it is a really nice one. It would be awesome to have it implemented. There is an open PR from last year that started the development: #324

We would need to:

  1. rebase it 😱
  2. test it and check what it is missing

If you want, I can tackle the number 1, since I have more familiar with the changes.

@jlugao
Copy link

jlugao commented Aug 25, 2021

@camilamaia that would be awesome. I would have no idea what to do on the rebase. But I sure can tackle the number 2.

@camilamaia
Copy link
Member Author

@camilamaia that would be awesome. I would have no idea what to do on the rebase. But I sure can tackle the number 2.

@jlugao Awesome! Number one is done ✅ #324 (comment)

@camilamaia
Copy link
Member Author

Closing this issue for now since there was no recent activity 🧹

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

No branches or pull requests

5 participants