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

Refactor ActivityLog DataBase Tables #273

Merged
merged 4 commits into from
Apr 24, 2023

Conversation

nnikolopoulos
Copy link
Contributor

Introduce IOActivity and IOContent tables (this PR closes issue #266).

  • Rename ImportExportActivity to IOActivity.
  • Rename KeyedImportExport to IOContent and make it non-abstract.

The proposed changes result to a new table (IOContent) in the DB which should contain information that previously existed in the tables: ExportFile, ImportFile, ImportString, ImportDataSet and ImportStream (IO Content replaces the aforementioned tables).

@nnikolopoulos nnikolopoulos added the enhancement New feature or request label Apr 19, 2023
@nnikolopoulos nnikolopoulos added this to the v1.2.x milestone Apr 19, 2023
@nnikolopoulos nnikolopoulos self-assigned this Apr 19, 2023
@nnikolopoulos nnikolopoulos requested a review from dcolleoni April 19, 2023 16:08
@nnikolopoulos nnikolopoulos marked this pull request as ready for review April 20, 2023 08:57
@nnikolopoulos nnikolopoulos linked an issue Apr 20, 2023 that may be closed by this pull request
Copy link
Contributor

@dcolleoni dcolleoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a meeting where you show me the DB with data

Copy link
Contributor

@andrey-katz-systemorph andrey-katz-systemorph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I went through all the files and in my humble opinion it does not do what you would like it to do and the problems are actually multifold.

  1. You generate upon migration tables for any non abstract class, there is no guarantee that they are populated properly though.
  2. At the moment you call .WithActivityLog() on the import, you generate ImportBuilderWriter. Take a look how the ExecuteAsync() works. It calls the ReportInputAndUpdateActivityAsync and writes the result in the TImport table, which is arguably not what you want.
  3. If you want the IOContent to be populated, you should get rid all the records that you do not want, the import inside the ReportInputAndUpdateActivityAsync should be of IOContent type (and TImport becomes obsolete all together) and consequently move all the logic that is now defined at the lower layers up all the way to the IOContent. There might be many variations to this "recipee" but I believe this roughly the way we should move.
  4. As a general rule, I believe it is healthy to avoid the situation that abstract record inherits from the nonabstract one, even if it does not spit out a compilation error.

@nnikolopoulos
Copy link
Contributor Author

Following discussion with @dcolleoni I tried to avoid a major restructuring of the ActivityLog notebook.

Therefore I did not proceed with the requested changes by @andrey-katz-systemorph.

The latest commits:

  • move Name and ContentType properties from ImportFile and ExportFile records all the way up to the IOContent record and also
  • update the migration notebooks.

@andrey-katz-systemorph the resulting IOContent table in the DataBase taking these changes into account looks like this after some mock imports / exports:

image

image

Currently the abstract modifier in the KeyedImport record is used to prevent the creation of a KeyedImport table in the DB.

@dtrzesniak perhaps you can also review the changes at this point.

@nnikolopoulos nnikolopoulos merged commit 2116d17 into develop Apr 24, 2023
@nnikolopoulos nnikolopoulos deleted the RefactorActivityLogDataBaseTables branch April 24, 2023 13:48
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
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor activity log db Tables
4 participants