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

Move interface from HTTP to JavaScript #111

Open
EvanHahn opened this issue Jan 30, 2024 · 7 comments
Open

Move interface from HTTP to JavaScript #111

EvanHahn opened this issue Jan 30, 2024 · 7 comments
Assignees

Comments

@EvanHahn
Copy link
Contributor

In short: I propose we move much of mapeo-map-server’s API from HTTP to JS. I made a draft PR to move one of the routes. It might be a bad idea!

Currently, mapeo-map-server exposes all its functionality over HTTP, except for starting the server: that's a regular JavaScript function call to createMapServer().

In other words, there are two interfaces: an HTTP API (where most stuff happens) and a JavaScript API (where you start the server).

This document proposes moving most of the HTTP API to the JavaScript API.

Instead of making an HTTP POST to /tilesets/import, you'd call something like importTileset(). Some routes, such as GET /tilesets/:tilesetId/:zoom/:x/:y, would be unaffected, as they are needed for the map renderer.

Here’s a draft PR that moves one of the routes: #110

This might be a bad idea!

What?

Conceptually, I see the current system like this:

CoMapeo function call → HTTP request → Route handler → Internal API

I see the new system like this:

CoMapeo function call → External API

Some routes, such as GET /tilesets/:tilesetId/:zoom/:x/:y, would be unaffected, as they are needed for the map renderer. In other words, there would still be an HTTP server.

Roughly, I imagine an interface like this:

import MapServer from '@mapeo/map-server'

// Create the server instance. Doesn't do much.
const server = new MapServer({
  database: new Database('./example.db'),
})

// Start the server.
await server.listen(3000)

// Start an import.
// You could fetch this import later with `server.getImport(importId)`.
const mbtileImport = server.importMbtiles('/path/to/mbtiles_file')
mbtileImport.on('progress', ({ soFar, total }) => {
  const ratio = soFar / total
  const percent = Math.round(ratio * 100)
  console.log(`Imported ${percent}%`)
})
await mbtileImport.completion()

// Get a list of all the styles.
const styles = server.getStyles()

// Stop the server.
await server.close()

Again, this is a rough sketch; please ignore the specifics.

Why?

I think this idea provides several advantages:

  • Less code. We could remove most of our route handlers. CoMapeo wouldn't need HTTP wrapper functions. Our server-side events code could be removed, as could many of our parameter validations.

  • Better performance. Though HTTP requests incur minimal overhead, in-process function calls should incur even less.

  • Easier type checking. Adding an HTTP boundary means that we either need to validate arguments and return values at runtime; not so with regular function calls.

  • Improved security. You could imagine a malicious web app making HTTP requests to the server and performing various undesirable operations. If the server were read-only, that would be less of a threat.

  • Simpler ergonomics for the consumer (and our tests). It's usually easier to call a function than to make an HTTP request. For example, you currently create an import and then request its status. What if this was a single function call?

  • Easier documentation. Many editors let you hover over a function and see its JSDoc information, but few do the same for HTTP endpoints.

I'm not aware of any significant downsides to this, but (1) I may be overconfident in my idea (2) I don't know the map server very well!

How?

Currently, we have a component called ApiPlugin, which handles most of the business logic. I suspect we could remove most routes and expose this plugin's functionality.

I think this can be done piecemeal. For example, we could migrate a single route in the first PR, like #110.

@achou11
Copy link
Member

achou11 commented Feb 7, 2024

Heads up for @rudokemper and @luandro in case you have any thoughts on what's being proposed here. Gregor and I had conversations related to doing this prior to Evan making a more structured proposal for it, as it would make the maintenance more straightforward and the usage less opinionated.

@achou11
Copy link
Member

achou11 commented Feb 7, 2024

Some routes, such as GET /tilesets/:tilesetId/:zoom/:x/:y, would be unaffected, as they are needed for the map renderer. In other words, there would still be an HTTP server.

Maybe I'm forgetting something, I don't think it would be necessary to keep this. I would lean towards exposing a method like getTile(...) that takes the relevant params as an argument, and then this can be used by an end-user that decides to set up that route.

Basically, I don't really see any reason for the library to include it's own server instance of any kind as part of the main implementation. This also means no inclusion of a listen() method or anything similar as a result. Thoughts?

@luandro
Copy link

luandro commented Feb 7, 2024

I think this makes a lot of sense. If there's not much overhead added, I like the idea of exposing at least the GET for tilesets, which could be consumed by other applications in the future.

@achou11
Copy link
Member

achou11 commented Feb 7, 2024

If there's not much overhead added, I like the idea of exposing at least the GET for tilesets, which could be consumed by other applications in the future.

