Add thenCallback method #2

APshenkin · 2018-02-11T08:25:11Z

thenCallback method allow to complete some operations with request before sending response.

example of usage:

await server.get("/mocked-endpoint").thenCallback(req => {
 if (req.body.myVar = 'foo') {
  return {status: 200, body: {obj: req.body.myVar}, headers: {myHeader: 'foo'}}
 }
 return {status: 404, body: 'not found'}
});

Wait for your thought and suggestions

this PR should be merged after #1

pimterry

Nice, I can definitely see how this would be useful, it's been in my plan for a little while but I haven't had any time to get to it, thanks for taking a look! I've added a few comments.

I'd like to ensure as much as possible that users don't need to use this though, so if there are cases where you're hitting this, do file an issue for the kind of handler you'd need to be built in, and we can look at that in future. E.g. in your example above we could have a json matcher for the body, and a json template responder like:

mockServer.get('/mocked-endpoint')
.withJsonMatching({ myVar: 'foo' })
.thenReplyWithTemplate(200, {
  obj: '{{ body.myVar }}'
});

Exact template format needs thought, but you get the idea. A real handler like this can be built to run faster, we can make it work more consistently across platforms etc etc etc, but mainly it just avoids lots of people all reimplementing the same kinds of callbacks.

There's an extra bonus issue here too, that I suspect you'll hit when you try to add tests: this is quite tricky to implement in browsers. To do this well you need some kind of persistent connection, where the server can ask the browser what to do, and hold the request until it responds.

For this first step we can ignore that imo (wrap tests with nodeOnly(() => { ... }), mention this in the docs, and make sure it throws something helpful if you try to use it in a browser). As long as we make sure the interface is something we can implement for browsers in future I'm fine with that, and I think this should work pretty easily as is. Nice work!

pimterry · 2018-02-11T16:27:21Z

src/rules/handlers.ts

@@ -27,7 +29,16 @@ export class SimpleHandlerData {

    constructor(
        public status: number,
-        public data?: string
+        public data?: string,
+        public headers?: http.OutgoingHttpHeaders


Looks like this is still on top of the headers PR - if you rebase on the latest master now these changes should disappear.

pimterry · 2018-02-11T16:28:38Z

src/rules/partial-mock-rule.ts

@@ -25,7 +25,8 @@ import {
    WildcardMatcherData
 } from "./matchers";

-import { SimpleHandlerData, PassThroughHandlerData } from "./handlers";


Just fyi - partial mock rule has now been renamed to mock rule builder (git's conflict resolution should be able to magically handle that for you if you rebase), but there might be some conflicts because I've added jsdoc to all the methods through here. Should be easy to sort out, just a heads up.

pimterry · 2018-02-11T16:28:59Z

src/rules/partial-mock-rule.ts

        };

        return this.addRule(rule);
    }

+    thenCallback(callback: Function): Promise<MockedEndpoint> {
+        const rule: MockRuleData= {


A space before the = here would be nice

pimterry · 2018-02-11T16:30:03Z

test/integration/smoke-test.spec.ts


        let response = await fetch(server.urlFor("/mocked-endpoint"));
+
+        expect(await response.headers.get("myHeader")).to.equal("foo");


This is just the test for the headers - it'd be good to add one for thenCallback too. There's now a separate handlers.spec.ts for that (which I've moved this header test into).

pimterry · 2018-02-11T16:38:44Z

src/rules/handlers.ts

+                formData = await request.body.asFormData();
+            } catch (err) {
+                formData = undefined;
+            }


I don't think we should have all this body parsing logic here. It'd be nice to have the callback be given requests in basically the same format as request verification, and to share the code to do that. Right now that's over here:

https://github.com/pimterry/mockttp/blob/4b21b6b9f401d371abbcee3c3557b37bea2de4b8/src/rules/mock-rule.ts#L60-L74

We carefully avoid doing this before the handler normally, because if we're passing the request through we want to avoid reading the entire body into memory unnecessarily, and it means we can start matching & responding before the request body has even arrived. Here though that's less important of course, as most callbacks are going to do that anyway.

I think we should pull that logic out to turn this into request = waitForCompletedRequest(ongoingRequest), and put the shared logic to somewhere else (maybe in parse-body?), so we can use it both here and in the above code in mock rule. That saves us a bunch of duplication, and makes the whole interface a bit more consistent.

pimterry · 2018-02-11T16:38:57Z

src/rules/handlers.ts

+                path: request.path,
+                headers: request.headers,
+                body: { buffer, text, json, formData }
+            }


Why does the request need cleaning?

this is a bad name for variables :) Will rename it

pimterry · 2018-02-11T16:44:25Z

src/rules/handlers.ts

+            try {
+                ourResponse = await callback(cleanRequest);
+            } catch (err) {
+                throw err;


We should return a clear response with the error here, so it's easier to tell what's gone wrong. This error will appear in the server logs, but it's much easier to debug if we send it as a response (with a failing status code) so it appears in people's test failure message.

What failing status code will be best in this case?

pimterry · 2018-02-11T16:53:42Z

src/rules/handlers.ts

+                throw err;
+            }
+            if (typeof ourResponse.body === 'object') {
+                ourResponse.body = JSON.stringify(ourResponse.body);


I don't think we should automatically encode bodies like this. In future there might be other object structured bodies we'd like to support (like XML or something), and it's a bit too magic. I'd be fine with something a bit more explicit though, like a .json property:

if (!!ourResponse.json) { outResponse.headers['Content-Type'] = outResponse.headers['Content-Type'] || 'application/json'; ourResponse.body = JSON.stringify(ourResponse.json); }

pimterry · 2018-02-11T16:59:27Z

src/rules/handlers.ts

+    readonly type: 'callback' = 'callback';
+
+    constructor(
+        public callback: Function


We should be more specific here than just Function, so we can check usage and properly document what you're allowed here. I'd suggest:

public callback: (request: CompletedRequest) => { status?: number; body?: string; json?: any; headers: { [key: string]: string; }; }

APshenkin · 2018-02-12T07:54:13Z

Just one more tip about why this feature is necessary.

If you want to mock any SOAP service, than you have to implement logic in callback to receive different responses in different cases. thenReplyWithTemplate will not allow to do that. Because of this I created this PR. In our project we want to mock SOAP service, but currently it's not possible(

APshenkin · 2018-03-01T07:52:42Z

@pimterry is there any blockers to merge this feature and update mockttp?

pimterry · 2018-03-02T18:13:43Z

No specific blockers, I just need time to sit down and review it again. Life's got a bit hectic, but I should be able to sort this this weekend.

pimterry · 2018-03-04T17:02:56Z

I've made a couple more tweaks here, but otherwise this all looks good to me now - I'll merge in a sec once the tests pass.

pimterry requested changes Feb 11, 2018

View reviewed changes

Pshenkin Andrey added 2 commits February 12, 2018 09:18

add callback method

75c894b

rebase

efe9510

APshenkin force-pushed the predicate branch from 3fc17ab to efe9510 Compare February 12, 2018 06:20

Pshenkin Andrey added 2 commits February 12, 2018 10:36

fixes after review

55a3aba

fix style

2f890ef

pimterry added 6 commits March 4, 2018 17:41

Move waitForCompletedRequest into a generic request utils method

daff08e

Create a common type for the callback return value

86d3921

Some small style tweaks for callback changes

40e5238

Include callback name in thenCallback explanation, if possible

8f7a578

Return a 500 if a callback handler throws an error

2445671

Simplify callback handling logic a little

af26982

pimterry approved these changes Mar 4, 2018

View reviewed changes

pimterry changed the title ~~[WIP] Add thenCallback method~~ Add thenCallback method Mar 4, 2018

pimterry merged commit a12a44c into httptoolkit:master Mar 4, 2018

BirdTho mentioned this pull request Jun 27, 2018

Can't use mockttp in the browser without a CommonJS bundler #10

Open

RISCfuture mentioned this pull request May 15, 2020

Rules leaking between mock servers? #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add thenCallback method #2

Add thenCallback method #2

APshenkin commented Feb 11, 2018 •

edited

Loading

pimterry left a comment

pimterry Feb 11, 2018

pimterry Feb 11, 2018

pimterry Feb 11, 2018

pimterry Feb 11, 2018

pimterry Feb 11, 2018

pimterry Feb 11, 2018

APshenkin Feb 12, 2018

pimterry Feb 11, 2018

APshenkin Feb 12, 2018

pimterry Feb 11, 2018

pimterry Feb 11, 2018

APshenkin commented Feb 12, 2018

APshenkin commented Mar 1, 2018

pimterry commented Mar 2, 2018

pimterry commented Mar 4, 2018


		let response = await fetch(server.urlFor("/mocked-endpoint"));

		expect(await response.headers.get("myHeader")).to.equal("foo");

Add thenCallback method #2

Add thenCallback method #2

Conversation

APshenkin commented Feb 11, 2018 • edited Loading

pimterry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APshenkin commented Feb 12, 2018

APshenkin commented Mar 1, 2018

pimterry commented Mar 2, 2018

pimterry commented Mar 4, 2018

APshenkin commented Feb 11, 2018 •

edited

Loading