rest.nvim icon indicating copy to clipboard operation
rest.nvim copied to clipboard

feat: Add option for json env and allow manipulating env files during the session

Open D-James-GH opened this issue 3 years ago • 9 comments

This addresses #110 as well as allowing the env files to be updated using a lua script inside {% script %} tags, similar to the intellij rest client. The lua code in the script tag will is passed a context table which includes the response, json_decode and a function to write/set environment variables. This is mostly useful for myself when I am working with authenticated apis.

example:

POST https://jsonplaceholder.typicode.com/posts

{
  title: 'foo',
  body: 'bar',
  userId: 1,
}

{% 

local body = context.json_decode(context.result.body)
context.set_env("postId", body.id)

%}

PATCH https://jsonplaceholder.typicode.com/posts/{{postId}}

{
  title: 'foo',
}

Environment files can also easily be swapped using the :RestSelectEnv command meaning you can have a .env.staging and a .env.production or a staging.env.json and a production.env.json.

D-James-GH avatar Jul 24 '22 19:07 D-James-GH

I had started writing a similar script but instead of the {% %} approach I was using > /my_script.sh. Your PR looks better so I tried it, one issue I have is that if I import an external body then I can't specify the script in my "test.http" file, e.g.

POST {{URL}}/test
Content-Type:application/json
Accept:application/json;charset=utf-8

< /home/teto/Simple.json

{% 

local body = context.json_decode(context.result.body)
print(body.id)
context.set_env("postId", body.id)
print("HELLO WORLD")

%}

the test script is ignored because the current code is looking for the post processing script in /home/teto/Simple.json while I would rather keep it in the test.http file

EDIT: I can get it to work by modifying the get_current_request_function to

  local headers, headers_end = get_headers(bufnr, start_line, end_line)

  local body_start = headers_end
  local body, script_line = get_body(bufnr, body_start, end_line)
  log.fmt_debug("Identified body as:\n %s", body)

  local script_str
  -- if script_line ~= nil then
  --   log.debug("getting response script")
  script_str = get_response_script(bufnr, headers_end, end_line)
  log.fmt_debug("Script string:\n%s", script_str)
  -- end

i.e., looking for the script_str regardless of the body

teto avatar Aug 08 '22 22:08 teto

another complaint is that I would like utils.replace_vars to be able to use the variables set via context.set_env which doesn't seem to work yet: For instance I set MODEL_PI in a previous request from the same .http file via context.set_env("MODEL_PI", 42) yet this fails

POST {{URL}}/simulation_manager/run
Content-Type:application/json
Accept:application/json;charset=utf-8

{
  "model": {{MODEL_PI}},
}

with rest.nvim] Failed to get the current HTTP request: ...pack/packer/start/rest.nvim/lua/rest-nvim/utils/init.lua:196: bad argument #2 to 'gsub' (string/function/table expected) due to this snippet

    -- Ignore commented lines with and without indent
    if not utils.contains_comments(line) then
      body = body .. utils.replace_vars(line)
    end

teto avatar Aug 08 '22 23:08 teto

Thank you for the feedback. I have updated the pull request so that the script works with external body's. I can't currently recreate your second problem, here is the http file i am using.

POST https://jsonplaceholder.typicode.com/posts
Content-Type: application/json

< ./body.json

{% 
local body = context.json_decode(context.result.body)
context.set_env("new_var", body.id)
%}

PATCH https://jsonplaceholder.typicode.com/posts/{{postId}}

{
  "title": {{new_var}}
}

Context.set_env sets "new_var" in my .env.json file

Do you have an env file set for context.set_env to write to? Or do you have a http file that i can use for testing?

D-James-GH avatar Aug 09 '22 20:08 D-James-GH

For the second issue, I would like utils.replace_vars to take into account the updated "context", https://github.com/rest-nvim/rest.nvim/blob/3c49a8cbf075ff49260402e7ec4ce27814fc89b3/lua/rest-nvim/request/init.lua#L70 I think utils.replace_vars should accept a dict of variables to use, which I've tried in https://github.com/teto/rest.nvim/commit/8462ec2aac9ec0e7ba414bfab2f5b2c93b235649 . This change could be beneficial also in terms of perf (fewer reloading of the .env).

Would you mind rebasing please ? I get errors about "b:current_syntax" that should be fixed on main

teto avatar Aug 09 '22 23:08 teto

I see, yes I think that is a good idea. Are you happy for me to edit that on this pull request? As it’s actually code that was in use before.

D-James-GH avatar Aug 10 '22 06:08 D-James-GH

sure !

teto avatar Aug 10 '22 07:08 teto

I have added an optional vars table to replace_vars and in get body moved the read_variables out side the loop for performance reasons. If no vars table is passed it will call read_variables inside replace_vars.

D-James-GH avatar Aug 11 '22 20:08 D-James-GH

Just a note to myself, it would be nice to have a command to dump the env seen by rest-nvim.

As for this MR, I think I have an issue: I have several {% ... %} sections in my .http files but the first one is called when It should be the second one ? Do you see the same thing ?

teto avatar Aug 12 '22 10:08 teto

Yes I do, It is to do with the way I am finding the start script line. I am finding the script_start relative to the header end. Eg. If your headers end on buffer line 40 then the script starts on line 45 the script start line will be 5. I have fixed this.

I am also looking into an issue when an env_variable has a newline character in. Not sure if this is a general issue or related to this PR.

D-James-GH avatar Aug 12 '22 10:08 D-James-GH

just so you know, I am looking forward to merging if not all at least parts of the MR, but I would like to make it possible to add some tests first, with this https://github.com/rest-nvim/rest.nvim/pull/159 being the first part of the plan

teto avatar Dec 12 '22 16:12 teto

No problem, thats a great idea, I am happy add tests once 159 is merged in. I am also happy to help with any other part. I will continue to update this branch as it is the branch i personally use daily. Unfortunately I had to add a json_encode to line 88 of request/init.lua as nested json bodies were not sending properly. I realise this is out of the scope of this PR.

D-James-GH avatar Dec 12 '22 18:12 D-James-GH

can you :

  • remove the .idea folder
  • rebase and maybe add an alternative environment in the test

@NTBBloodbath is https://github.com/rest-nvim/tree-sitter-http supposed to be some pure http stuff or could we add support to catch the script {% %} in there ?

teto avatar Dec 30 '22 21:12 teto

@NTBBloodbath is https://github.com/rest-nvim/tree-sitter-http supposed to be some pure http stuff or could we add support to catch the script {% %} in there ?

We can add support to it in the parser as it already covers some rest.nvim specific features. I'll probably make a pure http parser later in another branch of the parser repo too.

NTBBloodbath avatar Dec 31 '22 05:12 NTBBloodbath

will do, it might take a few days as I am away at the moment

D-James-GH avatar Jan 01 '23 17:01 D-James-GH

This branch cannot be rebased due to conflicts

Seems like this was not rebased correctly ? I've just tested it and found an issue when there are multiple queries with no payload, rest-nvim uses the query delimiter --- as a curl flag. This is a general bug in rest-nvim and not in this PR though I think so it should not prevent the merge.

teto avatar Jan 04 '23 12:01 teto

hmm interesting, I am not seeing any conflicts on github's sync branch. There have been a lot of updates to this pull request since it's inception as main has changed a lot, does a merge --squash have conflicts?

I also noticed the "---" bug when testing, but as you say I didn't want to adjust the tests that i hadn't written.

D-James-GH avatar Jan 04 '23 14:01 D-James-GH

well I can't do anything from the github UI, and anyway there are 19 commits in the MR which is a bit much. Can you try a git fetch --all && git rebase -i origin/main ?

teto avatar Jan 04 '23 15:01 teto

make lint should fix the lint job

teto avatar Jan 04 '23 20:01 teto

ahh yeah, sorry, that was a left over from the rebase

D-James-GH avatar Jan 04 '23 21:01 D-James-GH

so I did not notice before but this rewrites the .env file ? that's kind of annoying for my usecase since it records errors in my carefully handcrafted .env. Is there a way to disable that behavior ?

teto avatar Jan 05 '23 10:01 teto

Yes currently the set_env function takes what was previously in the env file, updates the one you are setting then writes it back to the file. There are currently no session env variables to add to, each request reads the env file separately which is why updating the variables there seamed useful, also this is the way I do it when using postman.

As for your issue, what do you think about introducing something like a "session_env" where you could call "set_session_var" this could save the variable to the users config/in-memory table? Personally I find the writing of the env variables to a file useful as I use it for an access_token and refresh_token which i want to persist, however, I think it is a great idea to accommodate different work flows if you have ideas of what would suit you best?

D-James-GH avatar Jan 05 '23 17:01 D-James-GH

Maybe just dont call write_env_file in set_env and let the user call it ? I think that's the best compromise, we keep the functionality while being less destructive. We should not override the values by default, in my case I lost my .env which is not versioned because it contains secrets and keeps changing for my test sake and it was not a big issue but it could be for others. The user calls write_env_file at the end of his request.

Drawback is you lose the automatic update of the env. Other options could be:

  1. a flag "write" in set_env that defaults to false
  2. have a "environment" object that can be configured as read-only/on-demand/autowrite (in which introduce a module utils/varenv.lua ).

NB: I've been playing with treesitter it's very promising and should make the plugin much robust than what we have right now !

teto avatar Jan 06 '23 01:01 teto

Okay, I understand. The problem right now is that without write_env_file the set_env doesn't do anything which might be confusing. We could rename set_env to write_env_file for now if you think what makes it clearer? I think set_env should at least store the value in memory somewhere, as you suggest with a "varenv" module, then I like your suggestion of a "write" flag.

Also do you mind sharing the shape of the env that was overridden as I think that sounds like an unintended bug which should be fixed. The write_env_file should only update the one variable in set_env.

Great news about treesitter.

D-James-GH avatar Jan 06 '23 15:01 D-James-GH