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

Make REST and GraphQL server running on the same port #1905

Closed
3 tasks
dhmlau opened this issue Oct 24, 2018 · 23 comments
Closed
3 tasks

Make REST and GraphQL server running on the same port #1905

dhmlau opened this issue Oct 24, 2018 · 23 comments

Comments

@dhmlau
Copy link
Member

dhmlau commented Oct 24, 2018

Description / Steps to reproduce / Feature proposal

Originated from the discussion in #1899 (comment)

Currently the REST server is running at http://localhost:3000 and using oasgraph loosely, the GraphQL server will be running at http://localhost:3001.
Ideally, it would be REST and GraphQL should share the same port.

Acceptance Criteria (needs discussion/ to be refined)

  • Use the generated OpenAPI schema by the library (already installed by --enable-graphql in our application) and get an internal representation of the GraphQL Schema
  • Use that generated GraphQL Schema to create an internal end-point (ie: /lb4graphql) by the internal express and use the express-graphql to consume that schema
  • (optional) Create a CLI option --enable-graphql

cc @bajtos @marioestradarosa

@bajtos
Copy link
Member

bajtos commented Oct 25, 2018

See the discussion in https://github.com/strongloop/loopback-next/pull/1899/files#r227358497 where I am advocating for a new extension package.

Cross-posting it here for posterity.

Does it make sense to have it in a separate package?, it takes two packages and a couple of lines to implement it.

If we keep graphql built into our @loopback/rest package, then we end up with a tight coupling to a specific semver-major version of oasgraph . Whenever oasgraph makes breaking changes, we need to release a new semver-major vresion of @loopback/rest.

We have been bitten by this kind of tight coupling many times in LB 3.x. I don't remember a single time when we would wish an independent extension was built into the framework. There were many times we wished a built-in feature was packaged as a standalone extension from the beginning: offline sync, authentication & authorization layer, etc.

Also by forcing all @loopback/rest consumers to depend on oasgraph, we make it harder for people not using GraphQL to avoid possible security vulnerabilities in oasgraph and its dependencies.

Last but not least, while oasgraph may be a good solution for prototyping GraphQL support, I think it's not suitable for production deployment on sites with a decent amount of traffic, because it cannot optimize database queries to fetch data from multiple related models in a single SQL query. I'd like us to look into ways how to allow LB4 app developers to leverage full power of database access and relations in the GraphQL API, see #656 and https://github.com/strongloop/loopback4-extension-graphql. If we bundle oasgraph as the default GraphQL implementation, we are nudging app developers to use a tool that may shoot them in their foot in a near future of their project.

@marioestradarosa
Copy link
Contributor

The extension package would be living inside the loopback-next as a repo?

@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

The extension package would be living inside the loopback-next as a repo?

Possibly. Although I think it may be better to keep the code in https://github.com/strongloop/loopback4-extension-graphql, at least until we have our own implementation invoking repositories directly (without the oasgraph layer).

@jannyHou
Copy link
Contributor

We have been bitten by this kind of tight coupling many times in LB 3.x. I don't remember a single time when we would wish an independent extension was built into the framework. There were many times we wished a built-in feature was packaged as a standalone extension from the beginning: offline sync, authentication & authorization layer, etc.

+1 For @bajtos 's concern. And I would vote for a new extension package too. A good practice for our extensibility.

A proposal:

  • Allow the component takes in a server instance by injection.
  • If the server is provided, add the graphql route to it, if not then create a new server like the current code.

Feel free to correct the details ^

@bajtos
Copy link
Member

bajtos commented Dec 13, 2018

If the server is provided, add the graphql route to it, if not then create a new server like the current code.

Well if there is no REST server, then there is no REST API to create GraphQL interface for.

IMO, the component should discover all RestServer instances configured by the app and a create a new GraphQL endpoint for each server instance.

In the first iteration, it may be simpler to assume the application is RestApplication and mount the GraphQL interface on the single rest server. Essentially do the same as REST API Explorer does now.

https://github.com/strongloop/loopback-next/blob/fcc76df71f2fe837dd62847418e9aa7e39da575a/packages/rest-explorer/src/rest-explorer.component.ts#L19-L34

@bajtos
Copy link
Member

bajtos commented Dec 13, 2018

Considering the growing importance of GraphQL, I am proposing to make GraphQL a first-class citizen of LB4:

@jannyHou
Copy link
Contributor

jannyHou commented Dec 13, 2018

Suggestion from @raymondfeng :
We could do it in 2 steps:

  • (in the shopping example)Add an express app, mount lb4 app and oasgraph app to it.

  • enable the template above in the CLI

    • like --oasgraph
  • the first-class support for graphql like miroslav's proposal in the comment above

    • the separation(different building blocks) of graphql UI (explorer UI) , graphql schema and graphql resolvers.
    • start with oasgraph and make it extensible to support the native graphql.
      • now we translate lb metadata into openapi spec, oasgraph takes the openapi spec and translate it to graphql spec. During this two-step translation we lose metadata. It would be good if we can translate to native graphql spec directly.

@jannyHou
Copy link
Contributor

summary of estimation meeting:

See the proposal above ^ let's continue discussion.

@bajtos
Copy link
Member

bajtos commented Dec 14, 2018

Let's take a look at how oasgraph-cli mounts the GraphQL on express:

https://github.com/strongloop/oasgraph/blob/8016aa8398fd3812605d378552875f055de05702/packages/oasgraph-cli/src/oasgraph.ts#L111-L137

import * as graphqlHTTP from 'express-graphql'

function startGraphQLServer(oas, port) {
 createGraphQlSchema(oas, {
    strict: program.strict,
    viewer: program.viewer,
    addSubOperations: program.addSubOperations,
    fillEmptyResponses: program.fillEmptyResponses
  }) 
     .then({schema, report}) => {
        // ...
        app.use('/graphql', graphqlHTTP({
          schema: schema,
          graphiql: true
        }))
        // ...
     });

Now as far as I understand GraphQL, it uses a single URL with two verbs GET and POST (see https://graphql.org/learn/serving-over-http/). As a result, it should be pretty easy to build a controller that provides two methods (GET /graphql and POST /graphql) and calls express-graphql under the hood to serve the request. Look at the following existing code for inspiration:

  • REST API Explorer: how it creates a controller with configurable HTTP path
  • StaticAssetsRoute: how it invokes serve-static middleware in async/await friendly way

I think the component should be fairly easy to implement and therefore I prefer to do this properly from the beginning, don't mount the LB4 app on an Express app.

@marioestradarosa
Copy link
Contributor

marioestradarosa commented Dec 15, 2018

@bajtos , but internally there is already an express instance used by the API Explorer, I guess?. If you plugin express-graphql, OASGraph and the app.restServer.getApiSpec() you can have the /graphql in the LB4 app.

The following is a pseudo code that could be running in the same code where the REST Server is exposing the api explorer (I guess, not sure for now). But at the end, the OASGraph is just transcoding from one format to another, and it is the express-graphql and the express server that will finally be calling the LB4 REST endpoints linked to this schema, right?.

const {schema} await OASGraph.createGraphSQSchema(
 app.getServer.getApiSpec(), 
strict: false, 
viewer:true,  
addSubOperations: true, 
);

and then using the internal reference to express (I don't recall very well, but I saw it :-)

express.use('/graphql',  graphqlHTTP(
{
  schema: schema,
   graphiql: true
}

This express object, can then use the same port as the api explorer?. The blocks are there, but I am not sure if this would be the best approach, but sounds simple .

@luxaritas
Copy link

luxaritas commented Dec 17, 2018

Per the early comment by @bajtos:

Last but not least, while oasgraph may be a good solution for prototyping GraphQL support, I think it's not suitable for production deployment on sites with a decent amount of traffic, because it cannot optimize database queries to fetch data from multiple related models in a single SQL query.

I think this is arguably more important than just letting oasgraph run on the same port (which on a cursory look should be pretty straightforward, with a similar conclusion to the above comment). Is this within scope of this conversation/does it require a new issue? Is the final step of the current plan referring to creating a new Server implementation or still using oasgraph as a proxy?

@bajtos
Copy link
Member

bajtos commented Feb 4, 2019

Last but not least, while oasgraph may be a good solution for prototyping GraphQL support, I think it's not suitable for production deployment on sites with a decent amount of traffic, because it cannot optimize database queries to fetch data from multiple related models in a single SQL query.

I think this is arguably more important than just letting oasgraph run on the same port (which on a cursory look should be pretty straightforward, with a similar conclusion to the above comment). Is this within scope of this conversation/does it require a new issue?

As far as I understand oasgraph, it cannot fetch related models in a single query, it's a limitation given by design. We will have to roll out our own GraphQL layer that will leverage model relation metadata and repository implementations. No need to open a new issue for that, it's already tracked by #656 and possibly by strongloop-archive/loopback4-extension-graphql#4

@bajtos
Copy link
Member

bajtos commented Feb 4, 2019

but internally there is already an express instance used by the API Explorer

Not at all. REST API Explorer contributes a Controller class implementing dynamic endpoints (e.g. the main HTML page serving the front-end UI) and calls app.static to expose static front-end assets (e.g. JavaScript and CSS files).

const {schema} await OASGraph.createGraphSQSchema(
 app.getServer.getApiSpec(), 
strict: false, 
viewer:true,  
addSubOperations: true, 
);

This code snippet looks reasonable 👍

express.use('/graphql',  graphqlHTTP(
{
  schema: schema,
  graphiql: true
}

I prefer to use LB4 handlers instead of accessing underlying Express instance. Here is a code snippet to illustrate what I mean:

const urlPath = '/graphql';
const handler = graphqlHTTP({schema, graphiql: true});
const spec = {
  'x-visibility': 'undocumented',
  responses: {},
};

app.route('get', urlPath, spec, handler);
app.route('post', urlPath, spec, handler);

@dougal83
Copy link
Contributor

dougal83 commented Mar 10, 2019

I'm excited by this discussion. I've tried implementing the code above on top of express EDITED LINK.

Personally, I am really excited to see support for graphql coming along.

@sreerajkrishnank
Copy link

Any updates on this?

@dhmlau
Copy link
Member Author

dhmlau commented Jul 29, 2019

@dougal83, seems like your repo is no longer accessible? I got a 404 when clicking on the link and when looking up for the repo as well.

When talking to @bajtos this morning, he also mentioned something similar, i.e. mounting to express app (?). I haven't got a chance to try it out yet, but here is his code snippet:

 // server schema:
  const app = new MyLbApp();
  app.mountExpressRouter('/graphql', graphqlHTTP({
    schema,
    graphiql: true
  }))
  app.start();

@dougal83
Copy link
Contributor

@dhmlau Thanks. I will take a closer look soon.

@SephReed
Copy link

SephReed commented Aug 1, 2019

Would it be possible to just forward one port to the other?

@tritoanst
Copy link

tritoanst commented Oct 22, 2019

This code is OK for me.
Any issue for this?

import {ApiApplication} from './application';
import {ApplicationConfig} from '@loopback/core';
import * as graphqlHTTP from 'express-graphql';
import { createGraphQlSchema } from 'openapi-to-graphql';
import { Oas3 } from 'openapi-to-graphql/lib/types/oas3';

export {ApiApplication};

export async function main(options: ApplicationConfig = {}) {
  const app = new ApiApplication(options);
  await app.boot();
  await app.start();
  const url : string = <string>app.restServer.url;
  console.log(`Server is running at ${url}`);

  const graphqlPath = '/graphql'; 
  const oas: Oas3 = <Oas3>app.restServer.getApiSpec();
  const {schema}  = await createGraphQlSchema(oas, {
    strict: false, 
    viewer:true,
    baseUrl: url,
  } );
  const handler : graphqlHTTP.Middleware = graphqlHTTP({
    schema,
    graphiql: true
  });
  app.mountExpressRouter(graphqlPath, handler );
  console.log(`Graphql: ${url}${graphqlPath}`);

  return app;
}

@christo911
Copy link

Works for me... until JWT from header is is needed...
Seems that the headers are changed by express-graphql, so if you are using JWT Authentication as in
looppback4-example-shopping

Its not possible to get the JWT token (valid) from the header and respective service... then get the 401 unauthorised response.

also setting the context doesn't help

const handler : graphqlHTTP.Middleware = graphqlHTTP( (request, response, graphQLParams) => ({
        schema,
        pretty: true,
        graphiql: true, 
        context:  {  request: request,  response: response},
    }))

If I look at the headers in the loopback App context ( found file sequence.ts), each time I make a post or get, then I get 2 different headers... First one has the Auth JWT, Second one does not (seems to override)

bit lost now..

@christo911
Copy link

Managed to get the JWT this way and works fine for me.

const graphqlPath = '/graphql'
    const oas: Oas3 = <Oas3>app.restServer.getApiSpec()
    const {schema} = await createGraphQlSchema(oas, {
        strict: false,
        viewer: true,
        baseUrl: url,
        headers: {
            'X-Origin': 'GraphQL'
        },
        tokenJSONpath: '$.jwt'
    })

    const handler : graphqlHTTP.Middleware = graphqlHTTP( (request, response, graphQLParams) => ({
        schema,
        pretty: true,
        graphiql: process.env.NODE_ENV === 'development', // Needed to activate apollo dev tools (Without true then interface does not load)
        context: { jwt: getJwt(request) }
    }))

    // Get the jwt from the Authorization header and place in context.jwt, which is then referenced in tokenJSONpath
    function getJwt(req:any) {
        if (req.headers && req.headers.authorization) {
            return req.headers.authorization.replace(/^Bearer /, '');
        }
    }

    app.mountExpressRouter(graphqlPath, handler);
    console.log(chalk.green(`Graphql API: ${url}${graphqlPath}`))

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Dec 25, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants