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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

EntityGenerator throwing given PostgreSQL index with function in condition #4911

Closed
maxsommer opened this issue Nov 8, 2023 · 5 comments
Closed

Comments

@maxsommer
Copy link

Hi! 馃憢

Firstly, thanks for your work on this project! 馃檪

Today I used patch-package to patch @mikro-orm/knex@5.9.3 for the project I'm working on.

Problem description:

  • Tried to use EntityGenerator to reverse engineer some entities from an existing database (specifically netbox)
  • That PostgreSQL database had lots of indexes - some of them with conditions with functions and type annotations
  • An example of such a condition:
(lower((name)::text), site_id) WHERE (tenant_id IS NULL)

Unfortunately as of now, generating entities with an index with this condition will fail because the function and type annotation surrounding the column name cause the following error to happen:

/Users/MYPATH/node_modules/@mikro-orm/knex/schema/DatabaseTable.js:270
        const fk = Object.values(this.foreignKeys).find(fk => fk.columnNames.includes(column.name));
                                                   ^
TypeError: Cannot read properties of undefined (reading 'name')
    at /Users/MYPATH/node_modules/@mikro-orm/knex/schema/DatabaseTable.js:270:94
    at Array.find (<anonymous>)
    at DatabaseTable.getPropertyName (/Users/MYPATH/node_modules/@mikro-orm/knex/schema/DatabaseTable.js:270:52)
    at /Users/MYPATH/node_modules/@mikro-orm/knex/schema/DatabaseTable.js:164:29
    at Array.map (<anonymous>)
    at DatabaseTable.getEntityDeclaration (/Users/MYPATH/node_modules/@mikro-orm/knex/schema/DatabaseTable.js:157:50)
    at /Users/MYPATH/node_modules/@mikro-orm/entity-generator/EntityGenerator.js:38:26
    at Array.map (<anonymous>)
    at EntityGenerator.generate (/Users/MYPATH/node_modules/@mikro-orm/entity-generator/EntityGenerator.js:29:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Here is the diff that solved my problem for right now:

diff --git a/node_modules/@mikro-orm/knex/schema/DatabaseTable.js b/node_modules/@mikro-orm/knex/schema/DatabaseTable.js
index 38c068b..961ef09 100644
--- a/node_modules/@mikro-orm/knex/schema/DatabaseTable.js
+++ b/node_modules/@mikro-orm/knex/schema/DatabaseTable.js
@@ -149,8 +149,23 @@ class DatabaseTable {
         const schema = new core_1.EntitySchema({ name, collection: this.name, schema: this.schema });
         const compositeFkIndexes = {};
         const compositeFkUniques = {};
+
         for (const index of this.indexes.filter(index => index.columnNames.length > 1)) {
-            const properties = index.columnNames.map(col => this.getPropertyName(namingStrategy, this.getColumn(col)));
+            const properties = index.columnNames.map(col => {
+
+                /**
+                 * This piece of code is a hack to remove the lower() function from the column name.
+                 * In postgres the column name reference may be wrapped in a lower() function and
+                 * is evaluated before applying, however the surrounding code is not aware of this.
+                 * To not throw an error we strip the lower() function from the column name and also
+                 * remove the datatype annotation from the column name.
+                 */
+                const fnStripRegex = /lower\((.*?)::text\)/g;
+                const fnStripped = fnStripRegex.exec(col);
+                const fnStrippedCol = fnStripped ? fnStripped[1] : col;
+
+                return this.getPropertyName(namingStrategy, this.getColumn(fnStrippedCol));
+            });
             const ret = { name: index.keyName, properties: core_1.Utils.unique(properties) };
             if (ret.properties.length === 1) {
                 const map = index.unique ? compositeFkUniques : compositeFkIndexes;

Unfortunately I'm not deep enough into any of the MikroORM code or architecture to know if this is at all a 'proper' way to fix the underlying issue or if it's maybe rather a feature gap for right now but I just wanted to post this here so maybe somebody with a better understanding could take a look at it.

@B4nan
Copy link
Member

B4nan commented Nov 8, 2023

Can you provide a schema for one table with such an index so I have something for testing?

@maxsommer
Copy link
Author

Sure.

CREATE TABLE "public"."dcim_device" (
    "created" timestamptz,
    "last_updated" timestamptz,
    "custom_field_data" jsonb NOT NULL,
    "id" int8 NOT NULL,
    "local_context_data" jsonb,
    "name" varchar,
    "_name" varchar,
    "serial" varchar NOT NULL,
    "asset_tag" varchar,
    "position" numeric,
    "face" varchar NOT NULL,
    "status" varchar NOT NULL,
    "vc_position" int2 CHECK (vc_position >= 0),
    "vc_priority" int2 CHECK (vc_priority >= 0),
    "comments" text NOT NULL,
    "cluster_id" int8,
    "role_id" int8 NOT NULL,
    "device_type_id" int8 NOT NULL,
    "location_id" int8,
    "platform_id" int8,
    "primary_ip4_id" int8,
    "primary_ip6_id" int8,
    "rack_id" int8,
    "site_id" int8 NOT NULL,
    "tenant_id" int8,
    "virtual_chassis_id" int8,
    "airflow" varchar NOT NULL,
    "description" varchar NOT NULL,
    "config_template_id" int8,
    "latitude" numeric,
    "longitude" numeric,
    "oob_ip_id" int8,
    "console_port_count" int8 NOT NULL,
    "console_server_port_count" int8 NOT NULL,
    "power_port_count" int8 NOT NULL,
    "power_outlet_count" int8 NOT NULL,
    "interface_count" int8 NOT NULL,
    "front_port_count" int8 NOT NULL,
    "rear_port_count" int8 NOT NULL,
    "device_bay_count" int8 NOT NULL,
    "module_bay_count" int8 NOT NULL,
    "inventory_item_count" int8 NOT NULL,
    CONSTRAINT "dcim_device_oob_ip_id_5e7219c1_fk_ipam_ipaddress_id" FOREIGN KEY ("oob_ip_id") REFERENCES "public"."ipam_ipaddress"("id"),
    CONSTRAINT "dcim_device_rack_id_23bde71f_fk_dcim_rack_id" FOREIGN KEY ("rack_id") REFERENCES "public"."dcim_rack"("id"),
    CONSTRAINT "dcim_device_primary_ip4_id_2ccd943a_fk_ipam_ipaddress_id" FOREIGN KEY ("primary_ip4_id") REFERENCES "public"."ipam_ipaddress"("id"),
    CONSTRAINT "dcim_device_platform_id_468138f1_fk_dcim_platform_id" FOREIGN KEY ("platform_id") REFERENCES "public"."dcim_platform"("id"),
    CONSTRAINT "dcim_device_tenant_id_dcea7969_fk_tenancy_tenant_id" FOREIGN KEY ("tenant_id") REFERENCES "public"."tenancy_tenant"("id"),
    CONSTRAINT "dcim_device_role_id_61edcc33_fk_dcim_devicerole_id" FOREIGN KEY ("role_id") REFERENCES "public"."dcim_devicerole"("id"),
    CONSTRAINT "dcim_device_config_template_id_316328c4_fk_extras_co" FOREIGN KEY ("config_template_id") REFERENCES "public"."extras_configtemplate"("id"),
    CONSTRAINT "dcim_device_site_id_ff897cf6_fk_dcim_site_id" FOREIGN KEY ("site_id") REFERENCES "public"."dcim_site"("id"),
    CONSTRAINT "dcim_device_device_type_id_d61b4086_fk_dcim_devicetype_id" FOREIGN KEY ("device_type_id") REFERENCES "public"."dcim_devicetype"("id"),
    CONSTRAINT "dcim_device_location_id_11a7bedb_fk_dcim_location_id" FOREIGN KEY ("location_id") REFERENCES "public"."dcim_location"("id"),
    CONSTRAINT "dcim_device_cluster_id_cf852f78_fk_virtualization_cluster_id" FOREIGN KEY ("cluster_id") REFERENCES "public"."virtualization_cluster"("id"),
    CONSTRAINT "dcim_device_virtual_chassis_id_aed51693_fk_dcim_virt" FOREIGN KEY ("virtual_chassis_id") REFERENCES "public"."dcim_virtualchassis"("id"),
    CONSTRAINT "dcim_device_primary_ip6_id_d180fe91_fk_ipam_ipaddress_id" FOREIGN KEY ("primary_ip6_id") REFERENCES "public"."ipam_ipaddress"("id"),
    PRIMARY KEY ("id")
);

Unfortunately my db client of choice doesn't seem to support exporting of indices, therefore I copied them from that display:

index_name;index_algorithm;is_unique;column_name;condition;comment
dcim_device_virtual_chassis_id_aed51693;BTREE;false;virtual_chassis_id;;NULL
dcim_device_unique_virtual_chassis_vc_position;BTREE;true;virtual_chassis_id,vc_position;;NULL
dcim_device_unique_rack_position_face;BTREE;true;"rack_id,""position"",face";;NULL
dcim_device_unique_name_site_tenant;BTREE;true;;(lower((name)::text), site_id, tenant_id);NULL
dcim_device_unique_name_site;BTREE;true;;(lower((name)::text), site_id) WHERE (tenant_id IS NULL);NULL
dcim_device_tenant_id_dcea7969;BTREE;false;tenant_id;;NULL
dcim_device_site_id_ff897cf6;BTREE;false;site_id;;NULL
dcim_device_rack_id_23bde71f;BTREE;false;rack_id;;NULL
dcim_device_primary_ip6_id_key;BTREE;true;primary_ip6_id;;NULL
dcim_device_primary_ip4_id_key;BTREE;true;primary_ip4_id;;NULL
dcim_device_platform_id_468138f1;BTREE;false;platform_id;;NULL
dcim_device_pkey;BTREE;true;id;;NULL
dcim_device_oob_ip_id_key;BTREE;true;oob_ip_id;;NULL
dcim_device_location_id_11a7bedb;BTREE;false;location_id;;NULL
dcim_device_device_type_id_d61b4086;BTREE;false;device_type_id;;NULL
dcim_device_device_role_id_682e8188;BTREE;false;role_id;;NULL
dcim_device_config_template_id_316328c4;BTREE;false;config_template_id;;NULL
dcim_device_cluster_id_cf852f78;BTREE;false;cluster_id;;NULL
dcim_device_asset_tag_key;BTREE;true;asset_tag;;NULL
dcim_device_asset_tag_8dac1079_like;BTREE;false;asset_tagvarchar_pattern_ops;;NULL

I assume these would be the correct SQL statements to generate the indexes in db:

CREATE INDEX dcim_device_virtual_chassis_id_aed51693 ON "public"."dcim_device" USING BTREE (virtual_chassis_id);
CREATE UNIQUE INDEX dcim_device_unique_virtual_chassis_vc_position ON "public"."dcim_device" USING BTREE (virtual_chassis_id, vc_position);
CREATE UNIQUE INDEX dcim_device_unique_rack_position_face ON "public"."dcim_device" USING BTREE (rack_id, position, face);
CREATE UNIQUE INDEX dcim_device_unique_name_site_tenant ON "public"."dcim_device" (lower(name), site_id, tenant_id);
CREATE UNIQUE INDEX dcim_device_unique_name_site ON "public"."dcim_device" (lower(name), site_id) WHERE tenant_id IS NULL;
CREATE INDEX dcim_device_tenant_id_dcea7969 ON "public"."dcim_device" USING BTREE (tenant_id);
CREATE INDEX dcim_device_site_id_ff897cf6 ON "public"."dcim_device" USING BTREE (site_id);
CREATE INDEX dcim_device_rack_id_23bde71f ON "public"."dcim_device" USING BTREE (rack_id);
CREATE UNIQUE INDEX dcim_device_primary_ip6_id_key ON "public"."dcim_device" USING BTREE (primary_ip6_id);
CREATE UNIQUE INDEX dcim_device_primary_ip4_id_key ON "public"."dcim_device" USING BTREE (primary_ip4_id);
CREATE INDEX dcim_device_platform_id_468138f1 ON "public"."dcim_device" USING BTREE (platform_id);
CREATE INDEX dcim_device_oob_ip_id_key ON "public"."dcim_device" USING BTREE (oob_ip_id);
CREATE INDEX dcim_device_location_id_11a7bedb ON "public"."dcim_device" USING BTREE (location_id);
CREATE INDEX dcim_device_device_type_id_d61b4086 ON "public"."dcim_device" USING BTREE (device_type_id);
CREATE INDEX dcim_device_device_role_id_682e8188 ON "public"."dcim_device" USING BTREE (role_id);
CREATE INDEX dcim_device_config_template_id_316328c4 ON "public"."dcim_device" USING BTREE (config_template_id);
CREATE INDEX dcim_device_cluster_id_cf852f78 ON "public"."dcim_device" USING BTREE (cluster_id);
CREATE UNIQUE INDEX dcim_device_asset_tag_key ON "public"."dcim_device" USING BTREE (asset_tag);
CREATE INDEX dcim_device_asset_tag_8dac1079_like ON "public"."dcim_device" USING BTREE (asset_tag varchar_pattern_ops);

@B4nan
Copy link
Member

B4nan commented Nov 8, 2023

Thanks, those should be probably generated as index expressions, e.g.

@Index({ name: 'custom_index_expr', expression: 'alter table `author` add index `custom_index_expr`(`title`)' })
@Property()
title!: string;

@maxsommer
Copy link
Author

I would assume so, yes :)

@B4nan
Copy link
Member

B4nan commented Nov 17, 2023

The error is no longer present with latest v6, but the entities are still not 100% correct, it will generate something like @Unique({ name: 'dcim_device_unique_name_site', properties: ['lower(name::text)', 'siteId'] }) which won't work, as the properties are validated and lower(name::text) is not a property name. Will try to deal with that too.

edit: hmm and the WHERE tenant_id IS NULL part is completely ignored, we really need that expression option there

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

2 participants