Generic dataset module and specific s3_datasets module - part 3 (Create DatasetBase db model and S3Dataset model)

[dlpzx]

Feature or Bugfix

⚠️ This PR should be merged after #1257.

• Feature
• Refactoring

Detail

As explained in the design for #1123 we are trying to implement a generic datasets_base module that can be used by any type of datasets in a generic way.

This PR does:

• Adds a generic DatasetBase model in datasets_base.db that is used in s3_datasets.db to build the S3Dataset model using joined table inheritance in sqlalchemy
• Rename all usages of Dataset to S3Dataset (in the future some will be returned to DatasetBase, but for the moment we will keep them as S3Dataset)
• Add migration script that backfills datasets table and renames s3_datasets ---> ⚠️ In the process of migrating we are doing some "scary" operations on the dataset table, if for any reason the migration encounters any issue it could result in catastrophic loss of information --> for this reason this PR implements RDS snapshots on migrations.

This PR does not:

• Feed registration stays as: FeedRegistry.register(FeedDefinition('Dataset', S3Dataset)) using Dataset with the S3Dataset resource type. It is out of the scope of this PR to migrate the Feed definition.
• Exactly the same for the GlossaryRegistry registration. We keep object_type='Dataset' to avoid backwards compatibility issues.
• It does not change the resourceType for permissions. We keep using a generic Dataset as target for S3 permissions. If we are to split permissions into DatasetBase permissions and S3Dataset permissions we would do it on a different PR

Remarks

Inserting new items of S3Dataset does not require any changes. SQL Alchemy joined inheritance automatically inserts data in the parent table and then another one to the child table as explained in this stackoverflow link (I was not able to find it in the official docs)

Relates

• Create generic dataset_base and s3_dataset modules from current datasets #1123
• Redshift Data Sharing #955
• Create RDS database snapshot before executing alembic migrations #1267

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

• Does this PR introduce or modify any input fields or queries - this includes
  fetching data from storage outside the application (e.g. a database, an S3 bucket)?
  • Is the input sanitized?
  • What precautions are you taking before deserializing the data you consume?
  • Is injection prevented by parametrizing queries?
  • Have you ensured no eval or similar functions are used?
• Does this PR introduce any functionality or component that requires authorization?
  • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  • Are you logging failed auth attempts?
• Are you using or adding any cryptographic features?
  • Do you use a standard proven implementations?
  • Are the used keys controlled by the customer? Where are they stored?
• Are you introducing any new policies/roles/users?
  • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[dlpzx]

Testing (before changes from PR review)

• locally update and list pre-existing datasets
• locally update and create new dataset - check that it creates successfully in database, it indexes in catalog and its permissions are created for the resource type Datasets
• locally run all migration scripts from scratch - without data
• locally downgrade each of the migration scripts one by one - without data
• locally downgrade both migration scrips at once - without data
• in a pre-existing AWS deployment merge this branch and check migration is correctly executed
• in a pre-existing AWS deployment check previous datasets are listed and can be accessed (checking permissions)
• in a pre-existing AWS deployment create a new Dataset

[noah-paige]

Overall left some minor comments - additionally tested out the migrations scripts back-filling a single dataset and all works locally for me as well

Will do one last look through first thing tomorrow once you are finished with you check list of testing as well

[noah-paige]

On downgrade I am getting an error column "label" of relation "s3_dataset" contains null values even after we set the label value in the above for loop

[dlpzx]

I will re-test with the changes from PR review, see the results below

[dlpzx]

Testing after changes from review

• locally create datasets with migrations until revision 458572580709, update schema with alembic migration d059eead99c2 -> check datasetType is backfilled, foreign keys are updated, dataset is renamed
• downgrade back to 458572580709 -> check that datasetType is deleted and object datasettype enum as well. Check that we have now a foreign key fk_dataset_env_uri and the dataset table is called dataset
• Upgrade to head -> check that new dataset dataset table is backfilled, datasettypes enum is used for dataset type
• Same for downgrade

[noah-paige]

Last last things:

WHen running alembic autogenerate migration I get the following

def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.alter_column('dataset', 'datasetType',
               existing_type=postgresql.ENUM('S3', name='datasettypes'),
               type_=sa.Enum('S3', name='datasettype'),
               existing_nullable=False)
    op.drop_constraint('s3_dataset_bucket_datasetUri_fkey', 'dataset_bucket', type_='foreignkey')
    op.create_foreign_key(None, 'dataset_bucket', 'dataset', ['datasetUri'], ['datasetUri'], ondelete='CASCADE')
    # ### end Alembic commands ###

Our models and migration scripts may not be in sync?

[noah-paige]

Changing datasetUri in dataset_bucket table model at backend/dataall/modules/s3_datasets/db/dataset_models.py to

    datasetUri = Column(String, ForeignKey('s3_dataset.datasetUri', ondelete='CASCADE'), nullable=False)

fixes the foreign key change generated by alembic

[dlpzx]

Hello hello, thanks @noah-paige for such a deep review :)

• foreign_key in dataset_bucket - I can make the change, looks reasonable
• postgresql.ENUM vs sa.Enum - there is a problem with sa.Enum and that is that it forces the creation of the enum object itself. I used postgresql.ENUM because it has the option to create_type=False which uses the existing datasettypes object. otherwise the migration fails

[noah-paige]

tested and looks good - approving