curveball / core

The Curveball framework is a TypeScript framework for node.js with support for modern HTTP features.
https://curveballjs.org/
MIT License
525 stars 7 forks source link

initial impressions/feature requests/developer ergonomics feedback #131

Closed rettgerst closed 2 years ago

rettgerst commented 4 years ago

hello, I saw the introductory article in my google discover feed and was initially very impressed with curveball. and since you mention in your readme that you're looking for feedback I thought I'd write about one of the features I'd like to see.


NestJS is the server framework I currently use in my projects, coming from express. one of the major pulls to NestJS for me is its swagger plugin which allows me to automatically generate an OpenAPI specification file from the server source code. in turn, I use the OpenAPI Tools Generator to turn this spec file into API documentation and a strongly typed client for our frontend.

the major downside to Nest's approach is that it is heavily verbose and requires liberal use of decorators in order to provide all of the documentation metadata. here is an example of an actual api method from one of my projects where I use NestJS:

import { ApiTags, ApiResponse, ApiOperation, ApiParam } from '@nestjs/swagger';
import { Get, Param, Controller, Res, HttpCode } from '@nestjs/common';

export class DownloadController {

    // other parts of the controller removed for brevity

    @ApiOperation({
        summary: 'Get artwork thumbnail.',
        operationId: 'downloadThumbnail'
    })
    @Get('/:jobId/thumbnail.png')
    @ApiParam({
        name: 'jobId',
        description: 'job id returned from /api/upload/complete-upload'
    })
    @HttpCode(302)
    @ApiResponse({
        status: 302,
        description:
            'Redirects to a presigned S3 url to the a png thumbnail of the original artwork file'
    })
    async downloadThumbnail(
        @Param('jobId') jobId: string,
        @Res() res: express.Response
    ) {
        const url = await s3.getSignedUrlPromise('getObject', {
            Bucket: this.finalizedBucket,
            Key: `${jobId}/thumbnail.png`
        });
        res.redirect(url);
    }
}

(this doesn't even get into the chore of writing DTOs)

and here is a mockup of what this same method might look like in a hypothetical future version of curveball:

import { Context } from '@curveball/core';
import { Controller } from '@curveball/controller';
import { Redirect } from '@curveball/http';

interface GetThumbnailRequest {
    /** Job id returned from /api/upload/complete-upload. */
    jobId: string;
}

class ThumbnailController extends Controller {
    async get(ctx: Context<GetThumbnailRequest>) {
        const { jobId } = ctx.request.body;

        const url = await s3.getSignedUrlPromise('getObject', {
            Bucket: this.finalizedBucket,
            Key: `${jobId}/thumbnail.png`
        });

        /** Redirects to a presigned S3 url to the a png thumbnail of the original artwork file. */
        return new Redirect({
            code: 302,
            target: url
        });
    }
}

not only is this mockup fewer lines of code, but I also find it more readable - it reuses existing language constructs instead of duplicating them, and the textual descriptions are written as comments rather than strings.

this api also retains all of the same data as the first example - the operationId is implied (thumbnail resource + get operation = 'getThumbnail'), the paremeter descriptions already exist in the method signature plus the jsdoc comments, and the response code as well as the response body schema can be inferred from the method's inferred response type.

tools like typedoc can produce surprisingly rich, human-readable documentation just from statically analyzing code, with very little additions to the original source - all the metadata you need is already there in the type system. I've received comments from my coworkers on how good my documentation is - I don't write documentation, I just write code, then run it through typedoc. it would be great to have a simlar experience with my http apis.

evert commented 4 years ago

Thanks for the nice feedback!

A few things that might be notable:

This:

    async get(ctx: Context<GetThumbnailRequest>) {

Already works. Context is a Generic, and the first type is type of the request, the second (if specified) is the response.

The syntax for this:

        /** Redirects to a presigned S3 url to the a png thumbnail of the original artwork file. */
        return new Redirect({
            code: 302,
            target: url
        });

Is currently:

ctx.redirect(302, url);

I don't think this project will ever evolve to a point where it can automatically generate OpenAPI definitions, there's a couple of reasons:

  1. I believe that your OpenAPI definition, if anything, should generate the API. I don't think the API should generate the OpenAPI definitions. I believe API design should inform the implementation. So the kind of OpenAPI facilities we'll have in the future will likely be the exact reverse of that. I very bunch believe in a docs-first approach.
  2. For the people that like this sort of thing, I hope that one day a project will spawn like Nest.JS, that uses Curveball, much like Nest currently uses express under the hood.

One thing that we'll probably implement soonish (as a first step towards OpenAPI support) is deep JSON-schema integration.

Ideally I want the syntax for this to look something like:

class ThumbnailController extends Controller {
        @validate('http://my-api/schemas/my-thumbnail.json');
    async get(ctx: Context<GetThumbnailRequest>) {
        /// ...
    }
}

We're using this syntax for the projects we're working on, which is very similar:

class ThumbnailController extends Controller {
    async get(ctx: Context<GetThumbnailRequest>) {
            const body = validate<GetThumbnailRequest>(
                'ctx.request.body,
                'http://my-api/schemas/my-thumbnail.json'
             );
    }
}

Validate in this case was returning the body, but this was before Typescript assertion functions.

I'd imagine something very similar could work for OpenAPI, especially since it's now compatible with json-schema. I think similar OpenAPI will probably have to be a community contribution as we're not using this yet, and I'm not comfortable adding features that are not in use and go through some level of real-world scenarios and maintenance cycles. The devil is in the edge-cases =)

Cmacu commented 4 years ago

+1000 for the feedback! +1 for the OpenAPI definition. In a startup environment, the documentation often comes after the code and constant refactoring is part of every project. However I agree that generating documentation from source is just a nice feature and there are always other ways. It must be nice to always have documentation and definitions before starting the implementation.

Lindsor commented 4 years ago

The swagger definition is true in small and enterprise environments. Its usually driven by business timelines not something in the control of developers (especially in large enterprises).

I believe we should support both ways, Definition => API and API => Definition It shouldn't be bundled into core since both are optional features, that way core stays clean.

rettgerst commented 4 years ago

In a startup environment, the documentation often comes after the code and constant refactoring is part of every project.

Its usually driven by business timelines not something in the control of developers (especially in large enterprises).

this is my experience as well and why I take the code-first approach.

I am one of two js developers at my workplace and we are juggling 4 (soon to be 5) projects between us, where each project's client expects regular feature updates on a weekly basis, so velocity is a higher priority for us than elegance.

I agree that neither approach should be baked into core. however I do think adjusting the API to leverage type inference could enable the use of tooling to support either approach.

typescript (afaik?) cannot statically infer a method's behavior from assigning properties to or calling methods on the response object, but it can infer the response type of a method. so returning a class instance from controller would enable tooling to do things like generating an a controller interface, forcing a developer to write an implementation that matches their spec.

ericmorand commented 2 years ago

@Lindsor ,

The swagger definition is true in small and enterprise environments. Its usually driven by business timelines not something in the control of developers (especially in large enterprises).

This is the exact opposite that happens actually: in small businesses, using code as contract is acceptable; in big entreprises, no serious technical leader would ever accept such an approach since each micro-service may depend on the contracts of all the others and changing just one of them outside of supervision would break the whole system.

Cmacu commented 2 years ago

@ericmorand

This is the exact opposite that happens actually: in small businesses, using code as contract is acceptable; in big entreprises, no serious technical leader would ever accept such an approach since each micro-service may depend on the contracts of all the others and changing just one of them outside of supervision would break the whole system.

This sounds more like an encapsulation problem than a question of documentation. The idea behind a micro-service architecture is to split the execution into completely independent and stateless input><output based functionalities that have no connection or representation of the external environment they are in. If any of these functionalities can break all the rest then this sounds like a monolith to me.

ericmorand commented 2 years ago

Your sound like if micro-services don't communicate between each other. Which may be true but I'd be curious of an example of system where the output of a micro-service is not the input of another one.

Cmacu commented 2 years ago

@ericmorand

Your sound like if micro-services don't communicate between each other. Which may be true but I'd be curious of an example of system where the output of a micro-service is not the input of another one.

It looks like our experience varies. Here are some examples: logging, indexing, text processing, validation, synchronization, authorization, encryption/decryption, file storage, generic lookups, anything directly distributed to the clients (chat, notifications), storing data in the database (although there could be an output (id, count) in some cases), image processing, various automations. anything related to pub/sub and other non http protocols.

Looking at the projects I am working on I am having a hard time finding a micro service that depends on specific output from another and it would break, if the output doesn't match specific expectation. Either way breaking changes like the ones you are describing should be handled via micro-service versioning and deprecation for backwards compatibility.

In the context of documentation each micro service has it's own documentation, which doesn't relate to any other micro services. Even in the case of the architecture documentation, the goal is to generally describe these relationships without the specifics we are discussing here that could result in breaking the whole system.

ericmorand commented 2 years ago

Either way breaking changes like the ones you are describing should be handled via micro-service versioning and deprecation for backwards compatibility.

Yes, of course. But how do you handle that if the contract is generated by the code itself. How do you detect change in contract so that you can version them if there is no contract to begin with?

I the code of a micro-service that listen to an HTTP POST request changes the expected body schema, the generated OpenAPI specification becomes the new contract and all the services that depends on that endpoint will cease to function and would have to be rewritten.

How do you handle this issue if the contract is generated by the code?

Cmacu commented 2 years ago

Hmm, we are going a bit off topic here, but let's have a specific example.

Let's say there is a POST /v1/user micro-service and the code generated swagger documentation includes body fields firstName (string, required) and lastName (string, required).

So you want to change the code to include a new body field: age (number, optional). The field is added to the code and the documentation. Since it's optional any other micro-service can adapt it at their convenience (no harm no foul).

But what if for example the new body field is: email (string, required). I would say that this is a breaking change. In this scenario, introducing a new version/micro-service POST /v2/user that has new swagger documentation and includes the new specs would be the way to go.

This allows other micro-services to keep using /v1/user and migrate to /v2/user. Once everything is migrated v1 is deprecated and shortly after it's discontinued.

This might also require some additional scripts/migrations/micro-services to handle any previously generated user records during the process.

What would be the difference, if this was a documentation first approach?

ericmorand commented 2 years ago

Why would the micro service expose a V2 since as soon as the code changes, it becomes the new contract. Let's take a look at the other side: the API is public. A software depending on this API starts to fail. The engineers discover that the API call returns some 500 error. They then take a look at the contract and see that there is a mandatory field that they don't send and then that their software is at fault because it does not respect the contract. All they can do is wonder how it worked before since they don't have any other contract as reference than the one they are looking at right now.

Another side effect of the code as contract pattern: let's assume that two projects are started at the same time. The second one depends on the API exposed by the first one. How can the second one be coded since it doesn't know the specification of the API before the first one is done and deployed? Now, apply this pattern to a third service depending on the second one, and you see that you have to finish entirely the first one to start working on the second one, then have to finish entirely the second one to start working on the third one and so on.

Cmacu commented 2 years ago

So what's your solution on the first question? Sounds like there is the same problem, micro-services depending on the changing one are failing. How is that acceptable? We can't just force other people to change their code to adapt to our requirements. The goal is to provide solutions, not to create problems for others. There is no respected API I am aware of, that would introduce a breaking change without a new major version. The only exception are beta versions, but that's a different story that would rarely (if ever) be based on contract first approach.

And as for the side effects, the second service will have to stub the first service either way and since it can't really test against not deployed service, it will need to wait for the deployment to complete the implementation. If for some reason the 2 micro-services depend on each other, than they are not really separate micro-services

ericmorand commented 2 years ago

And as for the side effects, the second service will have to stub the first service either way and since it can't really test against not deployed service, it will need to wait for the deployment to complete the implementation. If for some reason the 2 micro-services depend on each other, than they

How can it stub the first service since the contract of the first service is not available before the first service is done and deployed?

Cmacu commented 2 years ago

@ericmorand Same way you would stub something that doesn't exist. You will hardcode/randomize the output with your best guess and keep working on your part until you have a deployed service to work with. What would you do, if you have documentation/contract for it? In ideal world you will have all the information upfront, but in the real one you will have to talk to people, setup expectations and negotiate protocols. No documentation can replace that. And in the end once the other service is deployed, you should have everything you need and the developers of the other service would not hate you for pestering them about documenting and updating specs while dealing with the actual problem they are trying to solve. I don't know about you, but in my team we deal with services in development all the time. I have never encountered a case where the documentation describing the service exists before it's deployed. Yet I've had 0 problems so far. If your expectations is that you can replace communication with documentation, then I can only hope that it works out.

ericmorand commented 2 years ago

You will hardcode/randomize the output with your best guess

You don't get me. I'm not talking about the output.

Let's do the exercice here and now.

I'm the developer of Service A and you are developer of Service B. None of them have been coded yet. Your service will consume HTTP REST endpoints exposed by my service.

Now, please, start coding. How do you proceed?

Cmacu commented 2 years ago

serviceA.api.ts

export const callServiceAapi = (...input: Array<string>): Promise<void> => Promise.resolve()

serviceB.main.ts

// TODO: Parse seviceB input
// TODO: Magic cards trick
// TODO: Call serviceA (log the request)
// TODO: Respond with the cards from the input

I have some more work to do today, but I can keep working on the TODOs early next week. Do you think ServiceA will be ready by the end of next week? Let me know, if you have any specific requirements about the logs I will send you, it's just a bunch of strings. Either way, I will update the inputs when you are ready and John said that maybe I can deploy ServiceA without logs, so no rush.

Have a great weekend, Eric!

Cmacu commented 2 years ago

Hey Denis,

I started working on ServiceA. It's still early, but I am starting to think about the inputs. I just need the shuffled deck of 64 cards and 3 random indexes. So deck: string[] and cards: number[] should do. I don't have much, but I will keep you posted.

See you on the baseball game.

PS: Eric, keep asking me about some specs. I am contemplating between ignoring him or getting him lunch no Monday so we can figure it out. I know...

ericmorand commented 2 years ago

Do you think ServiceA will be ready by the end of next week?

No. What do you do now?

evert commented 2 years ago

Thanks for the discussion, it was interesting. Not sure if there's really something actionable here but if so feel free to let me know or open a new ticket!