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

Add HTTP cookies get/set JS interface #323

Merged
merged 7 commits into from
Oct 26, 2017
Merged

Add HTTP cookies get/set JS interface #323

merged 7 commits into from
Oct 26, 2017

Conversation

robingustafsson
Copy link
Member

@robingustafsson robingustafsson commented Sep 25, 2017

This PR adds an easier to use (than header manipulation) cookie interface to the JS API, implementing #68.

The API has slightly changed from #68. It now looks like this:

import http from "k6/http";
import { check, group } from "k6";

export default function() {
    group("Simple cookies", function() {
        let cookies = {
            name: "value1",
            name2: "value2"
        };
        let r = http.get("http://httpbin.org/cookies", { cookies: cookies });
        check(r, {
            "status is 200": (r) => r.status === 200,
            "has cookie": (r) => r.cookies.name.length > 0
        });
    });

    group("Advanced cookies", function() {
        let cookie = { name: "name3", value: "value3", domain: "httpbin.org", path: "/cookies" };
        let r = http.get("http://httpbin.org/cookies", { cookies: [cookie] });
        check(r, {
            "status is 200": (r) => r.status === 200,
            "has cookie": (r) => r.cookies.name3.length > 0
        });
    });
}

The accessor interface returns a map of all currently known cookies for the requested URL (those that are in the VU cookie jar) with cookie name as the key and an array of cookie values (to support multiple cookies with same name).

The cookie objects that are used in the "advanced" setter interface can contain the the following properties:

  • name
  • value
  • domain
  • path
  • expires
  • max_age
  • secure
  • http_only

@robingustafsson
Copy link
Member Author

robingustafsson commented Sep 25, 2017

I've now also added draft (hidden) documentation for this functionality: https://docs.k6.io/v1.0/docs/cookies

@ppcano
Copy link
Contributor

ppcano commented Sep 26, 2017

@robingustafsson

I have read the doc and example and I have some questions:

  • Does the simple cookie case allow to set additional cookie properties?
name=my nameValue; expires=Thu, 18 Dec 2013 12:00:00 UTC
  • What is the content of r.cookies.name3? It is the cookie value property or the whole cookie value. Shouldn't it return a Cookie object?

  • Are response.cookies returned when Cookies are configured through http header manipulation?

@robingustafsson
Copy link
Member Author

@ppcano

  • No, the simple cookie interface is for key/value "session cookies" only (they have no Expires or Max-Age attribute set, so expires when "browser session" is over, in the case of k6 when the VU/script iteration completes). The advanced interface allows for setting "persistent cookies" by specifying Expires and Max-Age attributes, although in the context of k6 there's no persistence beyond a VU/script iteration.

  • r.cookies.name3 is an array, so you'd access the first value by indexing: r.cookies.name3[0]. The reason for this being a list is explained in the docs; the cookie RFC allows for multiple cookies with the same name (but differing on attributes like domain and path). It only return the value of the cookie as a string. response.cookies did originally return full cookie objects but I then realized that I didn't get any of the attributes beyond name and value back from the underlying Go stdlib cookiejar implementation. It can of course be done by "subclassing" the stdlib cookiejar but I'd like to hear of that use case first (from request context you know the domain, path and secure values by looking at the request URL, you also now that the cookie hasn't expired or it wouldn't be returned).

  • Yes, the underlying Go cookiejar will see/handle both Set-Cookie headers and the cookies set through the cookie API as well as prepare Cookie header(s) to send with requests. So if you don't use the cookie API all regular cookie management of receiving, storing and sending will be handled automatically for you.

@ragnarlonn
Copy link

A late comment around the API here: I don't like the fact that http.get(url, { cookies: x }); manipulates global state (for the whole test or for a single VU). I think any parameters given to an http.get() or other request-generating method should only affect that particular request/transaction, not any that may come after it. It's more intuitive if it works that way. A cookie jar is great for global manipulation of all subsequent requests, but an option used for a specific HTTP request should only affect that request, IMO.

@liclac
Copy link
Contributor

liclac commented Oct 4, 2017

Yeah, I agree with @ragnarlonn, it makes no sense from a user's perspective for an argument to a request to make an implicit global state manipulation.

@robingustafsson
Copy link
Member Author

Thanks @ragnarlonn, @ppcano and @liclac for feedback!

Ok, how does the following changes sound:

  • VUs have a "global" cookie jar by default
    • cookies seen in responses (Set-Cookie) are added to cookie jar
    • you get access to cookie jar through http.getCookieJar()
  • ResponseObject.cookies contains full http.Cookie objects (name, value, domain, expires etc.) representing cookies seen in response Set-Cookie header(s) only (so not all matching cookies sent to and received from the host is question, as the behavior is now)
  • Dates of http.Cookie object are represented as JS Date objects (or Epoch timestamps with millisecond precision?)
  • User can create local cookie jars and switch to use them, rather than "global" jar, on a per request basis by passing in jar request param
  • Cookies passed into a request using the cookies request param are per-request overrides, won't be added to active cookie jar and can on a per-cookie basis decide whether to replace cookies with same name from active jar

Examples:

Global cookie jar:

export default function() {
    // Add cookie to global cookie jar
    let jar = http.getCookieJar();
    jar.set("name", "value", { domain: "example.com", path: "/" });

    // Global cookie jar is active for requests by default
    let res = http.get("https://example.com/");

    // Get object of active cookies (only name/value) for a particular URL
    let cookiesForURL = jar.getCookiesForURL("https://example.com/");
}

Response object cookies:

export default function() {
    let res = http.get("https://example.com/set-cookie?my_cookie=my_value");
    check(res, {
        "has correct value": (r) => r.cookies.my_cookie[0].value === "my_value",
        "has correct expires": (r) => r.cookies.my_cookie[0].expires > Date.now()
    });
}

Local cookie jars:

export default function() {
    // Create local cookie jar
    let jar = new http.CookieJar();
    jar.set("name", "value", { domain: "example.com", path: "/" });

    // When setting cookie jar like this it will only be "active" for the duration of this request/response.
    let res = http.get("https://example.com/", { jar: jar });

    // You can create multiple cookie jars, and switch which one is "active" for a particular requests
    let jar2 = new http.CookieJar([
        { name: "name2", value: "value2", domain: "example.com", path: "/" }
    ]);
    http.get("https://example.com/", { jar: jar2 });
}

Per-request cookie overrides:

export default function() {
    // Simple key/value setting of cookies, will be added to outgoing request 
    // along with cookies from active jar (but cookie specified with same name as 
    // cookie in jar won't override, just be appended)
    let res = http.get("https://example.com/", { cookies: { name: "value", name2: "value2" } });

    // Set cookie with option to replace any existing cookie in jar with same name 
    let res = http.get("https://example.com/", { cookies: { name: { value: "name", replace: true }, name2: "value2" } });
}

@liclac
Copy link
Contributor

liclac commented Oct 5, 2017

Sounds good! Just two things:

  1. I don't think we should have the "get" prefix on methods, try to make them concise. The "get" prefix is very... Java-y.
  2. Let's use epoch in milliseconds, instantiating date objects all the time that may not be used is a little much. Better to have the user do new Date(ts) themselves in that case.

@robingustafsson robingustafsson force-pushed the feature/goja-cookies branch 2 times, most recently from f0a2d0d to caabac6 Compare October 18, 2017 10:44
@robingustafsson
Copy link
Member Author

@liclac @ppcano @ragnarlonn I've now updated the PR according to agreed changes above.

Some changes:

  • jar.getCookiesForURL() is now jar.cookiesForURL()
  • http.getCookieJar() is now http.cookieJar()

If anyone has other suggestions for how to name these functions then please let me know.

Note: one test is failing as it depends on #324.

@liclac
Copy link
Contributor

liclac commented Oct 18, 2017

I like this. I'm just gonna hold off on merging it until #324 is merged, since the API on that one is still in flux.

@robingustafsson
Copy link
Member Author

@liclac Have now:

  • Rebased on master
  • Fixed the breaking test that needed the redirects PR merged
  • Moved cookiejar specific code to separate cookiejar.go file

@liclac liclac merged commit 7c9f913 into master Oct 26, 2017
@liclac liclac deleted the feature/goja-cookies branch October 26, 2017 13:56
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.

4 participants