The way I'm imagining this is that there would be a method that we expose e.g. getTilesets() and we'd leave it up to an application developer to wrap that with their own HTTP server implementation (whether it's Express, Fastify, raw Node HTTP, etc). think we'd have a Fastify plugin for sure out of those options, but the other options are viable and will be left for the application developer to implement

@rudokemper
Copy link
Member

To be honest, I'm not super up to date with the anatomy of mapeo-map-server and whether the current API routes could be leveraged in an environment that isn't CoMapeo, but one advantage of an HTTP API is that it is language-agnostic and so Python (or other) tooling could seamlessly work with the API without the need for any bridging. If the idea behind mapeo-map-server is to be modular, interoperable, and easily plugged into other toolchains (regardless of language), that might be a consideration. But even so there are workarounds and based on reading the thread, it definitely seems like a parsimonious solution for CoMapeo.

@EvanHahn
Copy link
Contributor Author

EvanHahn commented Feb 7, 2024

To be honest, I'm not super up to date with the anatomy of mapeo-map-server and whether the current API routes could be leveraged in an environment that isn't CoMapeo, but one advantage of an HTTP API is that it is language-agnostic and so Python (or other) tooling could seamlessly work with the API without the need for any bridging. If the idea behind mapeo-map-server is to be modular, interoperable, and easily plugged into other toolchains (regardless of language), that might be a consideration. But even so there are workarounds and based on reading the thread, it definitely seems like a parsimonious solution for CoMapeo.

Good point.

Currently, you need to use Node to start the server and set up the database, so it wouldn't currently work with Python (or other).

We could address this by making this repo a standalone executable with its own HTTP server and database, but that would make things a bit harder for CoMapeo.

Instead, I think the ideal design (in the short term) is to use regular JavaScript function calls in this module. In the future, one could easily wrap these functions with an HTTP server, or a Python bridge, or FIFOs, or whatever is needed by the consumer.

EvanHahn added a commit that referenced this issue Feb 12, 2024
This moves the public "get an import" interface from HTTP to JavaScript.
In other words, you won't make an HTTP GET to `/import/:importId` any
more. Instead, you'll call `server.getImport(importId)`.

This is the first step in [a larger refactor][0], where most of the
interface will be moved from HTTP to JS. Because this is the first step,
several changes have to be made:

- `createMapServer` returns a newly-created `MapServer` instance, rather
  than a `FastifyInstance`. This instance is the main public interface.
- `MapServer` takes on many of the responsibilities of `Api`, such as
  initializing the database and dealing with server shutdown.
- Tests need to work a bit differently. Those in the "new style" deal
  with the `MapServer` instance where those in the "old style" deal with
  the Fastify instance.
- Update documentation.

I suspect future moves, such as migrating `GET /styles`, will be much
smaller patches.

[0]: #111

BREAKING CHANGE: `GET /imports/:importId` replaced with `getImport()`
@EvanHahn EvanHahn self-assigned this Feb 12, 2024
@EvanHahn EvanHahn changed the title RFC: move interface from HTTP to JavaScript Move interface from HTTP to JavaScript Feb 12, 2024
@EvanHahn
Copy link
Contributor Author

Given the discussion here, I'm going to get started on this. Merged the first patch in f374a64.

I'm not going to completely remove the HTTP server as part of this work, but I'll plan to move the rest.

EvanHahn added a commit that referenced this issue Feb 27, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /imports/progress/:importId` replaced with
  `getImportProgress()`
EvanHahn added a commit that referenced this issue Feb 27, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /imports/progress/:importId` replaced with
  `getImportProgress()`
EvanHahn added a commit that referenced this issue Feb 27, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /imports/progress/:importId` replaced with
  `getImportProgress()`
EvanHahn added a commit that referenced this issue Feb 29, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /imports/progress/:importId` replaced with
  `getImportProgress()`
EvanHahn added a commit that referenced this issue Mar 3, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /tilesets` replaced with `listTilesets()`
EvanHahn added a commit that referenced this issue Mar 4, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /tilesets` replaced with `listTilesets()`
EvanHahn added a commit that referenced this issue Mar 4, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /styles` replaced with `listStyles()`
EvanHahn added a commit that referenced this issue Mar 4, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111

BREAKING CHANGE: `GET /styles` replaced with `listStyles()`
EvanHahn added a commit that referenced this issue Mar 5, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111
EvanHahn added a commit that referenced this issue Mar 5, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111
EvanHahn added a commit that referenced this issue Mar 6, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111
EvanHahn added a commit that referenced this issue Mar 6, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111
EvanHahn added a commit that referenced this issue Mar 6, 2024
This is the next step in [a larger refactor][0] where we replace the
HTTP API with a JS one.

[0]: #111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants