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

Should nullable columns always be optional? #76

Closed
jgonera opened this issue Jun 28, 2021 · 3 comments
Closed

Should nullable columns always be optional? #76

jgonera opened this issue Jun 28, 2021 · 3 comments

Comments

@jgonera
Copy link

jgonera commented Jun 28, 2021

Given this table in a PostgreSQL database:

CREATE TABLE my_table (
    id uuid DEFAULT gen_random_uuid() PRIMARY KEY,
    value_one numeric NOT NULL,
    value_two numeric
);

I can run the following query:

INSERT INTO my_table (value_one) VALUES (123);

However, sql-ts will generate types that look like this:

export interface MyTableEntity {
  id?: string;
  value_one: string;
  value_two: string | null;
}

which makes specifying value_two mandatory. I think a more correct type would be:

export interface MyTableEntity {
  id?: string;
  value_one: string;
  value_two?: string | null;
}

I can get around it by modifying the handlebars template:

"{{propertyName}}"{{#if optional}}?{{else}}{{#if nullable}}?{{/if}}{{/if}}: {{propertyType}} {{#if nullable}}| null {{/if}}

It seems though that perhaps optional should be true for column value_two since I don't need to provide a value for this column in an INSERT. Is there something I might be missing?

@sam-outschool
Copy link

I agree that nullable columns should "default" to null without another explicit default. Omitting the value on insert in SQL would result in the value being null, and query builders like knex have the same interface.

@rmp135
Copy link
Owner

rmp135 commented Sep 4, 2021

Thinking about it, you've convinced me.

Given the following different column null / default configurations.

NullableWithDefault
NullableWithoutDefault
NotNullableWithDefault 
NotNullableWithoutDefault

Should output the following

NullableWithDefault?: string | null
NullableWithoutDefault? : string | null
NotNullableWithDefault?: string
NotNullableWithoutDefault: string

Essentially null acts like a default and the only instance in which a column isn't optional is when it's not nullable and there's no default value.

I've gone through and updated all dialects to work as above, fixing a couple of bugs along the way. Hopefully be able to get it out within a day or so.

@rmp135
Copy link
Owner

rmp135 commented Sep 6, 2021

This is fixed in version 1.10.0. All clients have had their optionality and nullability standardised as per the above.

@rmp135 rmp135 closed this as completed Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants