stoplightio / prism

Turn any OpenAPI2/3 and Postman Collection file into an API server with mocking, transformations and validations.
https://stoplight.io/open-source/prism
Apache License 2.0
4.2k stars 343 forks source link

Prism as a HTTP Client #511

Closed philsturgeon closed 4 years ago

philsturgeon commented 5 years ago

User story. As a Prism user writing JS/TS, I want to make HTTP requests with Prism easily, using something that feels a lot like a common HTTP client, and it should be easy and seamless to talk to real servers or a mock from OpenAPI.

I should be able to create an instance of the client, and make multiple requests, without needing to create new instances.

Describe the solution you'd like

I'd like to deprecate or remove the http package, create a new http-client package, and work on a nice interface for using Prism as a HTTP client. If keeping the http package and making this a thin layer over the top is easier, then we can do that. If renaming, rehoming, and refactoring code in one big change is easier, that is also ok.

Axios is a great example of a HTTP client which is clear to understand:

const axios = require('axios');

axios.get('/user', {
    params: {
      ID: 12345
    }
  })
  .then(function (response) {
    console.log(response);
  })
  .catch(function (error) {
    console.log(error);
  });

// Want to use async/await? Add the `async` keyword to your outer function/method.
async function getUser() {
  try {
    const response = await axios.get('/user?ID=12345');
    console.log(response);
  } catch (error) {
    console.error(error);
  }
}

They have a bunch of aliases:

axios.request(config)
axios.get(url[, config])
axios.delete(url[, config])
axios.head(url[, config])
axios.options(url[, config])
axios.post(url[, data[, config]])
axios.put(url[, data[, config]])
axios.patch(url[, data[, config]])

Config can be created for a specific instance which means the requests later on can be made with less information:

const instance = axios.create({
  baseURL: 'https://some-domain.com/api/',
  timeout: 1000,
  headers: {'X-Custom-Header': 'foobar'}
});

they have a very useful response object:

{
  // `data` is the response that was provided by the server
  data: {},

  // `status` is the HTTP status code from the server response
  status: 200,

  // `statusText` is the HTTP status message from the server response
  statusText: 'OK',

  // `headers` the headers that the server responded with
  // All header names are lower cased
  headers: {},

  // `config` is the config that was provided to `axios` for the request
  config: {},

  // `request` is the request that generated this response
  // It is the last ClientRequest instance in node.js (in redirects)
  // and an XMLHttpRequest instance the browser
  request: {}
}

I think Prism could benefit from using axios as inspiration for a new and improved HTTP client package.

Current API

const Prism = require('@stoplight/prism-http');
const path = require('path');

// Create Prism instance and configure it as a mocker generating static examples
const prism = Prism.createInstance({
  mock: { dynamic: false },
  validate: { request: { query: true } },
});

// Load openapi spec file
const specPath = path.resolve(process.cwd(), 'basic.oas3.yaml');
prism
  .load({ path: specPath })
  .then(() => {
    // Make a "GET /todos" request
    return prism.process({
      method: 'get',
      url: {
        path: '/todos',
      },
      headers: {
        Accept: 'text/plain',
      },
    });
  })
  .catch(e => console.error(e))
  .then(prismResponse => {
    console.log(prismResponse.output);
  });

Potential API

const prism = require('@stoplight/prism-http');
const descriptionDoc = 'basic.oas3.yaml';

const httpClient = prism.createInstance({
  description: descriptionDoc,
  baseUrl: 'http://some.server.url.or.google.com/'
});

// make a request
httpClient.get('/todos', {
  mock: 'static', // or 'dynamic'
  validate: { request: true }, // default is true, checks headers and everything in the request
});

// make another request, same instance, this time its a post
const data = { foo: 'bar' }
const response = httpClient.post('/todos', data, {
  headers: {
    contentType: 'application/json',
  },
  mock: 'dynamic',
  validate: { request: true },
});

console.log(response.data);
console.log(response.status);
console.log(response.statusText);
console.log(response.headers);
console.log(response.config);

This contains a lot of changes, but it turns the Prism HTTP Client into something which is usable as a HTTP client, instead of just being kinda possible.

Acceptance Criteria

  1. mock mode should be configurable when creating instance and on request (request supersedes)
  2. description document must be configurable when creating an instance
  3. passing a HTTP body (a.k.a data) must be available for the same methods that allow the data argument in Axios
  4. enabling any sort of mock mode when there is no description provided should result in an exception
  5. get, post, put, patch, delete, and all the OpenAPI supported methods should be available
  6. the concept of an instance and a request are separated so one instance can make multiple requests
  7. camelCase headers are translated to hyphen-case
  8. requests can have just path (e.g: /foo) if the baseUrl was set, but if the baseUrl is not set, that will try sending a request to literally /foo which is not a valid URL so I would expect an error like Please provide a full URL or whatever Axios says if the URL is not valid
  9. Requests can provide (and override) the baseUrl by providing their own full URL: prism.post(http://example.com/foo)
  10. all mock modes will validate server urls
  11. ditch super granular controls over validating query, headers, etc, just offer controls for: request and maybe a special security option.
  12. New package should have unit tests, showing it works as expected

Additional context

Relates to #407.

chris-miaskowski commented 5 years ago

Hey @philsturgeon, thanks for putting this together. It's a great start!

Here are some thoughts and suggestions:

Let's break it down into smaller chunks. I don't want this to be a single big issue that one person will work on for a week. Instead here's how I would split it:

mock mode should be configurable when creating instance and on request (request supersedes)

That's done already, but I see you want the interface to look a bit different. It's an isolated issue.

description document must be configurable when creating an instance

This itself will require good amount of refactoring. Isolated issue.

passing a HTTP body (a.k.a data) must be available for the same methods that allow the data argument in Axios

"get, post, put, patch, delete, and all the OpenAPI supported methods should be available"

enabling any sort of mock mode when there is no description provided should result in an exception

camelCase headers are translated to hyphen-case

Related, should be a separate issue.

the concept of an instance and a request are separated so one instance can make multiple requests

Yup, more like a context than a task I guess (that's how it works now anyway - except the load we ditched). Can stay as AC in the epic.

requests can have just path (e.g: /foo) if the baseUrl was set, but if the baseUrl is not set, that will try sending a request to literally /foo which is not a valid URL so I would expect an error like Please provide a full URL or whatever Axios says if the URL is not valid

This will be a big issue. Currently, prism tries to "figure it out" if you don't provide a baseUrl. Consider the CLI use case. Prism will be instantiated without a baseUrl and serve as a mocker. You can't force it to use a specific baseUrl because it will falsely fail server validation.

Requests can provide (and override) the baseUrl by providing their own full URL: prism.post(http://example.com/foo) all mock modes will validate server urls (we already do it)

Separate issue. This isn't that simple. Feel free to implement it this way but the complexity of validating OAS3 servers hits us back here again. It's way easier to provide "baseUrl" as a param.

ditch super granular controls over validating query, headers, etc, just offer controls for: request and maybe a special security option.

Isolated issue


Going forward I want to stop creating big PRs that rewrite half of the code in a single run. We need to me more thoughtful about this.

Let's use this epic as an opportunity to put this good practice into life. Also we're gonna prioritize all this stuff independently. Some tasks are less important than others.

chris-miaskowski commented 5 years ago

@philsturgeon it would make sense IMO to start this conversation in design-docs (it's a technical design) but since we haven't let's leave here.

philsturgeon commented 5 years ago

It's great that loaders are swapped for an array of operations, but a) we are meant to have a helper so people don't have to figure that all out by theirselves, and b) I got this example from the docs so our docs are out of date.

As for the rest of it, I feel like you're just putting "isolated issue" next to every single argument, option and parameter. Yeah, we could work that way, but the plethora of chained PRs would be an awful mess to try and manage and it would be a sea of never-ending conflicts. Dunno if we want to bother messing around with all that.

We can have a Prism implementor chat about it and write down the outcomes here, but I don't know why we want to open this up to the whole company for discussions when its just an improvement to how prism works as a HTTP client. 🤷‍♂

chris-miaskowski commented 5 years ago

We can have a Prism implementor chat about it and write down the outcomes here,

@philsturgeon it's much simpler than that actually. We will present this issue in this form on the planning meeting, point it and if it's more than 8 SP we're gonna split it up.

It's actually neither up to me or you to decide whether it should be one issue or 10 ;) It's up the the implementing team. If Vincenzo/Matt say it's max 3 days of work or 8 SP I don't mind keeping one issue.

I'm suggesting it should be more than one because having experience with this code I have a rough sense of how big will this be. But anyone, let's leave that for the planning meeting.

philsturgeon commented 5 years ago

Yeah it can be made into however many issues the implementer would like to turn it into, I don't really mind, I'm just not doing it prematurely. This is the work we need to get done, do it however.

philsturgeon commented 5 years ago

We can have a Prism implementor chat about it and write down the outcomes here, but I don't know why we want to open this up to the whole company for discussions when its just an improvement to how prism works as a HTTP client. 🤷‍♂

This bit was an alternative to a design doc, not the 2nd topic of num issues which popped up. I initially offered a pairing session but nobody seemed interested. Instead I took a swing at it to provide some inspiration for the implementer.

XVincentX commented 5 years ago

I finally found the time to go a little bit through all this and here's my takeaway.

mock mode should be configurable when creating instance and on request (request supersedes)

I am assuming that if mock is false, the intent is to proxy the request somewhere else, right?

mock: 'static' | 'dynamic' | false

If not — as in the the Http Client is only for mocking — it a bit of a different story because the whole interaction is sync, and not async. Not a big deal, just knowing this ahead of the time will be helpful.


httpClient.get('/todos', {
  mock: 'static', // or 'dynamic'
  validate: { request: true }, // default is true, checks headers and everything in the request
});

I am assuming that validate is something like this:

type validateOptions = {
  request: boolean,
  response: boolean,
}
  1. Does this mean that you cannot selectively enable/disable the query/header validation (this is pointed in n. 11 but I want to make sure they're referring to the same thing)? I'm ok with this, just asking.

  2. Maybe we can flat the options instead of another subtype?

httpClient.get('/todos', {
  mock: 'static',
  validateRequest: true,
  validateResponse: true,
});

const httpClient = prism.createInstance({
  description: descriptionDoc,
  baseUrl: 'http://some.server.url.or.google.com/'
});

Prism does not really handle raw documents anymore, but we have an internal helper function we can reuse in case we want to go with such API (I do understand that's going to help significantly the people to use the client)


passing a HTTP body (a.k.a data) must be available for the same methods that allow the data argument in Axios

Note to self: I can actually enforce this with TypeScript.


enabling any sort of mock mode when there is no description provided should result in an exception

This is so obvious that maybe there's something I'm missing; you should be able to construct an instance only if you have a document, so is this even a possible scenario?


camelCase headers are translated to hyphen-case

ContentLenght -> Content-Lenght — is this what you want? Shall we convert them somehow? Lowercase? ContentLenght -> content-lenght?


Q1: Who's supposed to work on this? Me? Matt? Just making sure. If so, I can start draft some ideas tomorrow and see how this would work and go from there.

philsturgeon commented 5 years ago

@XVincentX Prism is meant to be an actual HTTP client so no its not just for mocking. I'm trying to emphasize this as much as possible throughout but I appear to not be making sense?

As a Prism user writing JS/TS, I want to make HTTP requests with Prism easily, using something that feels a lot like a common HTTP client, and it should be easy and seamless to talk to real servers or a mock from OpenAPI.

philsturgeon commented 5 years ago

Does this mean that you cannot selectively enable/disable the query/header validation (this is pointed in n. 11 but I want to make sure they're referring to the same thing)? I'm ok with this, just asking.

Correct, that was too granular to be useful.

Maybe we can flat the options instead of another subtype?

Sure, whatever you think is best.

Prism does not really handle raw documents anymore, but we have an internal helper function we can reuse in case we want to go with such API (I do understand that's going to help significantly the people to use the client)

Right so this HTTP client can use that helper. Prism itself will be handed the operations but the HTTP Client wont force the users to do that.

This is so obvious that maybe there's something I'm missing; you should be able to construct an instance only if you have a document, so is this even a possible scenario?

Descriptions are optional. You can make a request to google.com/some.json and get that data back. If you ask for a mock to happen but have not provided a description, then we cannot mock it for you.

ContentLenght -> Content-Lenght — is this what you want? Shall we convert them somehow? Lowercase? ContentLenght -> content-lenght?

Yeah so Because JavaScript™️ folks cannot send barewords for hyphenated things. { contentLength:application/json` } should transform. Lowercase hypenated is fine.

philsturgeon commented 5 years ago

Oh and this one is assigned to you, so crack on with any drafts or notes or however you want to go about it. This can be split into multiple issues however you think makes sense, maybe a "now" and "later" story, or make this an epic. Not too bothered exactly how it's done.

XVincentX commented 4 years ago

Plan of attack

This will be updated as I go through the code

Changes

Challenges