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

Improve DTO types, validation and serialization #1003

Open
tsa96 opened this issue Jul 17, 2024 · 1 comment
Open

Improve DTO types, validation and serialization #1003

tsa96 opened this issue Jul 17, 2024 · 1 comment
Labels
Blocked: Needs more info This is lacking essential information like steps to recreate or a video showing reported behavior. Type: Dev/Internal Something that is more internal to development than end user facing.

Comments

@tsa96
Copy link
Member

tsa96 commented Jul 17, 2024

WIP: I'm running out of steam, leaving this half-finished!

We've been discussing this for over a year and wanted to start getting some concrete plans written out. Haven't finalized anything yet, using this issue to collect my thoughts.

Definitions

To set the scene, overall we're concerned with data. Not gonna get bogged down in domain objects/models/entities/POJOs/whatever terminology, I'm just calling things objects, then let's distinguish:

  • Database objects: Data in database tables, for now, Postgres. In the future we'll have Redis as well but I don't want to worry about that here. Defined in our Prisma Schema, which is applied to Postgres via migrations (SQL files).
  • API objects: POJOs used in the backend service logic at runtime. We fetch these using Prisma, raw SQL, plus probably Kysely in future. We use Prisma/SQL/Kysely for JOINs, SELECTS etc... Their TS types are generated by Prisma.
  • Data Transfer Objects (DTOs): The JSON data incoming and ongoing to and from the API, via HTTP or WS.
    • For Incoming DTOs we have to transform to POJOs and validate
    • For Outgoing DTOs we have to serialize it

Both serialization and validation is a considerable amount of work, running on the main thread. A significant reason why Fastify is faster than Express is that it uses fast-json-stringify

Goals

Provide simpler types for our DTOs

This is really the main reason I want to do all this. The frontend makes use of these constantly, and I want to be able to use them in Panorama as well.

Currently our real source of truth for our types is the Prisma schema, but the types it generates are horrific. I'm not even going to try paste what they look like here, they're completely unreadable and composed of multiple types dotted around the 60k line TypeScript file that Prisma generates. There is no way we use these with Pano. They're far too unwieldy, and require installing the Prisma dependency and running the generate script to even use. Not making game devs have to do all that just to use what should be some relatively simple types.

Also,

  • Our DTOs tend to differ from what we store in DB. This might change more in the future if we use generated columns (more on this below), but I expect we'll always have some differences between the DB Objects/API Objects and DTOs.
  • You can't specify the type of json columns, which we use a lot. Support for type annotations has been requested for years, but Prisma updates are getting slower and slower so unlikely it ever happens (VC money probably drying up + their cloud stuff looks meh!).

We could use a generator for generating simpler types, I actually messed around with this about a year ago. But again we want types for our DTOs, which will differ from Prisma types anyway, and I don't want to add some insane system using comments in the schema for expressing the differences.

The current solution is to hand-maintain .model.ts files for every DTO, but make them derive from the Prisma type. Here's an example (user.model.ts):

export interface User extends Omit<PrismaUser, 'avatar'> {
  roles: Bitfield<Role>;
  bans: Bitfield<Ban>;
  avatarURL: string;
  profile?: Profile;
  userStats?: UserStats;
}
  • avatar we don't want, since the DTO transforms it to an actual URL pointing to Steam's CDN; we just store the actual ID.
  • roles and bans are really just numbers, but they're using a branded type to make Bitfields mutually incompatible, and to highlight they're a bitfield.
  • profile isn't a Prisma type, it's one of the same DTO/.model.ts types and User. We need to do this for all relational types.

Given that we're doing these .model.ts files already, the simplest solution is to just manually enter in every field by hand. I have no issue with doing that, we're never going to have thousands of tables, it's not a big deal. However I think it's very much worth ensuring that our DTO types (.model.ts types) are still assignable to Prisma types; when we modify our schema we want any changes to cause type errors.

However we need to do this in such a way that Panorama can use the types without having any Prisma dependency. I want the process of pulling types from this repo to the Panorama repo to be as simple as possible, and therefore I actually think we should just use a node script that yoinks the source files and scrubs any Prisma stuff, nothing else.

  • I don't want to use submodules: we modify constants all the time and don't want the hassle of web devs having to PR into two different repos whenever they change something.
  • Publishing constants as an NPM package would be hard to do and bad for developers, you'd have to update the package before you could access the changed types.

So I think a bash script that clones this repo, and copies just stuff from libs/constants/src into a subdir of the Pano repo would be easiest, just Git CLI and some coreutil stuff to remove lines matching the Prisma checks. Happy to hear other alternatives but IMO this is the best approach. Would look something like

cd ./panorama/scripts/ 
git clone https://github.com/momentum-mod/website tmp # should allow passing in a branch, or maybe even a code path that uses a local dir, for devs working on both repos at once
cd tmp/libs/constants/src/types/models/
for i in *.model.ts; do
  cat $i | grep -v -E ^assertType > $i
done
cd ../../../../
mv tmp/libs/constants/src constants
rm -rf tmp

Not have multiple sources of truth for our types

types separate from Prisma is worth it IMO but annoying to have that plus DTOs. but not all that bad idk. This makes Typia/Nestia very appealing, we'd just have the prisma schema then the DTO/.model.ts files.

Swagger docs generation

ct/cv are fucking slow and ugly

this probably isn't a huge deal, we're using prisma of all things, which is a huge perf hit according to our traces.

Current Setup

I don't like CT/CV but @Expose and @Transform are very helpful!

Ideas

I realised today we could use Postgres generated columns + SELECT (Prisma has omit now which is nice) in place of @Expose and @Exclude. Requires manually modifying migration files but I'm not against this.
See https://github.com/nikolasburk/generated-columns

Various approaches we could use on top of proposed type changes:

  • Typia + Nestia for both validation and serialization
  • Zod for validation, skip ClassSerializationInceptor to let fastify do it's serialization, without schemas for outgoing
    • Or try to just use native fastify (ajv-based) validation?
@tsa96 tsa96 added Type: Dev/Internal Something that is more internal to development than end user facing. Blocked: Needs more info This is lacking essential information like steps to recreate or a video showing reported behavior. labels Jul 17, 2024
@Gocnak
Copy link
Member

Gocnak commented Jul 19, 2024

The bash script seems fine to update it, we can even trigger a workflow in the Pano repo to do it via a runner and auto generate + commit it to main / open a PR for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked: Needs more info This is lacking essential information like steps to recreate or a video showing reported behavior. Type: Dev/Internal Something that is more internal to development than end user facing.
Projects
None yet
Development

No branches or pull requests

2 participants