Feat: Icon HTTP Server

[tomasciccola]

This would solve #292

[achou11]

have some questions about the end-usage of this plugin:

• my understanding is that the plugin should work across several projects because we may create a single server instance, similar to the blob plugin. is that not the case?

• in terms of design, my thinking is that the plugin should be able to be used like so (in our case this would probably happen in the MapeoManager implementation):

const fastify = fastify()

fastify.register(BlobServerPlugin, { prefix: '/blob/', ... })
fastify.register(IconServerPlugin, { prefix: '/icon/', ... })

[tomasciccola]

On the last commit I basically moved the iconServer to MapeoManager. This meant, roughly:

• Exposing dataTypes from MapeoProject (so I can get an Icon doc)
• Exposing coreManager from MapeoProject (so I can get the actual Icon buffer)
• Passing the mapeoManager instance to the server (this is kinda weird maybe? I'm passing this to the createIconServer...). This is so I can do manager.getProject(projectId) inside the response callback

@gmaclennan @achou11 opinions??

[achou11]

my thinking is that each of the Fastify plugins (so both blobs and icons) should accept an option that is either:

1. The manager instance (like you have it so far)
2. An async function called getProject().

The fundamental thing that each plugin eventually needs is to access project-specific resources, so we need to define a way for the plugin to access a project and its API. For this reason, I think 2 makes a little more sense than 1, but either should work.

In my opinion, the end goal is to be able to set up the server in MapeoManager like so:

this.#fastifyServer = new Fastify({...})

// Register the blob routes
this.#fastifyServer.register(BlobServerPlugin, {
  prefix: '/blob/',
  // One of the following...
  // 1. The manager option
  manager: this,
  
  // 2. The getProject option
  getProject: async (projectId) => {
   return this.#getProject(projectId)
  }
)

// Register the icon routes
this.#fastifyServer.register(IconServerPlugin, {
  prefix: '/icon/',
  // One of the following...
  // 1. The manager option
  manager: this,
  
  // 2. The getProject option
  getProject: async (projectId) => {
   return this.#getProject(projectId)
  }
)

At a glance, a shortcoming with this approach is that you may need to expose random instance properties on the project instance that aren't necessarily useful to a frontend client (remember, we're going to proxy this when interfacing with an IPC layer). I think a solution to that is using the symbol key accessor pattern that we're using to expose certain methods/fields but for internal usage + testing. For example, if you need the core manager instance, you can set it up like so:

export const kCoreManager = Symbol('coreManager')

class MapeoProject {
  get [kCoreManager]() {
    return this.#coreManager
  }
}

// Somewhere else where you have the project instance:
import {kCoreManager} from '...' // must explicitly import and use this symbol if you want to access that exposed getter
const core = await project[kCoreManager].getCoreByDiscoveryKey(...)

[tomasciccola]

I commented this regexes 'cause I was getting validation errors and I couldn't bother with regex today.
It seems that the project key is actually 26 bytes? (printing projectId that's returned from manager.createProject() yields 52...)
Anyways, there's probably something I'm missing...

[achou11]

that number seems familiar for some reason, but the project id that's used for the url should be 32 bytes since we're using this from @mapeo/crypto to create them:

https://github.com/digidem/mapeo-crypto/blob/a3ec70b97318260e2f1593d6abbaeda065cc9407/utils.js#L39

[achou11]

ah yeah the encoding is different, which produces an id of 26 bytes. The projectId here is really the project's "public" id, which is a one-way hashed version of the project's "internal" id (which is 32 bytes). Think it's a matter of updating this regex to use length of 52 characters

Needs to be done for the blob server plugin too (but that's out of scope here) as that was implemented before we introduced the public id approach

[tomasciccola]

Mhm, following the trail, the keys.projectKey on mapeo-manager:217 seems to be 32-bytes long. Now, after passing it to projectKeyToPublicId (which wraps keyToPublicId) the resulting string length is 52 (shouldn't it be 64?).
I see in that fn that you linked that it uses some encoding called z-base-64. I'm not familiar with that, but does that changes something??
I see that I'm on mapeo-crypto-alpha.8 which is the last version, but at the same time on my local version of mapeo-crypto that function is named projectKeyToPublicId instead of keyToPublicId (I've rm -rf node_modules && npm install just in case...)

[achou11]

@tomasciccola yeah not sure if you saw my last comment but the project id we're returning from createProject is not a hex-encoded string but rather the z-base-64 one. this is expected, i just forgot that it's a different length and the only change needed here is updating the regex to use 52 instead of 64, since this API would only be accepting "public" ids

[achou11]

also updating the variable name to something like PROJECT_PUBLIC_ID_REGEX, maybe with the addition of a comment to summarize what's been explained here 😄

[tomasciccola]

yeah, an also include every letter since its not hex (I think only lowercase, but left also upper just to be safe)

[gmaclennan]

not essential for this PR, but just a thought on how we expose "getProject". The Fastify "way" is to have a plugin that "decorates" the fastify instance with fastify.getMapeoProject(), and making that plugin the dependency of this and the blob http server. It's fine I think how it is implemented here, but if we start moving things into separate files if the code grows then it gets more complicated passing the getProject function to the enclosure (vs. attaching to context as a plugin).

[gmaclennan]

Good work. Needs a couple of changes. Is the SQL changed here because of a change in schema in the stacked PR? I don't see any changes to schema in this PR.

[tomasciccola]

Is the SQL changed here because of a change in schema in the stacked PR? I don't see any changes to schema in this PR.

Yeah, no schema work here, must have slipped me...

[gmaclennan]

all looks good! good work. Hopefully my comment helps resolve the type issue, otherwise just @ts-ignore for now

[tomasciccola]

all looks good! good work. Hopefully my comment helps resolve the type issue, otherwise just @ts-ignore for now

yeah, thanks for that. I saw that fastify documentation, just was trying to avoid using .d.ts and using jsdoc directly...

[gmaclennan]

Great job. Lgtm.

[achou11]

Couple of small comments on things I noticed (mostly for myself). Keeping in mind that with #365, some other changes are probably going to be needed here, but otherwise generally in a good state