bsky: Add caller did in grpc calls to dataplane

[matthieusieben]

Adds a bsky-caller-did header to the GRPC calls to the dataplane that contains the did of the authenticated user making the inbound HTTP request.

Does so by creating a new HydrateCtx per HTTP request that contains a wrapped dataplane instance (that will add the header), and that is used from controllers (including pipelines).

[devinivy]

This is a much simpler change to getLists and unrelated to the rest of the work— could we peel it out of this PR so that we can ship it separately? e21ada7

[devinivy]

I have a slight concern around getting rid of all handler function bodies and replacing them with this method, which is that it becomes awkward to add a custom behaviors to a given route. Instead, all the custom behavior ends-up needing to get expressed within this method, which seems like it will end-up accumulating many special cases over time.

A good example is the canPerformTakedownRequired option— this clearly stands out as a behavior of some specific endpoint that performs a takedown, but it ends-up folded into this method next to other unrelated behaviors from all other routes. I find that admin.updateSubjectStatus was easier to reason about and to maintain in its previous state.

Overall it seems more natural to me to place the special behaviors directly where they're need, and instead create utilities if necessary to make the handlers more concise. By leaving function bodies around for each route's handler, they'd all remain open to extension. I am interested to hear your thoughts on all of this!

[matthieusieben]

I generally agree with your point of view. Special behaviors should be visible where they are needed. And ideally, those special code paths should not impact places where they are not needed.

• The case of canPerformTakedownRequired is quite isolated and was easily reverted back to the way it was.

• allowInclude3pBlocks and allowIncludeTakedowns are options needed to create the HydrateCtx object, so it is quite natural to put them here IMO. We can rename them if you think there might be a better name.

• inputHeaders and ouputHeaders were removed.

• Regarding exposeRepoRev, there are lots of alternative solution, some of which are detailed below. In my opinion, it comes down to two things:

  1. This is needed in about 50% of the methods. Having an explicit boolean makes it easier to distinguish omissions from explicit opt-in.
  2. Most options that make this available as a function make the code more verbose & significantly increase repetition.

alternatives to the exposeRepoRev option

we could easily re-write it as a withExposedRepoRev wrapper function. With this, the following:

  server.app.bsky.feed.getAuthorFeed({
    auth: ctx.authVerifier.optionalStandardOrRole,
    handler: ctx.createPipelineHandler(
      skeleton,
      hydration,
      noBlocksOrMutedReposts,
      presentation,
      {
        allowIncludeTakedowns: true,
        exposeRepoRev: true
      },
    ),
  })

would be replaced by:

  const getAuthorFeed = createPipeline(
    skeleton,
    hydration,
    noBlocksOrMutedReposts,
    presentation,
  )

  server.app.bsky.feed.getAuthorFeed({
    auth: ctx.authVerifier.optionalStandardOrRole,
    handler: ctx.createHandler(withExposedRepoRev(getAuthorFeed), {
      allowIncludeTakedowns: true,
    }),
  })

I personally think this is not really better in terms of readability. It increases repetition (ctx.createHandler + createPipeline instead of ctx.createPipelineHandler) & makes the code either more "dense" (like above) or more repetitive (if introducing intermediary variables).

A more readable alternative, that would be more aligned with the current implementation, could look like this:

  const getAuthorFeed = createPipeline(
    skeleton,
    hydration,
    noBlocksOrMutedReposts,
    presentation,
  )

  server.app.bsky.feed.getAuthorFeed({
    auth: ctx.authVerifier.optionalStandardOrRole,
    handler: async (reqCtx) => {
      const hCtx = await ctx.createHydrateCtx(reqCtx, {
        allowIncludeTakedowns: true,
      })

      hCtx.exposeLabelers()
      await hCtx.exposeRepoRev()

      return getAuthorFeed(hCtx)
    },
  })

I find that this notation has several drawbacks:

• exposeLabelers() must basically always be called. This yields a lot of repetition that adds un-necessary noise to the function.
• exposeRepoRev() is async and exposeLabelers() not. could be error prone (forget to await in some instances)
• As maintainer, it is not clear when a function does not contain exposeRepoRev() whether it is intentional or an omission ({ exposeRepoRev: boolean } makes this explicit)

[matthieusieben]

Note: I refactored that function to be much more lightweight and easier to use in a raw handler function:

atproto/packages/bsky/src/context.ts

Lines 206 to 218 in 7c8774c

	return async (
	reqCtx: HandlerRequestContext<Params, Auth, Input>,
	): Promise<Output> => {
	const ctx = await this.createHydrateCtx(reqCtx, options)
	// Always expose the labelers that were actually used to process the request
	ctx.setLabelersHeader()
	// Conditionally expose the repo revision
	if (exposeRepoRev) await ctx.setRepoRevHeader()
	return view(ctx)
	}

I believe that this helper function still has value, and should be used instead of duplicating its logic because:

1. It avoids having to set the Atproto-Content-Labelers header in every controller
2. It makes it explicit whether Atproto-Repo-Rev by having to opt-into it through the options.