Skip to content

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

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

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

D-James-GH
Copy link
Contributor

@D-James-GH D-James-GH commented Jul 24, 2022

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.

@teto
Copy link
Collaborator

teto commented Aug 8, 2022

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
Copy link
Collaborator

teto commented Aug 8, 2022

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

@D-James-GH
Copy link
Contributor Author

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?

@teto
Copy link
Collaborator

teto commented Aug 9, 2022

For the second issue, I would like utils.replace_vars to take into account the updated "context",

body = body .. utils.replace_vars(line)
I think utils.replace_vars should accept a dict of variables to use, which I've tried in teto@8462ec2 . 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

@D-James-GH
Copy link
Contributor Author

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.

@teto
Copy link
Collaborator

teto commented Aug 10, 2022

sure !

@D-James-GH
Copy link
Contributor Author

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.

@teto
Copy link
Collaborator

teto commented Aug 12, 2022

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 ?

@D-James-GH
Copy link
Contributor Author

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.

@teto
Copy link
Collaborator

teto commented Dec 12, 2022

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 #159 being the first part of the plan

@D-James-GH
Copy link
Contributor Author

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.

@teto
Copy link
Collaborator

teto commented Dec 30, 2022

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 ?

@NTBBloodbath
Copy link
Member

@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.

@D-James-GH
Copy link
Contributor Author

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

@teto
Copy link
Collaborator

teto commented Jan 4, 2023

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.

@D-James-GH
Copy link
Contributor Author

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.

@teto
Copy link
Collaborator

teto commented Jan 4, 2023

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 ?

@D-James-GH D-James-GH force-pushed the feature/selectable-json-env branch from 0f7b39d to 8c1d031 Compare January 4, 2023 20:03
@teto
Copy link
Collaborator

teto commented Jan 4, 2023

make lint should fix the lint job

@D-James-GH
Copy link
Contributor Author

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

feat: running a response script to set env
@D-James-GH D-James-GH force-pushed the feature/selectable-json-env branch from 8c1d031 to b83ea48 Compare January 4, 2023 21:27
@teto teto merged commit 090e253 into rest-nvim:main Jan 4, 2023
@teto
Copy link
Collaborator

teto commented Jan 5, 2023

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 ?

@D-James-GH
Copy link
Contributor Author

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?

@teto
Copy link
Collaborator

teto commented Jan 6, 2023

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 !

@D-James-GH
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

3 participants