yugabyte / yugabyte-db

YugabyteDB - the cloud native distributed SQL database for mission-critical applications.
https://www.yugabyte.com
Other
8.88k stars 1.05k forks source link

[Platform] Customer config records are not removed on customer deletion #8298

Open SergeyPotachev opened 3 years ago

SergeyPotachev commented 3 years ago

We need to update the customer_config scheme to use foreign key and cascade deletion. Also please update CustomerControllerTest.testCustomerDELETEWithValidUUID to track these changes.

adarsh312 commented 3 years ago

@streddy-yb can i pick this issue ?

Tigojiang commented 3 years ago

Hello, I am a first-time contributor, may I pick up this issue if it is not finished? Could you please help point out the files that needed to be changed? Thank you!

iSignal commented 3 years ago

@adarsh312 : feel free to pick it up. You can join the channel #topic-platform-dev on our Slack channel at http://yugabyte.com/slack for more information on how to build Platform code.

@SergeyPotachev : can you add more details about the files involved? Can we test these through the unit test alone or would you suggest UI testing as well?

iSignal commented 3 years ago

@Tigojiang : maybe you can consider https://github.com/yugabyte/yugabyte-db/issues/5876 instead?

SergeyPotachev commented 3 years ago

The problem is very simple: we have CustomerConfig entity which has a field customerUUID. Unfortunately, this field is not declared as a foreign key:

           Table "public.customer_config"
    Column     |          Type          | Modifiers 
---------------+------------------------+-----------
 config_uuid   | uuid                   | not null
 customer_uuid | uuid                   | not null
 type          | character varying(30)  | not null
 name          | character varying(100) | not null
 data          | json_alias             | not null
 config_name   | character varying(100) | 
Indexes:
    "pk_customer_config" PRIMARY KEY, btree (config_uuid)
    "unique_config_name" btree (config_uuid, name, config_name)

So if you delete customers (using CustomerController::delete(uuid)), customer config entries still remain in the database. The change is to introduce the new migration which declares this field as a foreign key. Our migrations are situated here: https://github.com/yugabyte/yugabyte-db/tree/master/managed/src/main/resources/db/migration/default

The main rule we are trying to follow is to make migrations idempotent. So before creation of the foreign key we need to check that there is no such key already exists. For PostgreSQL migration it could be done using something like:

DO
$$
  BEGIN
    -- Look for our constraint
    IF NOT EXISTS (select constraint_name 
                   from information_schema.table_constraints 
                   where table_name='customer_config' and constraint_name='fk_customer_uuid') THEN
        EXECUTE 'alter table customer_config add constraint fk_customer_uuid FOREIGN KEY (customer_uuid) REFERENCES customer(uuid) ON UPDATE CASCADE ON DELETE CASCADE;';
    END IF;
  END;
$$;

This code is database specific, so our migration should be placed in yugabyte-db/managed/src/main/resources/db/migration/default/postgres (PostgreSQL version) and in yugabyte-db/managed/src/main/resources/db/migration/default/h2 (H2, used for junit tests). BTW, in the H2 migration we don't need to check the foreign key existence. As example, see migration V75Alter_Provider: https://github.com/yugabyte/yugabyte-db/blob/master/managed/src/main/resources/db/migration/default/postgres/V75__Alter_Provider.sql and https://github.com/yugabyte/yugabyte-db/blob/master/managed/src/main/resources/db/migration/default/h2/V75Alter_Provider.sql

SergeyPotachev commented 3 years ago

The next step is to add a small test to CustomerControllerTest.java. We already have test testCustomerDELETEWithValidUUID there. So we can simply extend it - to create CustomerConfig record at the function beginning and to check that it is deleted together with the customer at the end.

adarsh312 commented 3 years ago

@iSignal @SergeyPotachev Thanks for helping. I have started working on issue.