dbsrgits / sql-translator

SQL::Translator (SQLFairy)
http://sqlfairy.sourceforge.net/
82 stars 91 forks source link

Allow constraint name re-use in diff file after dropping (Oracle) #171

Open hazardv opened 6 months ago

hazardv commented 6 months ago

This appears to be an issue only if you are using DBIx::Class::Schema and calling create_ddl_dir with a previous version specified. With that being said...

If you have an Oracle schema with tables/constraints identified as version 0.1:

CREATE TABLE app_category (
  id number(11) NOT NULL,
  name varchar2(256) NOT NULL,
  description varchar2(4000),
  PRIMARY KEY (id)
);

CREATE TABLE app_alert (
  id number(11) NOT NULL,
  category number(11) NOT NULL,
  name varchar2(512) NOT NULL,
  PRIMARY KEY (id)
);

ALTER TABLE app_alert ADD CONSTRAINT app_alert_category_fk FOREIGN KEY (category) REFERENCES app_category (id);

and in version 0.2 you change the app_alert_category_fk constraint to cascade deletes

ALTER TABLE app_alert ADD CONSTRAINT app_alert_category_fk FOREIGN KEY (category) REFERENCES app_category (id) ON DELETE CASCADE;

but all other things remain the same, I would expect the diff file that gets generated by $schema->create_ddl_dir() to look like this:

ALTER TABLE app_alert DROP CONSTRAINT app_alert_category_fk;

ALTER TABLE app_alert ADD CONSTRAINT app_alert_category_fk FOREIGN KEY (category) REFERENCES app_category (id) ON DELETE CASCADE;

This is the way it works when you are NOT using DBIx::Class::Schema.

When using DBIx::Class::Schema, the producer has already produced the SQL from the perl and has all of the constraint names in the %global_names hash of the Oracle producer so when it's handling the diff things get a bit messy. Instead of the expected diff above, you end up with

ALTER TABLE app_alert DROP CONSTRAINT app_alert_category_fk;

ALTER TABLE app_alert ADD CONSTRAINT app_alert_category_fk01 FOREIGN KEY (category) REFERENCES app_category (id) ON DELETE CASCADE;

This is an issue because the version 0.2 schema has

ALTER TABLE app_alert ADD CONSTRAINT app_alert_category_fk FOREIGN KEY (category) REFERENCES app_category (id) ON DELETE CASCADE;

so if you went directly to version 0.2 your constraint is named app_alert_category_fk and if you upgraded from version 0.1 to 0.2 then your constraint is named app_alert_category_fk01.

My proposed change involves decrementing the value in %global_names when a constraint is dropped so that the name can be reused.

hazardv commented 6 months ago

I realize I don't have any tests to prove this or prove how the change fixes the issue. I have a separate super minimal project that I used to test it on. I didn't know if you wanted me to include that because it requires all of DBIx::Class.

rabbiveesh commented 6 months ago

ya, let's not require DBIC for our test suite. I think you should be able to just make a test which sets up this situation artificially, with comments explaining how this would happen IRL.