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

Inserting NOT NULL auto-generated properties #25

Closed
PizzaPartyInc opened this issue Jul 22, 2020 · 11 comments
Closed

Inserting NOT NULL auto-generated properties #25

PizzaPartyInc opened this issue Jul 22, 2020 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@PizzaPartyInc
Copy link

PizzaPartyInc commented Jul 22, 2020

Hi,

I'm trying out zapatos and stumbled on one usage obstacle.

Let's say I have a table like this:

CREATE TABLE app_public.assets (
  id INT NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY,
  title text NOT NULL,
  created_by text NOT NULL
);

id is auto-generated and created_by is populated automatically by a trigger function BEFORE an insert.

Generated Insertable interface will then look like this:

  export interface Insertable {
    id: number | SQLFragment;
    title: string | SQLFragment;
    created_by: string | SQLFragment;
  }

Which means that on inserts I cannot use an object like {title: 'some title'}, it will complain that id and created_by are missing.
Is there any way to work around this? (except for removing NOT NULL from both id and created_by)

Edit: Actually, even if I define id as INT PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, in resulting schema it will still be marked as NOT NULL and Insertable will still have it as required...

Thanks! :)

@jawj
Copy link
Owner

jawj commented Jul 22, 2020

Thanks — interesting.

I hadn't run into this with primary keys. I tend to declare them the old Postgres way as id SERIAL PRIMARY KEY. It turns out this puts a non-null value in information_schema.columns.column_default, and if there's a default value defined there, Zapatos makes the column optional on insert.

$ create temporary table tmp_serial (id SERIAL PRIMARY KEY);
CREATE TABLE

$ select column_default from information_schema.columns where table_name = 'tmp_serial';
             column_default             
----------------------------------------
 nextval('tmp_serial_id_seq'::regclass)
(1 row)

However, when we do it your way (which I can see is standards-conformant and has some benefits), information_schema.columns.column_default appears to end up NULL, and this causes problems.

$ create temporary table tmp_gbdai (id INT PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY); 
CREATE TABLE

$ select column_default from information_schema.columns where table_name = 'tmp_gbdai';
 column_default 
----------------
 
(1 row)

I also hadn't run into this with triggers, just because I haven't used triggers with Zapatos, but funnily enough we've been discussing setting up triggers for our createdAt and updatedAt fields on the work API, so we're likely to come up against it soon.

My thoughts are that we can either try to be clever, and detect these GENERATED BY DEFAULT AS IDENTITY columns — and perhaps even columns filled in by triggers (?) — and/or we can add some new config options to the schema generation process — maybe something like optionalInsert (with a list of columns to make optional in Insertables) and noInsertUpdate (with a list of columns to wholly omit from Insertables and Updatables).

I'll look into this and make one of those happen.

@jawj
Copy link
Owner

jawj commented Jul 22, 2020

OK, based on https://www.postgresql.org/docs/current/infoschema-columns.html, for the primary key issue it looks like we should be checking is_identity/identity_generation (and also is_generated) and acting accordingly.

As far as I can see there's no sensible way we can tell if a particular column is set by a BEFORE INSERT or BEFORE UPDATE trigger, so additional config options will be needed for that.

To help me prioritise this — is it holding you up?

@PizzaPartyInc
Copy link
Author

Thanks for the quick reply! :)

I wouldn't say that these are blockers, but the Identity fix would be super nice to have. Triggers are a bit less important, since they can be worked around by setting a default value. :)

@jawj
Copy link
Owner

jawj commented Jul 22, 2020

I wouldn't say that these are blockers, but the Identity fix would be super nice to have.

Right. I'll do this one as soon as I can.

Triggers are a bit less important, since they can be worked around by setting a default value. :)

Indeed — in fact, created-at and updated-at columns would I guess generally take a default of NOW() anyway.

@jawj jawj added the bug Something isn't working label Jul 22, 2020
@jawj jawj self-assigned this Jul 22, 2020
@PizzaPartyInc
Copy link
Author

Indeed — in fact, created- updated-at columns would I guess generally take a default of NOW() anyway.

Yep. My case was with created_by/updated_by, which is a username string, but it too can be set to something like "Unknown" or "Anonymous" :)

@jawj jawj closed this as completed in 0b98413 Jul 22, 2020
@jawj
Copy link
Owner

jawj commented Jul 22, 2020

OK, 0.1.51 should treat identity columns the same as columns with a default value, which essentially they are.

Do let me know if this doesn't fix things for you. And, in fact, it would also be nice to get confirmation if it does!

I think I'll leave the new config options I proposed for now, and see whether the default value work-around is good enough for people.

@PizzaPartyInc
Copy link
Author

Thanks for the quick fix!

As far as I can see, it works just as expected. :)

@champikasam
Copy link

I'm currently checking the possibility of using Zapatos in my project. I'm also facing this same issue. I have a number of tables with some mandatory columns whose values are being set by a before insert trigger. These columns are not identity columns, so the identity fix does not work in my case. Setting a database default does not make much sense either considering the purpose of these columns.

It would be great if the additional config option (eg: optionalInsert) is added so that I can use it to make these columns optional during insert.

@jawj jawj reopened this Nov 16, 2020
@jawj
Copy link
Owner

jawj commented Nov 16, 2020

OK, I'll have a think about this.

@jawj jawj added enhancement New feature or request and removed bug Something isn't working labels Nov 16, 2020
@jawj
Copy link
Owner

jawj commented Nov 19, 2020

OK, current thinking is this: we add a columnOptions key to the schema generation config, which now looks like this:

export interface RequiredConfig {
  db: pg.ClientConfig;
}

export interface OptionalConfig {
  outDir: string;
  schemas: SchemaRules;
  progressListener: boolean | ((s: string) => void);
  warningListener: boolean | ((s: string) => void);
  customTypesTransform: 'PgMy_type' | 'my_type' | 'PgMyType' | ((s: string) => string);
  columnOptions: {
    [k: string]: {  // table name
      [k: string]: {  // column name
        insert?: 'auto' | 'disabled' | 'optional';
        update?: 'auto' | 'disabled';
      };
    };
  };
}

So you'll be able to manually disallow inserts and/or updates (e.g. if your column is set by a trigger), which removes the column from the relevant Insertable and/or Updatable. You'll also be able to force the column to be optional on inserts, which appends a ? to the relevant Insertable key (all columns are optional on update anyway).

I've also now made it so GENERATED ALWAYS AS (/* ... */) STORED and GENERATED ALWAYS AS IDENTITY columns are automatically removed from the relevant Insertable and Updatable, which has been a longstanding TODO item.

This is in master and will probably be released in the next few days, unless I find any problems or think of a nicer interface meanwhile.

@jawj
Copy link
Owner

jawj commented Nov 23, 2020

OK — I've renamed disabled to excluded and added support for treating a particular column name the same in all tables, and this is now released in v3.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants