chore(api-server): improve tests

[jtoar]

This PR improves the tests for the @redwoodjs/api-server package. In doing this, I deliberately didn't edit any sources files (save one edit to export an object for testing).

My goal was basically to test everything I could. I'm going to be deduplicating code across the framework (a lot of the code in the api-server package was duplicated to avoid breaking anything while iterating on experimental features), and since users can invoke api-server code in a few different ways (npx @redwoodjs/api-server, yarn rw serve, and importing handlers from @redwoodjs/api-server), I don't want to accidentally break anything anywhere. Improving test coverage also revealed some things that need to be fixed.

A few high-level notes. Overall, I reduced our reliance on mocking (in many tests, we weren't using fastify at all). Most tests covered configureFastify and while a few checked to see that routes were registered correctly, none of them actually started the api-server, or used any of fastify's testing facilities like inject to check that the right file was sent back, etc. (Now they do!) Lastly, while most of the tests I added assume fastify, the dist.test.ts isn't and is more like a black-box test.

Re the "revealed some things that need to be fixed", in no particular order:

• apiUrl and apiHost can be a bit confusing. Even the previous withApiProxy.test.ts got them wrong

  While this test checked if the options were passed correctly, it mixed up the apiUrl for the apiHost. apiHost is the upstream (and should be a URL).

  redwood/packages/api-server/src/__tests__/withApiProxy.test.ts

  Lines 51 to 54 in daaa199

  	await withApiProxy(mockedFastifyInstance as unknown as FastifyInstance, {
  	apiUrl: 'http://localhost',
  	apiHost: 'my-api-host',
  	})

  @Josh-Walker-GM alerted me to this some time ago, but I didn't understand the nuance. Now that I do, these are just poorly named and we should try to fix them.

• Everything in web dist is served

  redwood/packages/api-server/src/plugins/withWebServer.ts

  Lines 50 to 53 in daaa199

  	// Serve other non-html assets
  	fastify.register(fastifyStatic, {
  	root: getPaths().web.dist,
  	})

  We may want to revisit this. I'm sure most of it needs to be served, but it seems like a poor default when it comes to security. Also, it just results in routes being served that don't work. E.g., all prerendered routes are available at their .html suffix, but they error out as soon as they're rendered.

• The lambda adapter seems a little too simple

  redwood/packages/api-server/src/requestHandlers/awsLambdaFastify.ts

  Lines 11 to 27 in daaa199

  	const lambdaEventForFastifyRequest = (
  	request: FastifyRequest
  	): APIGatewayProxyEvent => {
  	return {
  	httpMethod: request.method,
  	headers: request.headers,
  	path: request.urlData('path'),
  	queryStringParameters: qs.parse(request.url.split(/\?(.+)/)[1]),
  	requestContext: {
  	requestId: request.id,
  	identity: {
  	sourceIp: request.ip,
  	},
  	},
  	...parseBody(request.rawBody || ''), // adds `body` and `isBase64Encoded`
  	} as APIGatewayProxyEvent
  	}

  There's a core fastify plugin for lambdas, and it's much more complicated than our implementation. Should we just use that? What's the difference between our implementation and theirs?

• setLambdaFunctions should ignore functions that don't export a handler.

  It probably doesn't do any harm but it registers them as undefined:

  redwood/packages/api-server/src/plugins/lambdaLoader.ts

  Lines 32 to 40 in daaa199

  	LAMBDA_FUNCTIONS[routeName] = handler
  	if (!handler) {
  	console.warn(
  	routeName,
  	'at',
  	fnPath,
  	'does not have a function called handler defined.'
  	)
  	}

• The lambda adapter supports callback syntax

  redwood/packages/api-server/src/requestHandlers/awsLambdaFastify.ts

  Lines 71 to 89 in daaa199

  	const handlerCallback =
  	(reply: FastifyReply) =>
  	(error: Error, lambdaResult: APIGatewayProxyResult) => {
  	if (error) {
  	fastifyResponseForLambdaError(req, reply, error)
  	return
  	}
  	fastifyResponseForLambdaResult(reply, lambdaResult)
  	}
  	// Execute the lambda function.
  	// https://docs.aws.amazon.com/lambda/latest/dg/nodejs-prog-model-handler.html
  	const handlerPromise = handler(
  	event,
  	// @ts-expect-error - Add support for context: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/0bb210867d16170c4a08d9ce5d132817651a0f80/types/aws-lambda/index.d.ts#L443-L467
  	{},
  	handlerCallback(reply)
  	)

  I don't think we need to support this anymore.

• The CLI handlers don't have help and don't error out on unknown args

  It's not uncommon to use this CLI standalone so we should polish it.

  redwood/packages/api-server/src/index.ts

  Lines 26 to 28 in daaa199

  	apiServerHandler(
  	yargs(hideBin(process.argv)).options(apiCliOptions).parseSync()
  	)

  $ yarn rw-server --help
  Options:
    --help     Show help                                                 [boolean]
    --version  Show version number                                       [boolean]
  

  $ yarn rw-server --foo --bar --baz
  Starting API and Web Servers...
  

• You can't import @redwoodjs/api-server outside a Redwood project

  This one isn't relevant to users, but it made testing the built package more complicated than it needed to be. The issue is that getConfig is called as soon as the file is imported. And if you're not in a redwood project at that point, you're out of luck:

  redwood/packages/api-server/src/cliHandlers.ts

  Line 24 in daaa199

  port: { default: getConfig().web?.port || 8910, type: 'number', alias: 'p' },

[Josh-Walker-GM]

I left a couple of really very minor comments that shouldn't hold this up. I hope they don't appear overly nit-picky as that was certainly not my intension.

This work to add these tests to understand better the effects of incoming changes is really great!

Edit: The only other thing I was going to mention was the location of the dist.test.ts file. At first it struck me as a little odd that it was at the root level rather than in src/ somewhere but I don't really have a strong opinion on where it should live so wherever you put it is good with me.

[jtoar]

Forgot to address your comment about the dist.test.ts file. The tests in that file use the built code in dist, not src, so that's why I called them "dist tests". I.e. But the more practical reason that they're run differently and not as part of the "src tests" is that they take a lot longer to run.