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

Provide possibility to extract values from response #140

Merged
merged 47 commits into from Feb 27, 2018

Conversation

Projects
None yet
7 participants
@cbrevik
Copy link
Contributor

cbrevik commented Dec 3, 2017

As stated in #74 (comment), this is a naive proof-of-concept implementation of #74.

It's inspired by @fmartingr's suggestion of "piping" the response to a variable name like;
POST url/auth -> authResponse.

The goal would be to allow the developer refer to the response by a variable name, and use a "dotted" property approach to access its values.

This implementation should work with something like:

@baseUrl = https://jsonplaceholder.typicode.com
GET {{baseUrl}}/posts/1 -> myResponse

###

GET {{baseUrl}}/users/{{myResponse.body.userId}}

Opening PR for discussion of approach. I do not consider this to be a ready solution yet. Would love input from @Huachao.

Things to discuss:

  • How to parse response.body (and possibly other HttpResponse values?). This naive approach does a JSON.parse automatically on body, which is not satisfactory. Maybe adding jsonBody, xmlBody, etc helper properties like @fmartingr suggested is one approach.
  • This approach means the response variable can be accessed across files, because it is saved statically on ResponseStore. Should it be tied to the file which ran the request instead? Or is there value in cross-file sharing? Can be confusing if variables start overwriting each other on name collisions.
  • Possible to support autocomplete like with other variables? Dotted autocomplete?
  • Other things I haven't thought of?
Add simple response caching to variable name
And add ability to extract response values through dotted
variable name
@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 4, 2017

@cbrevik @fmartingr @akalcik thank you all, and @cbrevik, this is a really nice patch and makes a solid step towards implementing Chain Request feature. And after we finalized the design here, we can continue working on this PR to make the code all ready, before that I still have some questions to discuss with you. Any feedback and suggestions are welcomed.

  1. Since we need to create a syntax to identify the response, what's the proper syntax. And @cbrevik has provided a cool way to use ->, like GET https://api.example.com -> exampleResp, or define request name in comment to reference later like following:
###
# @name ExampleRequest
GET https://api.example.com

###
  1. For an already defined request/response name, what's the scope of this identifier, is it file scope or global scope? That means, for the former, if we define a response named loginResponse, we can't use the same name in other request files. If we use the same name, the response cache would corrupt. And for the later, maybe we should include the file path as part of the cache key. However, how to handle untitled files, e.g., we have multiple files named Untitled - 1.
  2. What's the life time of the stored/cached response? If we store them in cache(memory), when we restart vscode, these responses won't be available, and those requests which reference other responses won't succeed. Or we need to persist the response to disk to overcome this.
  3. I'd like to provide several response reference types as you do, like Raw Body, Body Elements(with syntax similar to XPath/JSONPath) and Response Header Value. Do you think it's enough to cover most requirements.
  4. I really like your idea of storing the previous responses, while user may wish the ability to view corresponding response to decide which response section needs to reference. Or at least show the actual value when user hove in each of response reference place.

Again, thank you all for your great contribution to this extension. 😄

@akalcik

This comment has been minimized.

Copy link

akalcik commented Dec 4, 2017

@Huachao @cbrevik @fmartingr

we should go agile and try to make simple and usable solution, which can be enhanced later.

  1. I'm fine with the syntax, we can use | instead of ->, but no really matters.
  2. Ok with file scope
  3. Ok with persistence in the memory
  4. Yes

When we are working with the JSON we could probably use same JSON Query Language, like https://marketplace.visualstudio.com/items?itemName=joshwong.vscode-jmespath or http://www.jsoniq.org/

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Dec 4, 2017

  1. It might be that @Huachao's approach here (adding a @name tag in a comment # is a good idea. The problem with adding -> is that it is not exactly HTTP standard compliant. Also doing it with a tags means we could maybe add additional functionality. Say we want the response.body deserialized from JSON could be done by adding a @json tag. I.e.
    # @json @name ExampleRequest
    GET https://api.example.com
    Might also be more extensible, @xml, etc. Thoughts?
  2. File scope should probably be the default. But again, if we use "tags" we could use something like @global or whatever to give it global scope. Maybe sometimes we want to overwrite/use a variable cross-file?
  3. I agree that there might be a need to cache stuff on disk at some point, but that is maybe out-of-scope for this feature? I think in-memory is good enough for now.
  4. Is there a need to go all-out XPath? Since both properties and array-indices can be accessed through brackets ([ ]), maybe response.body.someItemArray.0.itemId dotted syntax is okay? Seems like XPath at least has a heavy and often misunderstood syntax.
@fmartingr

This comment has been minimized.

Copy link

fmartingr commented Dec 4, 2017

  1. Having the variable name in a comment before it would make for future request configuration for more stuff and also make the file compatible with other editor's rest-client plugins, so 👍
  2. File scope is enough IMO.
  3. I think that using memory is enough too, if you're trying to do a request that references another one that you did not run, the request could either fail silently or throw an error explaining to the user that a previous request is required.
  4. In most scenarios the user would only want the serialized body to use response parameters in another request, we just need to identify what content-types to support first (json/xml?)

I'm not denying/ignoring the comments for more enhancements here but I'm focusing only to reach a MVP feature set where this is usable and when is ready and launched iterate more over it.

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 5, 2017

@fmartingr @cbrevik @akalcik thank you all for your quick responses. And I summarize the answers for developing this feature:

  1. Use comment as the syntax to define referenced request, e.g.,
# @name login
POST https://api.example.com/login HTTP/1.1
Content-Type: application/json

{
   "user":  "xxx",
   "password": "yyy"
}

###

GET https://api.example.com/docs HTTP/1.1
Authorization: Bearer {{login.resp.headers.token}}
  1. Set request name to file scope, document uri needs to be part of cache key.
  2. Use in memory cache to store referenced response, and if the referenced response is not found in memory(e.g., restart vscode), display a warning message box.
  3. Judge response content type(JSON/XML) from response Content-Type header, and use JSONPath/XPath syntax for each case. And currently only these two formats are supported.

Do you agree with this design, and any other suggestions? 😊

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Dec 5, 2017

Agreed on all points :) But I'm still wondering about number 4. When you say XPath syntax, do you mean full syntax like this with node/attribute retrieval, predicates, etc. Or just a simple root/someNode[0]/childNode[1].

Is full syntax easy to support in JavaScript?

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 5, 2017

@cbrevik I can't agree with you more. And maybe we can first support JSON path only, and leave the XPATH(or related solution) later.

@fmartingr

This comment has been minimized.

Copy link

fmartingr commented Dec 5, 2017

I agree with that as first iteration. I don't know how complex will be the XPath for XML because I haven't developed a lot with JS/TS but if it comes bundled in it could be nice to have at least a working way to use them.

Have any of the people from #140 mentioned they want this for XML explicitly? Maybe we are trying to support it when no one has requested it (yet).

Add comment based response variable naming
Cache by file name + variable key
@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Dec 5, 2017

I don't think XML support is strictly necessary as an MVP either.

I've done a preliminary implementation at cbrevik@f51bc85

Not finished of course. I'll merge into this PR (or create a new PR) when it is working better. And we can iterate from there.

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 6, 2017

@cbrevik thanks so much, and waiting for your updated PR.

@akalcik

This comment has been minimized.

Copy link

akalcik commented Dec 8, 2017

Actually I liked the first proposal of @cbrevik much more! Defining variables in comment seems to be very confusing. What about this one? This is pretty clean and aligned with the current syntax.

@myResponse
@baseUrl = https://jsonplaceholder.typicode.com
GET {{baseUrl}}/posts/1 -> @myResponse

@myResponse is in this case own declared variable. We can introduce later also convention based variables (for example @jsonMyResponse) for variables which content will be deserialized automagically.

@myResponse
@jsonMyResponse
@baseUrl = https://jsonplaceholder.typicode.com
GET {{baseUrl}}/posts/1 -> @myResponse | @jsonMyResponse

@fmartingr @Huachao

"...file compatible with other editor's rest-client plugins, so..."

