Integrate graphql crunch

[zephraph]

Graphql Crunch is a library that flattens deeply nested object hierarchies into an array and deduplicates references in the array. This could yield some nice performance benefits, especially when used with complex queries.

This implementation follows their docs example and only crunches when the request has the crunch query param. It could just as easily be added as a header though. Ultimately this allows for incremental adoption across the Artsy clients.

I'd like to add some sort of validation around this functionality and it'd also be beneficial to benchmark against some actual Artsy queries.

[zephraph]

Only apply when

1. crunch is supplied as a query param
2. There's actual data to return
3. The current response isn't an introspection query

[mzikherman]

This looks cool! I like the 'opt-in' pattern, it can thus be safely deployed w/o any feature flag (although a spec for the behavior as you mention would be nice).

We certainly have lots of deeply nested queries so it would be interesting to see what the results of some benchmarks look like.

Congrats on the first PR!

[zephraph]

I was going down the path of writing tests and ran into a few interesting issues.

1. The docs in graphql-crunch are targeting apollo-server-express, not express-graphql. The latter doesn't have formatResponse (though there is a PR). That means the initial implementation didn't even work.
2. This isn't written in a way that's testable in isolation. I decided to go down the route of creating a custom middleware just for crunching and pulling that out into lib. Turns out express-graphql never calls next() as a part of its middleware... therefore it won't yield execution to another middleware in the chain. The issue was reported here with no official word on fixes, but there are a couple workarounds.

The ultimate solution I came up with isn't quite as hacky feeling as the things mentioned in the end of 2, but I'd definitely value input on the approach.

I found a library called express-interceptor which gives you the ability to intercept and alter the body of a response before it's returned from express. This allows us to add the middleware before express-graphql but have the crunching happen after express-graphql has resolved. I do have a concern about having to parse and re-stringify the body in the interceptor, but I'm not sure how big of an issue that will be.

As for testing, I added a library called supertest that allows the assertion of a request to express or similar http frameworks. It's an integration test of sorts because it tests the flow from express to express-graphql then to graphql-crunch via the interceptor. The good news is that it's a test we can run in jest so it's not going to require a real live environment to test against.

[zephraph]

The expectation is that a crunched result will be an array whereas a non-crunched result will be an object. This isn't a very sophisticated test, but it at least provides some level of validation. I'm not convinced this should even be a thing though... more on that below.

[zephraph]

Can anyone see why checking for __schema here would be necessary? That part comes more or less straight from the docs, but I'm assuming if the client was adding a crunch param then it already has immediate crunch handling before the results are actually processes by the client's gql library. Also, it doesn't seem like an exhaustive search for introspection queries... I mean, there's a lot of other queryable fields, right? At the very least it doesn't hurt anything so I left it there for discussion.

[zephraph]

Also there are two other things that I'm a bit concerned with here.

1. parsing the body at the beginning of the interceptor and stringifying it at the end seems like it could negatively impact perf
2. I'm not sure if the interceptor will be triggered on an error response. I likely need to add a more robust check in the isInterceptable method.

[orta]

I'm pretty sure the __schema comes from the official recommendation on generating a full introspection query, basically to generate the data that fills GraphiQL.

[orta]

Ah yeah, good point, those clients wouldn't be adding the crunch param, so wouldn't see it at all

[zephraph]

Even if a client did do an introspection query with the crunch flag, so long as the uncrunch step happened on the client before it was passed to the graphql layer I'd think that it wouldn't matter. I reached out to @jamesreggio, maybe I'm just missing something.

[jamesreggio]

Hey there — just confirmed that including the __schema condition was an unnecessary mistake.

We built graphql-crunch as a drop-in replacement for graphql-deduplicator, which is a lossy compressor that depends upon Apollo-specific behavior. The graphql-deduplicator example code includes that condition, and we accidentally cargo-culted it into our README.

All that to say, it's safe to remove.

Also, hey @orta 👋

[zephraph]

Sweet, thanks!

[orta]

Makes sense 👋

[peril-staging]

New dependencies added: express-interceptor, graphql-crunch and supertest.

express-interceptor

Author: AxiomZen

Description: A tiny interceptor for Express responses

Homepage: https://github.com/axiomzen/express-interceptor#readme

Created	about 3 years ago
Last Updated	7 months ago
License	MIT
Maintainers	2
Releases	6
Direct Dependencies	debug
Keywords	express, expressjs, middleware, modify, response, hack, interceptor, hijack and hijackresponse

README

express-interceptor

A tiny Express response interceptor

Express-interceptor allows you to define a previous step before sending a response. This allows you to do anything you want with the response, such as processing, transforming, replacing, or logging it. Express-interceptor allows you to avoid calling next() over and over. Further more, you can avoid managing nested scopes. Using a declarative API, it’s simple to use and maintain.

Some use cases include:

• Transpile custom elements into standard HTML elements.
• Transform JSX or compile less files on the fly.
• Store statistics about responses.
• Set response headers based on tags in the response body.
• Dynamically inject live-reload scripts to HTML.

Usage

Install the package

npm i --save express-interceptor

Define your interceptor

var express     = require('express');
var cheerio     = require('cheerio');
var interceptor = require('express-interceptor');

var app = express();

var finalParagraphInterceptor = interceptor(function(req, res){
  return {
    // Only HTML responses will be intercepted
    isInterceptable: function(){
      return /text\/html/.test(res.get('Content-Type'));
    },
    // Appends a paragraph at the end of the response body
    intercept: function(body, send) {
      var $document = cheerio.load(body);
      $document('body').append('<p>From interceptor!</p>');

      send($document.html());
    }
  };
})

// Add the interceptor middleware
app.use(finalParagraphInterceptor);

app.use(express.static(__dirname + '/public/'));

app.listen(3000);

You're defining an interceptor that will be executed whenever the response contains html as Content-Type. If this is true, it will append a child at the end of the body. In other words, it will transform the response:

<html>
<head></head>
<body>
  <p>"Hello world"</p>
</body>
</html>

Into:

<html>
<head></head>
<body>
  <p>"Hello world"</p>
  <p>From interceptor!</p>
</body>
</html>

See more examples. Also, you can debug express-interceptor actions using debug env variable DEBUG=express-interceptor.

API

• isInterceptable() (required): is a predicate function where you define a condition whether or not to intercept a response. Returning true buffers the request, and proceeds calling intercept() as well as afterSend(). Typically, you want to check for this condition in the res object in the definition of the middleware.

• intercept(body, send) (required): Parse the body as an encoded string. After processing the body, call send(newBody) with the content to be sent back to the client.

• afterSend(oldBody, newBody): This method will be called after sending the response to the client – after the done() callback in the send() method is executed. This method would typically be used to cache something, log stats, fire a job, among other things.

Who is using it?

• node-stylus-require
• express-html-minify
• express-jare
• hospitalrun-server-routes

Similar to

• express-hijackresponse
  Different API, using callbacks with no top down structure, more obtrusive to HTTP internals.

• hijackresponse
  Attempting to solve same problems than express-hijackresponse. Different API using pipes, recommended if you have problems with streaming or if you need more control over responses.

• tamper
  Similar functionality but different internals and API.

Words of advice

If your intercept method make calls to a database, or needs to make expensive transformations to the original response, you should take in account the time that will add to the response. You should define your isInterceptable method carefully, checking the type of the response, or even the route that was fired by the request, also you may consider implementing a cache strategy to get faster responses.

If you face any issue, don't hesitate to submit it here.

Contributing

Please check CONTRIBUTING.md.

This project adheres to the Open Code of Conduct. By participating, you are expected to honor this code.

Author

• AxiomZen.

Collaborators

• Fabiano Soriani.
• Raul Pino.

License

MIT

graphql-crunch

Author: Banter.fm

Description: Normalizes GraphQL responses by reducing duplication, resulting in smaller payloads and faster JSON parsing.

Homepage: https://github.com/banterfm/graphql-crunch#readme

Created	3 months ago
Last Updated	3 months ago
License	MIT
Maintainers	1
Releases	6
Keywords	graphql, apollo, crunch, compress, deduplicator and normalize

This README is too long to show.

supertest

Author: TJ Holowaychuk

Description: SuperAgent driven library for testing HTTP servers

Homepage: https://github.com/visionmedia/supertest#readme

Created	almost 6 years ago
Last Updated	3 days ago
License	MIT
Maintainers	5
Releases	38
Direct Dependencies	methods and superagent
Keywords	superagent, request, tdd, bdd, http, test and testing

README

SuperTest

HTTP assertions made easy via superagent.

About

The motivation with this module is to provide a high-level abstraction for testing
HTTP, while still allowing you to drop down to the lower-level API provided by superagent.

Getting Started

Install SuperTest as an npm module and save it to your package.json file as a development dependency:

npm install supertest --save-dev

Once installed it can now be referenced by simply calling require('supertest');

Example

You may pass an http.Server, or a Function to request() - if the server is not
already listening for connections then it is bound to an ephemeral port for you so
there is no need to keep track of ports.

SuperTest works with any test framework, here is an example without using any
test framework at all:

const request = require('supertest');
const express = require('express');

const app = express();

app.get('/user', function(req, res) {
  res.status(200).json({ name: 'john' });
});

request(app)
  .get('/user')
  .expect('Content-Type', /json/)
  .expect('Content-Length', '15')
  .expect(200)
  .end(function(err, res) {
    if (err) throw err;
  });

Here's an example with mocha, note how you can pass done straight to any of the .expect() calls:

describe('GET /user', function() {
  it('respond with json', function(done) {
    request(app)
      .get('/user')
      .set('Accept', 'application/json')
      .expect('Content-Type', /json/)
      .expect(200, done);
  });
});

One thing to note with the above statement is that superagent now sends any HTTP
error (anything other than a 2XX response code) to the callback as the first argument if
you do not add a status code expect (i.e. .expect(302)).

If you are using the .end() method .expect() assertions that fail will
not throw - they will return the assertion as an error to the .end() callback. In
order to fail the test case, you will need to rethrow or pass err to done(), as follows:

describe('POST /users', function() {
  it('responds with json', function(done) {
    request(app)
      .post('/users')
      .send({name: 'john'})
      .set('Accept', 'application/json')
      .expect(200)
      .end(function(err, res) {
        if (err) return done(err);
        done();
      });
  });
});

You can also use promises

describe('GET /users', function() {
  it('responds with json', function() {
    return request(app)
      .get('/users')
      .set('Accept', 'application/json')
      .expect(200)
      .then(response => {
          assert(response.body.email, '[email protected]')
      })
  });
});

Expectations are run in the order of definition. This characteristic can be used
to modify the response body or headers before executing an assertion.

describe('POST /user', function() {
  it('user.name should be an case-insensitive match for "john"', function(done) {
    request(app)
      .post('/user')
      .send('name=john') // x-www-form-urlencoded upload
      .set('Accept', 'application/json')
      .expect(function(res) {
        res.body.id = 'some fixed id';
        res.body.name = res.body.name.toUpperCase();
      })
      .expect(200, {
        id: 'some fixed id',
        name: 'john'
      }, done);
  });
});

Anything you can do with superagent, you can do with supertest - for example multipart file uploads!

request(app)
.post('/')
.field('name', 'my awesome avatar')
.attach('avatar', 'test/fixtures/avatar.jpg')
...

Passing the app or url each time is not necessary, if you're testing
the same host you may simply re-assign the request variable with the
initialization app or url, a new Test is created per request.VERB() call.

request = request('http://localhost:5555');

request.get('/').expect(200, function(err){
  console.log(err);
});

request.get('/').expect('heya', function(err){
  console.log(err);
});

Here's an example with mocha that shows how to persist a request and its cookies:

const request = require('supertest');
const should = require('should');
const express = require('express');
const cookieParser = require('cookie-parser');

describe('request.agent(app)', function() {
  const app = express();
  app.use(cookieParser());

  app.get('/', function(req, res) {
    res.cookie('cookie', 'hey');
    res.send();
  });

  app.get('/return', function(req, res) {
    if (req.cookies.cookie) res.send(req.cookies.cookie);
    else res.send(':(')
  });

  const agent = request.agent(app);

  it('should save cookies', function(done) {
    agent
    .get('/')
    .expect('set-cookie', 'cookie=hey; Path=/', done);
  });

  it('should send cookies', function(done) {
    agent
    .get('/return')
    .expect('hey', done);
  });
})

There is another example that is introduced by the file agency.js

API

You may use any superagent methods,
including .write(), .pipe() etc and perform assertions in the .end() callback
for lower-level needs.

.expect(status[, fn])

Assert response status code.

.expect(status, body[, fn])

Assert response status code and body.

.expect(body[, fn])

Assert response body text with a string, regular expression, or
parsed body object.

.expect(field, value[, fn])

Assert header field value with a string or regular expression.

.expect(function(res) {})

Pass a custom assertion function. It'll be given the response object to check. If the check fails, throw an error.

request(app)
  .get('/')
  .expect(hasPreviousAndNextKeys)
  .end(done);

function hasPreviousAndNextKeys(res) {
  if (!('next' in res.body)) throw new Error("missing next key");
  if (!('prev' in res.body)) throw new Error("missing prev key");
}

.end(fn)

Perform the request and invoke fn(err, res).

Notes

Inspired by api-easy minus vows coupling.

License

MIT

Generated by 🚫 dangerJS

[zephraph]

Okay, I think this is ready for final review.

I cleaned up the crunchInterceptor test file and created helpers in src/tests to assist in setting up a graphql-express instance and mocking out interceptors. I also added another test to ensure the interceptor isn't called on an error case. Added response checks on each of the existing tests.

Let me know what you think.

[mzikherman]

This looks awesome to me, with the opt-in and tested functionality I'm 👍 to merging and maybe looking at some benchmarks!

Test-wise, also looks great to me! I guess our existing tests aren't actually integration-y in that we just run queries directly against the schema using graphql. At first I thought it might be nice to have an integration test for crunch that actually makes a request against the server, but that seems complicated. The current test is nice since its testing the code contained in the interceptor itself. I'm just wondering (but in no way is it a blocker) if it might be worth it for a test asserting that the interceptor is located 'properly' in the stack (before the express-graphql stuff).

But that's just wishful thinking....this is an impressive PR! 😎

[zephraph]

I think there's a lot we could do around testing in general. It's something I'd love to talk more about after I start. Until then, this has been a fun intro to metaphysics. 😄

[mzikherman]

ok, merging! 😎 Great work!

[alloy]

I’d love to see a benchmark against Emission/Relay using e.g. the Home query (and thus also adding support to Emission/Relay for consuming crunched responses).

[zephraph]

I'll open a PR.