feat: upgrade HTTP request calls from query to update upon canister's request

[paulyoung]

Edit: the code in this PR no longer infers when to make an update call, it does it upon the canister’s request.

---

This is a first pass at inferring when to make an update call instead of a query call based on HTTP request methods.

It's based on this discussion thread in the developer forum. Thanks to @nomeata for some feedback and help getting to this point.

I think this could be made a lot more configurable (perhaps via a file that is read when icx-proxy starts which maps methods/routes to functions) but what's here suits my needs for now so I thought I'd share it in case there's any interest in merging.

Thanks in advance for considering this change.

---

import Text "mo:base/Text";

actor {
  type HeaderField = (Text, Text);

  type Token = {};

  type StreamingCallbackHttpResponse = {
    body : Blob;
    token : Token;
  };

  type StreamingStrategy = {
    #Callback : {
      callback : shared Token -> async StreamingCallbackHttpResponse;
      token : Token;
    };
  };

  type HttpRequest = {
    method : Text;
    url : Text;
    headers : [HeaderField];
    body : Blob;
  };

  type HttpResponse = {
    status_code : Nat16;
    headers : [HeaderField];
    body : Blob;
    streaming_strategy : ?StreamingStrategy;
  };

  public query func http_request(request : HttpRequest) : async HttpResponse {
    {
      status_code = 200;
      headers = [];
      body = Text.encodeUtf8("Response to " # request.method # " request (query)");
      streaming_strategy = null;
    };
  };

  public shared func http_request_update(request : HttpRequest) : async HttpResponse {
    {
      status_code = 200;
      headers = [];
      body = Text.encodeUtf8("Response to " # request.method # " request (update)");
      streaming_strategy = null;
    };
  };
};

$ icx-proxy --fetch-root-key
version: 0.2.1
 May 27 18:24:15.771 INFO Log Level: INFO
 May 27 18:24:15.797 INFO Starting server. Listening on http://127.0.0.1:3000/

$ curl -v -X GET -H "Content-type: application/json" -H "Accept: application/json" -d '{}' 'http://localhost:3000?canisterId=rrkah-fqaaa-aaaaa-aaaaq-cai'
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET /?canisterId=rrkah-fqaaa-aaaaa-aaaaq-cai HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.64.1
> Content-type: application/json
> Accept: application/json
> Content-Length: 61
>
* upload completely sent off: 61 out of 61 bytes
< HTTP/1.1 200 OK
< content-length: 31
< date: Fri, 28 May 2021 01:27:24 GMT
<
* Connection #0 to host localhost left intact
Response to GET request (query)* Closing connection 0

$ curl -v -X POST -H "Content-type: application/json" -H "Accept: application/json" -d '{}' 'http://localhost:3000?canisterId=rrkah-fqaaa-aaaaa-aaaaq-cai'
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 3000 (#0)
> POST /?canisterId=rrkah-fqaaa-aaaaa-aaaaq-cai HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.64.1
> Content-type: application/json
> Accept: application/json
> Content-Length: 61
>
* upload completely sent off: 61 out of 61 bytes
< HTTP/1.1 200 OK
< content-length: 33
< date: Fri, 28 May 2021 01:27:41 GMT
<
* Connection #0 to host localhost left intact
Response to POST request (update)* Closing connection 0

[hansl]

Hi @paulyoung ! Nice to see you :D

We're working towards a proposal system for those big changes to protocols. There are a lot of details here that will bring problems so I want to make sure we take them all into account.

I suggest you wait for this new system and that we work together for a better design document. You can champion the changes if you want to take the work.

Cheers!
Hans

[hansl]

As a note I'll keep this PR open to make sure we don't forget about it.

[paulyoung]

@hansl sounds good, please keep me posted.

[paulyoung]

I just wanted to leave a note for myself to say that (assuming it works for my use case) I might prefer the approach @nomeata took in ic-http-lambda, which automatically upgrades query requests to update requests.

[nomeata]

I wouldn't say automatically, it upgrades upon canister's request

[paulyoung]

I think I’ve convinced myself that @nomeata’s “upgrade” approach is the way to go.

I explain why that is here: https://forum.dfinity.org/t/is-it-possible-to-build-a-rest-api-in-a-canister/5355/4?u=paulyoung

I’m happy to make further changes to that effect but I’ve been holding off until the proposal system @hansl mentioned is ready.

[paulyoung]

I made changes to this effect but haven't pushed them yet. I wanted to try them out some more locally first.

[paulyoung]

I think it would be worth introducing http_request_query as well for consistency. We could keep http_request as a wrapper around that for backwards compatibility for now.

[paulyoung]

I think it would be worth introducing http_request_query as well for consistency. We could keep http_request as a wrapper around that for backwards compatibility for now.

Should we do this?

[paulyoung]

Is there anything I can do to help move this along?

I’ve heard that there’s a HTTP Gateway spec that relates to this but I don’t think that’s been made public.

[nomeata]

Sounds like a “community proposal” in https://forum.dfinity.org/t/megathread-community-submissions-for-dfinity-foundation-s-roadmap/6175/55 might be a good way to get this on the agenda.

[paulyoung]

Thanks, done: https://forum.dfinity.org/t/megathread-community-submissions-for-dfinity-foundation-s-roadmap/6175/59?u=paulyoung

[ghost]

@paulyoung , would you please push a change that fixes the formatting and lint CI errors? Then we'll get this merged. Thanks!

[jplevyak]

Having the canister request the upgrade is certainly one approach, but it seems to me that many times the client may know what it wants. Should we not have a way for the client to specify that it wants an update e.g. in a header?

[paulyoung]

In my use case, many clients already exist and work with an existing API spec. It would be a big blow if I couldn’t control this on the canister side.

[jplevyak]

@paulyoung I would have both, client chosen and canister driven.

[nomeata]

If you have a dedicated client that has application specific knowledge anyways, then often you'd just to plain IC calls, instead of piggybacking on HTTP as an extra layer here. So client-specified is much less important and pressing than canister side.

(And clients can just call http_update so in a way this design incldes client chosen…)

[jplevyak]

I agree that the client could use IC calls, but sometimes e.g. forms it might be easier to use plain HTTP.

[nomeata]

Right, but then the canister could just do it as well, which keeps the client (and thus the overall mental complexity for the developer) simple.

How would you even let the client choose? A custom header? Hard to do in a plain form…

I'm not strongly opposed, but it's a distraction from the highly relevant and somewhat urgent proposal by Paul that we should get in.

[jplevyak]

Agreed that this is more trouble than it is worth at this point. LGTM

[jplevyak]

We need a spec and service worker change AFAICT @nomeata ?

[nomeata]

There is a spec on some internal Notion page. Or actually two, one for the HTTP gateway feature and one for the certification protocol. That ought to be moved somewhere more proper (possibly a new section of the Interface Specification?), and then extended.

This would also help to uncover things like this: in update calls, we can't use certified variables to certify responses. But that's ok, as update responses are certified by the system! So no problem, but worth pointing out, and implementing as such in the service worker.

[jplevyak]

When we submit this, let's create an issue to upgrade the service worker to allow upgrades on the http asset path.

[levifeldman]

Hey looks like we are good to go on this feature, Is there something I can do to help move this forward?

[paulyoung]

Does anyone have any advice on what to do here now?

icx-proxy was removed from this repo in #279

[paulyoung]

I guess I should open a PR at https://github.com/dfinity/icx-proxy for the changes here that are specific to that.

[nomeata]

Does anyone have any advice on what to do here now?

I think we™ need to

• Write a proper specification (like the Interface Spec) for the HTTP Gateway protocol as it works now (including streaming and certification) in some suitable, ideally public place. Could be the icx-proxy repo for lack of better place, although I like it when specs are kept at a distance from implementation, lest they are confused with docs :_)
• Write up the necessary change to that protocol to enable the feature outlined here. This hopefully helps us to think through the interaction between upgrade-to-update and streaming, and other aspects.
• Implement in icx-proxy and the service worker (where is that hosted now?)
• Profit.

[ghost]

Closing this PR since icx-proxy now lives at https://github.com/dfinity/icx-proxy

[paulyoung]

I opened #291 and dfinity/icx-proxy#6.