Just confused. Is the file compatible now? I think to make file compatible with other editor's we need to redesign the syntax completely.

cbrevik added some commits Dec 9, 2017

Ugly diagnostic code for variables
Refactor incoming
@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Dec 10, 2017

@akalcik The -> is sort of invented syntax, which might be even more confusing for people.
Of course the variables {{variable}} is "invented" syntax which is non-valid as well. But they are typical of "merge fields", think handlebar.js, etc.
Using comment-based @tags seems a bit cleaner to me, and more extensible.

@Huachao I've pushed a few more changes to this PR, reverting the old commit. It's working in a very basic manner, with red diagnostic squiggly lines if variable reference is not in memory.
Basic code completion of response variables, basic hover with printed value.

Just thought I'd check in here. Not done yet. Needs more error handling, better response.body parsing, and will refactor so it's a bit cleaner.

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 12, 2017

@akalcik sorry for the late to response to your comment about response declaration syntax.

@cbrevik Thanks again for your great contribution for this feature.

@akalcik @cbrevik @fmartingr I'd like to share with you about my consideration for three most requested features(#67 #74 #86) which I think all related to this PR.

I have several reasons about why personally I suggest to use a syntax in comment not in request line.
As @cbrevik mentioned, use @tag in comment seems more extensible and cleaner to me. I think what the syntax represents is not just define response variable, it's more of a way to add metadata to the underling request. So with this syntax we can have more powerful capabilities in the future, like supporting API request documentation(#67), response assertion/validation.

And a key difference of our proposals is that I use request name instead of response name to reference the response. Many of existing REST API tools(e.g., Postman, Insomnia) have the ability to add name for the request, so this way has a good compatibility if we later want to import/export data from those tools. We even have the ability to reference other request's request part, not only response. And when user references other request/response with name, we can also easily know the request dependency, even further we can trigger dependent request automatically.

Another reason that I don't want to use -> to represent creating a new variable is that I just want to create a file scope variable with original syntax: @variableName: variableValue. Since it's more explicit to know what the value of variable value should be. So when I am considering another requested feature(#86), I can also leverage from this feature to store token in variable automatically with little cost, like following:

# @name login
POST https://api.example.com/login
Content-Type: application/json

{
  "user_name": "foo",
  "password": "bar"
}

###

@token = {{login.resp.body.access_token}}

...

Below is a full example http file for this issue:

# @name listDocs
# @descirption A test request to list all the docs
GET https://api.example.com/docs
Authorization: token 123456

###

# @name createDoc
# @status_code 201
# @resp_content_type application/json
POST https://api.example.com/docs
Authorization: {{listDocs.req.headers.Authorization}}
Content-Type: application/json

{
  "author": "foo",
  "content": "bar"
}

###

# @name getDoc
GET https://api.example.com/docs/{{createDoc.resp.body.id}}
@akalcik

This comment has been minimized.

Copy link

akalcik commented Dec 12, 2017

@Huachao @cbrevik I get your points. Still confused to understand some language design decisions. Please help me.

When is a file scope variable created it is like this:

@variableName = variableValue

Name of this variable is a variableName and value is variableValue, scope is a file. Actually no brainer. We can call this type of variable a constant as the value can't be change again.

Now:

# @name listDocs
# @description A test request to list all the docs
GET https://api.example.com/docs
Authorization: token 123456

In this case the we have a two variables with name name and description. Scope of those variables seems to be also file. Now the most confusing part for me ist following. listDocs seems to be a somehow placeholder for the request, which can be used later. I don't really know how I should name it properly. Now what is a value of description variable? A test request to list all the docs, right? In one case value of the variable is a placeholder, in other case seems to be specific value. Not sure if the value of listDocs and description can be changed later. My question is: what define what can I use as a value? It is by a convention? When it is a convention then I understand. I would call than the variables like request scoped variables and it shouldn't be possible to change the values later. When it is convention than I would propose to change name to request.

What also I would propose to change is the missing operator. In one case is value set per = in other case act (empty space) as a operator.

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Dec 13, 2017

@akalcik the @tag defined before each request in comments, I'd like to call it metadata not variable. So in this way, you don't create two variables named name and description, just add two metadata for underlying request, one is name which can be used as an identifier of the request to reference, another is description which may used to add some description of the request(which can be later export to other REST tools). You can also think of them as Attributes of the method compared with C#(I guess maybe you are familiar with).

So the metadata names are predefined for special use, currently seems we only need the name metadata for this feature, since the name metadata is used as identifier of the request, if not defined this metadata, I think the request is an anonymous request, which can't be referenced by other requests. And the value of these metadata can be changed as your wish, just remind that, name metadata is special since it's an identifier of the request and can be used by other requests, so if you update the name, you also need to update the reference parts(that means the value of @name metadata is file scope, just like a function name).

And the syntax of adding metadata to request just brought from JSDoc, I don't have many good ideas either, I agree with @akalcik that this seems a little confusing with file scope variable declaration, @cbrevik @akalcik @fmartingr what kind of syntax do you think appropriate for this?

Thanks again @akalcik @cbrevik @fmartingr , without your discussions, I can't think about it deeply.

@narayanan

This comment has been minimized.

Copy link

narayanan commented Dec 13, 2017

Good Discussion. How about using ! as the meta character to declare the metadata?. This will also allow declaring metadata out side a comment.

!name - mechanism to declare a file scope variable of type Javscript object which can be used to access various aspects of the request / response
!description - mechanism to describe the purpose of the API invocation

Example

!name @listDocs
!descirption "A test request to list all the docs"
GET https://api.example.com/docs
Authorization: token 123456

###

!name @createDoc
!status_code 201
!resp_content_type "application/json"
POST https://api.example.com/docs
Authorization: {{listDocs.req.headers.Authorization}}
Content-Type: application/json

{
  "author": "foo",
  "content": "bar"
}

###

!name @getDoc
GET https://api.example.com/docs/{{createDoc.resp.body.id}}
@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Jan 10, 2018

@cbrevik any updates on this, sorry to bother you.

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Jan 10, 2018

@Huachao I've been extremely busy since Christmas. So I haven't had a chance to work on this, sorry. I'll try to devote some more time this weekend.

It's basically in a working state. The difference from your proposal is that I've only cached the Response. So it needs to be updated to cache the Request as well. Other than that, my previous comment stands about needing better error handling, refactoring, etc.

@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Jan 11, 2018

@cbrevik that's ok, I am sorry that I didn't intend to push you, and I've fixed some conflicts and pushed into your branch.

@lostfields

This comment has been minimized.

Copy link

lostfields commented Jan 18, 2018

I really like this, when will it hit marketplace?

For xml and other strange format, you could support regex as well, lets say you have the captured data in myDoc after using #@name myDoc

In a sub-sequent request you may define variable @mycapture = /(\b\w+\b)/.exec({{myDoc}})[1] to capture the first word in response myDoc, then continue to use {{mycapture}} anywhere you want. This will make it support any responses as text, xml and json.

I'm not too sure about the syntax of naming response, since tripple hash (###) is really a part of the previous request when you are folding/collapsing a request. Anyway, this naming of a response should also including naming of the request for easy navigation of some kind.

"use strict";

export enum VariableType {
Custom,

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

How about rename Custom to File

@@ -35,5 +35,6 @@ export enum ElementType {
Authentication,
SystemVariable,
EnvironmentCustomVariable,
FileCustomVariable
FileCustomVariable,
RequestVariable

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

How about rename to RequestCustomVariable to be consistent with other user-level custom variables

);
}

public static async getVariableDefinitionsInFile(document: TextDocument): Promise<Map<string, VariableType[]>> {

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

Make modifier to private seems enough

This comment has been minimized.

@cbrevik

cbrevik Feb 24, 2018

Contributor

Sure, was thinking this is a utility-method which can be used in the future. But it can be made public again then. I'll update 👍

const firstPartRegex: RegExp = /^(\w+)\.$/;
const secondPartRegex: RegExp = /^(\w+)\.(request|response)\.$/;

export class RequestVariableCompletionItemProvider implements CompletionItemProvider {

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

How about only allow these completion items when the first part or so called variable name is a request variable?

This comment has been minimized.

@cbrevik

cbrevik Feb 24, 2018

Contributor

You mean checking if the request variable exists at all in memory? I'm not sure that is developer friendly, personally I like having completion on "known" valid syntax, even if it won't run.

Is there a good reason to do that?

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

@cbrevik I don't express myself clearly, I just mean this completion item hint only make sense for request variable, not for other types of variables. So it's not necessary for request variables loaded in memory, just exist or already declared in file is enough.

And except this, other code LGTM. 👍 Really great pull request!

This comment has been minimized.

@cbrevik

cbrevik Feb 24, 2018

Contributor

I was thinking maybe the check at the top was enough;

if (!VariableUtility.isPartialRequestVariableReference(document, position)) {
    return [];
}

Which basically checks if the path is using a dot, which would by definition would be a request variable. (Since only that type uses dotted values).

But it does not check if the variable name is declared in the file as a request variable (# @name variableName), is that what you're thinking should also be checked?

This comment has been minimized.

@cbrevik

cbrevik Feb 24, 2018

Contributor

I've added a check for if the variable name is defined in the file as a request variable (and not necessarily in memory)

return undefined;
}

const parsedBody = JSON.parse(body as string);

This comment has been minimized.

@Huachao

Huachao Feb 24, 2018

Owner

How about check the Content-Type header first in the request or response, and check if it's JSON, if so, do as is. Otherwise, simply return the body?

@Huachao
Copy link
Owner

Huachao left a comment

CIL

cbrevik added some commits Feb 24, 2018

@cbrevik cbrevik force-pushed the cbrevik:response-to-var branch from 82916fb to 7459df3 Feb 24, 2018

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Feb 24, 2018

Think I've adressed the comments now 👍

@@ -67,6 +74,19 @@ export class RequestVariableCompletionItemProvider implements CompletionItemProv
return completionItems;
}

private checkIfRequestVariableDefined(document: TextDocument, variableName: string) {
const text = document.getText();
const regex = new RegExp(Constants.RequestVariableDefinitionRegex, 'mg');

This comment has been minimized.

@Huachao

Huachao Feb 26, 2018

Owner

Why not directly construct a regex with given variableName, so that if we can found one in text means True, otherwise False.

completionItems = props.map(p => {
let item = new CompletionItem(p);
item.detail = `(property) ${p}`;
item.documentation = p;

This comment has been minimized.

@Huachao

Huachao Feb 26, 2018

Owner

How about make the value of documentation to the actual value of p? User can better know what's the actual value will be inserted into the request.


completionItems = props.map(p => {
let item = new CompletionItem(p);
item.detail = `(property) ${p}`;

This comment has been minimized.

@Huachao

Huachao Feb 26, 2018

Owner

Does user really need the (property) prefix


const valueAtPath = RequestVariableCacheValueProcessor.getValueAtPath(variableValue, fullPath);
if (valueAtPath) {
let props = Object.getOwnPropertyNames(valueAtPath);

This comment has been minimized.

@Huachao

Huachao Feb 26, 2018

Owner

Minor: Why not use Object.keys(), since your one will also include not enumerable properties?

@Huachao
Copy link
Owner

Huachao left a comment

Minor Comments

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Feb 26, 2018

Fixed!

@Huachao Huachao merged commit 3a8a103 into Huachao:master Feb 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Huachao

This comment has been minimized.

Copy link
Owner

Huachao commented Feb 27, 2018

@cbrevik Merged, thanks, really a great patch 👍 . And I will publish this in next release.

@cbrevik

This comment has been minimized.

Copy link
Contributor

cbrevik commented Feb 27, 2018

Awesome, thanks for the help landing this @Huachao! It's going to be fun using this day-to-day.

Huachao added a commit that referenced this pull request Feb 28, 2018

Huachao added a commit that referenced this pull request Mar 1, 2018

Huachao added a commit that referenced this pull request Mar 1, 2018

Huachao added a commit that referenced this pull request Mar 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment