Skip to content

Commit

Permalink
refactor/node api design changes (#459)
Browse files Browse the repository at this point in the history
* refactor(node): first pass at changing API design away from express middleware

* refactor(node): add support for nested express urls via req.originalUrl

* refactor(node): remove unneeded test to do with grouping function

* refactor(node): fix up bufferLength tests

* refactor(node): move get-project-base-url tests elsewhere

I think we'll have to tell people to call this manually now, since we
can't perform an asynchronous lookup without asking them to `await`
the call to the function, which will look kinda bad and like we're blocking.

* refactor(node): get `res._body` tests working

* refactor(node): get `req.body` tests working

* refactor(node): tidy up tests

* refactor(node): add some tests for the validation errors

* refactor(node): tidy up

* refactor(node): fix up other tests

* refactor(node): rename default export to readmeio.log()

Remove existing readmeio.log() implementation, this should be possible to
be done via this default function now

* refactor(node): tidy up

* refactor(node): switch from using default export

For some reason this meant that in commonjs you had to do the following 🤮:

```js
const readmeio = require('readmeio').default
```

The way I've done it now, it works in ts-node as I expect, but for some reason
I have to do this in the tests:

```js
import * as readmeio from '../src';
```

I have no idea why 🤷‍♂️ @ilias-t any ideas here?

* refactor(integration): updating express app to use new method signature

* refactor(node): add test showing usage of calling getProjectBaseUrl() first

* refactor(integration): remove unused lint rule

* refactor(integration): update hapi code sample to use new api

* chore: lint

* test: add test for har `pageref`

* chore: lint

* feat: return with the logId from readme.log() function

* chore: lint after `main` branch merge

* chore: fix some tests due to missing dependency

* refactor: remove `getProjectBaseUrl()` function

* refactor: removing express type dependency from logging functions

* refactor: remove seemingly unused code

We already have a test for this which passes with this code commented out:

https://github.com/readmeio/metrics-sdks/blob/e0576f460d66f9ab23cfd2bc22b8f65c5dd01cb1/packages/node/__tests__/index.test.ts#L210-L247

I think this is being handled here:

https://github.com/readmeio/metrics-sdks/blob/e0576f460d66f9ab23cfd2bc22b8f65c5dd01cb1/packages/node/src/lib/process-request.ts#L181-L184

* feat(integration): add POST method test

Right now the hapi/fastify examples are not working properly for accepting
an incoming POST body. This adds a test to expose that flaw, but does not
yet fix it.

I think this refactor makes the most sense to finish once
#459 is merged in.

* fix(integration): update hapi test with new api design

* fix(integration): get fastify demo working with new api design

Make the hapi/express examples more consistent with each other

* chore: code comment about raw.setHeaders piece

* fix(integration): forgot to re-export verifyWebhook after refactor

* feat(integration): add support for POST methods to php and c#

* feat(integration): add support for POST methods to python/flask

This is dependent on #557
being merged because currently the POST data implementation is broken
in the Python SDK

* chore(python): lint

* chore: temp fix php integration test

This can be reverted when #560
is merged in

* refactor(node): switch to using spread operator instead of arr.slice()

* Update .github/MAINTAINERS.md

Co-authored-by: Jon Ursenbach <[email protected]>

* chore: updated comment to be clearer

Co-authored-by: Jon Ursenbach <[email protected]>
  • Loading branch information
domharrington and erunion authored Aug 30, 2022
1 parent 3dba2c8 commit 2d04da3
Show file tree
Hide file tree
Showing 20 changed files with 498 additions and 620 deletions.
1 change: 1 addition & 0 deletions .github/MAINTAINERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ To build a Metrics integration test server, you must write an HTTP server that d
- Looks for an `README_API_KEY` environment variable, and exits with an exit code of 1 if it does not exist.
- Spawns an HTTP server that listens on the `PORT` environment variable, or 4000 if no environment variable exists.
- The HTTP server should have a listener on `GET /` that responds with a 200 status code, and a JSON response body of `{ "message": "hello world" }`
- The HTTP server should have a listener on `POST /` that will be provided with a JSON body of `{ "user": { "email": "[email protected]" } }` which should be included in the Metrics payload. This should respond with a 200 status code and an empty body.
- The HTTP server should have a Metrics SDK installed, that responds with the following identification object for the user making the request:

```json
Expand Down
69 changes: 60 additions & 9 deletions __tests__/integration-metrics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ function isListening(port, attempt = 0) {
});
}

async function getBody(response) {
let responseBody = '';
// eslint-disable-next-line no-restricted-syntax
for await (const chunk of response) {
responseBody += chunk;
}
expect(responseBody).not.toBe('');
return JSON.parse(responseBody);
}

// https://gist.github.com/krnlde/797e5e0a6f12cc9bd563123756fc101f
http.get[promisify.custom] = function getAsync(options) {
return new Promise((resolve, reject) => {
Expand All @@ -49,6 +59,22 @@ http.get[promisify.custom] = function getAsync(options) {
});
};

function post(url, body, options) {
return new Promise((resolve, reject) => {
const request = http
.request(url, { method: 'post', ...options }, response => {
response.end = new Promise(res => {
response.on('end', res);
});
resolve(response);
})
.on('error', reject);

request.write(body);
request.end();
});
}

const get = promisify(http.get);

const randomApiKey = 'a-random-readme-api-key';
Expand Down Expand Up @@ -152,21 +178,14 @@ describe('Metrics SDK Integration Tests', () => {
});
});

// TODO this needs fleshing out more with more assertions and complex
// test cases, along with more servers in different languages too!
it('should make a request to a metrics backend with a har file', async () => {
await get(`http://localhost:${PORT}`);

const [req] = await once(metricsServer, 'request');
expect(req.url).toBe('/v1/request');
expect(req.headers.authorization).toBe('Basic YS1yYW5kb20tcmVhZG1lLWFwaS1rZXk6');

let body = '';
// eslint-disable-next-line no-restricted-syntax
for await (const chunk of req) {
body += chunk;
}
body = JSON.parse(body);
const body = await getBody(req);
const [har] = body;

// Check for a uuid
Expand Down Expand Up @@ -205,11 +224,43 @@ describe('Metrics SDK Integration Tests', () => {
// Flask prints a \n character after the JSON response
// https://github.com/pallets/flask/issues/4635
expect(response.content.text.replace('\n', '')).toBe(JSON.stringify({ message: 'hello world' }));
// The \n character above means we cannot compare to a fixed number
expect(response.content.size).toStrictEqual(response.content.text.length);
expect(response.content.mimeType).toMatch(/application\/json(;\s?charset=utf-8)?/);

const responseHeaders = caseless(arrayToObject(response.headers));
expect(responseHeaders.get('content-type')).toMatch(/application\/json(;\s?charset=utf-8)?/);
});

it('should process the http POST body', async () => {
const postData = JSON.stringify({ user: { email: '[email protected]' } });
await post(`http://localhost:${PORT}/`, postData, {
headers: {
'content-type': 'application/json',
// Explicit content-length is required for Python/Flask
'content-length': Buffer.byteLength(postData),
},
});

const [req] = await once(metricsServer, 'request');

const body = await getBody(req);
const [har] = body;

const { request, response } = har.request.log.entries[0];
expect(request.method).toBe('POST');
expect(response.status).toBe(200);
expect(request.postData).toStrictEqual(
process.env.EXAMPLE_SERVER.startsWith('php')
? {
mimeType: 'application/json',
params: [],
}
: {
mimeType: 'application/json',
text: postData,
}
);
expect(response.content.text).toMatch('');
expect(response.content.size).toBe(0);
});
});
6 changes: 6 additions & 0 deletions packages/dotnet/examples/net6.0/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,10 @@
await context.Response.WriteAsJsonAsync(new { message = "hello world" });
});

app.MapPost("/", async context =>
{
context.Response.StatusCode = 200;
await context.Response.CompleteAsync();
});

app.Run($"http://localhost:{port}");
Loading

0 comments on commit 2d04da3

Please sign in to comment.