prisma-engines icon indicating copy to clipboard operation
prisma-engines copied to clipboard

Add native types to the dmmf json output

Open halfmatthalfcat opened this issue 3 years ago • 8 comments

This exposes native db types to the dmmf json output for use in custom generators.

Helps fix:

https://github.com/prisma/prisma/issues/6844 https://github.com/prisma/prisma/issues/10252

halfmatthalfcat avatar Feb 05 '22 18:02 halfmatthalfcat

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 05 '22 18:02 CLAassistant

@pimeys @tomhoule @Jolg42 👀

halfmatthalfcat avatar Feb 08 '22 14:02 halfmatthalfcat

Hey @halfmatthalfcat thanks for the PR! We'll be looking into it soon.

For now, we have a couple of questions that I'd like to get your input on:

  • Why are number arguments strings?
  • Do all native types fit this format?
  • Should the database scope be attached too? (e.g. pg1)

We'll also discuss these questions on our side when we have a closer look at your PR.

matthewmueller avatar Feb 08 '22 17:02 matthewmueller

Ah, good questions:

  1. Arguments are treated as strings since the model of NativeTypeInstance uses Vec<String> for the arguments. I'm assuming the true type (e.g. a number for things like Decimal) will be parsed by the implementing generator(s) (or whomever is parsing the dmmf). I'd rather not add any additional parsing logic at this level unless there is another place in the codebase that does provide correct argument types for the given native type. Granted, I haven't looked that hard.
  2. I believe they do since we're essentially using the NativeTypeInstance in whole (name and args). Unless the core Native Type model changes, I think we'd have parity amongst all native types parsed by Prisma.
  3. I'd be for adding the scope as another field in NativeType if you all feel it's necessary. I'm personally not sure of how pervasive mixing different scopes in your model is. I guess in the case of multiple scopes (and in turn different dbs), having it around would be useful.

halfmatthalfcat avatar Feb 08 '22 18:02 halfmatthalfcat

https://github.com/prisma/prisma-engines/pull/2669 was just merged and it will most likely cause a little bit of merge conflicts. 2669 is nearly exclusively moving files around and rearranging imports, so it should be straightforward to fix, but please reach out if you need help with that after there is a decision on how to move forward on this PR.

tomhoule avatar Feb 14 '22 12:02 tomhoule

Rebased and ready for reconsideration whenever you all have discussed :)

halfmatthalfcat avatar Feb 16 '22 14:02 halfmatthalfcat

Any news about this PR? I need it to auto generate the class-validators from the native types and names.

zzau13 avatar May 20 '22 10:05 zzau13

No updates on this right now.

We are aware it exists, also know it might be useful for some usages (like the one you mentioned), but have no current use case in Prisma itself yet pushing us to look into this right now and are cautious because of the cost it would incurr: Making the DMMF bigger. As that is already sometimes a problem, we are currently investigating ways to avoid that becoming a problem which could also make this PR more merge-able. But no ETA.

janpio avatar May 23 '22 10:05 janpio

@janpio So here some facts on why I think it's stupid to not merge this PR:

  • I have a schema file of 1045 line. 10 enums, 29 models, and 49 types
  • I dump the GeneratorOptions Object into a JSON file.
    • The file does 252157 lines and 8.2 mo
    • Adding NativeType add 4-5 lines by field if use.
    • In my DB, I have 118 times @db.which represent my usage of NativeType
    • Doing the math : 5*118= 590 lines added.

Here the issue:

The GeneratorOptions object is huge! But the change we ask for is very small compare to other feature of the DMMF Definition.

Take this screenshot : image The added part of this PR would be found in the ~8k lines aka datamodel. Let's said adding this will add up to 9k lines, it would be huge if the "schema" object wasn't THAT BIG... It's a whopping ~250k lines of data...

It's 8.2 Mo of data! That's why you have issues on performance and stability...

image

Let's said that I'm deleting the "schema" object, it's 8 mo less just by deleting this object.

image

Looking at what it does (DMMF.Schema), I'm not sure you would want to remove that. BUT, you could generate the content using the datamodel. From what I understand, it just "generated" data that you could deduce from the data in DMMF.Datamodel. While it's useful for generating Types for Typescript, it is still generated data from the DMMF.Datamodel object. There is probably more to it than what it seem, but the issues is not in the DMMF.Datamodel, it's in the DMMF.Schema.

Solution

  • Merge this PR because the impact should be minimal.
  • Refactor the code for generating DMMF.Schema and take it out of the DMMF.Document definition.
    • Create function like getSchema(dmmf.datamodel) and that's it!

Sorry, I'm harsh and rude in this comment, but I don't think it's justify to keep this PR waiting for the reasons mention earlier. The prisma Team might not be using that part, but having the option to create Generator and generated basic code that could be updated automatically every time you edit your schema, it's pretty awesome and adding nativeType is the way forward to be able to fully use the generator feature in Prisma.

Personally, I would not make this PR wait any longer because it has low impact and would greatly benefits the "Prisma Ecosystem".

maxime4000 avatar Mar 02 '23 20:03 maxime4000

No updates on this right now.

We are aware it exists, also know it might be useful for some usages (like the one you mentioned), but have no current use case in Prisma itself yet pushing us to look into this right now and are cautious because of the cost it would incurr: Making the DMMF bigger. As that is already sometimes a problem, we are currently investigating ways to avoid that becoming a problem which could also make this PR more merge-able. But no ETA.

@janpio I'm starting up a Scala client in earnest at the moment and want to know more about how you all are coming to the conclusion that adding this change will drastically increase DMMF size. Have you all run tests to this effect and if so, what were the results of the test(s)? How much bigger was the size increase?

Could there potentially be a way to hide these DMMF enhancements behind runtime feature flag(s) for library authors in order to get these (somewhat table stakes) features?

halfmatthalfcat avatar Mar 28 '23 14:03 halfmatthalfcat

We don't think it will increase it drastically, but it will increase it. We already have people hitting limits where Prisma just does not work for their schema size any more, and increasing the size would add to that. Hence our caution.

Could there potentially be a way to hide these DMMF enhancements behind runtime feature flag(s) for library authors in order to get these (somewhat table stakes) features?

Yes, something related has been discussed as introducing a variant of DMMF that custom generators/clients could request (can recall the location), but not with the suggestion to use a preview feature to influence this instead. That is an interesting additional option.

janpio avatar Mar 28 '23 15:03 janpio

We don't think it will increase it drastically, but it will increase it. We already have people hitting limits where Prisma just does not work for their schema size any more, and increasing the size would add to that. Hence our caution.

Could there potentially be a way to hide these DMMF enhancements behind runtime feature flag(s) for library authors in order to get these (somewhat table stakes) features?

Yes, something related has been discussed as introducing a variant of DMMF that custom generators/clients could request (can recall the location), but not with the suggestion to use a preview feature to influence this instead. That is an interesting additional option.

With that in mind, I'll continue with my efforts as-is but maybe revisit this branch with a proposal to add a flag somewhere to allow this extra functionality. For now, I'll close this PR however.

halfmatthalfcat avatar Mar 28 '23 15:03 halfmatthalfcat

We might just decide to not worry too much and update DMMF for everyone, but have not had that discussion and made that decision yet. If we do, this PR would be the first thing we look at. (But as so often, this is not really at the top of our priority list - especially with the potential downside)

janpio avatar Mar 28 '23 17:03 janpio