Skip to content

Comments

Soft Deletion of Script#330

Open
Suyash-d wants to merge 51 commits intodevelopfrom
feature/34993e6/Soft-deletion-of-scripts
Open

Soft Deletion of Script#330
Suyash-d wants to merge 51 commits intodevelopfrom
feature/34993e6/Soft-deletion-of-scripts

Conversation

@Suyash-d
Copy link
Contributor

@Suyash-d Suyash-d commented May 3, 2021

Is your feature request related to a problem? Please describe.
Instead of physical deleting a script from the repository when the DELETE /api/scripts endpoint is called, the script should be marked as 'inactive'. This way scripts can be re-enabled should the script be deleted by accident.

When retrieving scripts using the GET /api/scripts, GET /api/scripts/<name> and GET /api/scripts/<name>/<version> endpoints, only active scripts should be considered.

Starting documentation:

Tasks

  • define logical deletion strategy
  • add column to scripts table to denote logical deletion flag
  • adapt ScriptConfiguration to handle logical delete for DELETE action
  • adapt ScriptDtoRepository to handle logical deletion of scripts during FETCH actions
  • adapt unique constraint on script table (should primary key of Script/ScriptVersion be adapted?)

Approach

  • Every script version has a field 'deleted_at'. If a script version is active, the field 'deleted_at' has the value 'NA'. When a script version is soft deleted, the 'deleted_at' field will be set to the current timestamp. All script versions having a 'deleted_at' field different than 'NA' are inactive.
  • Every script has a field 'deleted_at'. If a script has any active script versions, the 'deleted_at' field is set to 'NA'. Should all script versions be soft deleted, then the 'delete_at' field of the script is set to the current timestamp.

Verification
The following should be working:

  • Scenario 1: Delete an existing script
    1. perform GET /api/scripts
    2. perform DELETE /api/scripts/<name>/<version> for 1 specific script
    3. perform GET /api/scripts and verify it does not contain the <name> and <version>
    4. perform GET /api/scripts/<name>/<version> and returns a 404
  • Scenario 2: Recreate a deleted script
    1. perform GET /api/scripts
    2. perform DELETE /api/scripts/<name>/<version> for 1 specific script
    3. perform GET /api/scripts and verify it does not contain the <name> and <version>
    4. perform GET /api/scripts/<name>/<version> and returns a 404
    5. perform POST /api/scripts using the <name> and <version> previously removed
    6. perform GET /api/scripts and verify it does contain the <name> and <version>
    7. perform GET /api/scripts/<name>/<version> and returns a 200
  • Scenario 3: Delete a recreated script
    1. perform GET /api/scripts
    2. perform DELETE /api/scripts/<name>/<version> for 1 specific script
    3. perform GET /api/scripts and verify it does not contain the <name> and <version>
    4. perform GET /api/scripts/<name>/<version> and returns a 404
    5. perform POST /api/scripts using the <name> and <version> previously removed
    6. perform GET /api/scripts and verify it does contain the <name> and <version>
    7. perform GET /api/scripts/<name>/<version> and returns a 200
    8. perform DELETE /api/scripts/<name>/<version>
    9. perform GET /api/scripts and verify it does not contain the <name> and <version>
    10. perform GET /api/scripts/<name>/<version> and returns a 404
  • Scenario 4: reinstate a deleted script
    1. perform GET /api/scripts
    2. perform DELETE /api/scripts/<name>/<version> for 1 specific script
    3. perform GET /api/scripts and verify it does not contain the <name> and <version>
    4. perform GET /api/scripts/<name>/<version> and returns a 404
    5. set 'deleted_at' column of script:<name>-<version> in DB to NA
    6. perform GET /api/scripts and verify it does contain the <name> and <version>
    7. perform GET /api/scripts/<name>/<version> and returns a 200

Test them by creating a new IESI sandbox from scratch, load the dummy data and verify the scenario's above using Postman

@Suyash-d Suyash-d added the enhancement New feature or request label May 3, 2021
@Suyash-d Suyash-d added this to the v0.7.0 milestone May 3, 2021
@Suyash-d Suyash-d self-assigned this May 3, 2021
@BerrevoetsRobbe
Copy link
Collaborator

The following functionalities need to be discussed:

  • does 'deleted_at' become part of the primary key of a Script and ScriptVersion?
  • add functionality to restore an 'inactive' script
  • keep the hard delete logic
  • create tests related to the soft delete of a tests

@BerrevoetsRobbe
Copy link
Collaborator

update ScriptKey to contain the deletedAt field. Created the following constructors:

    public ScriptKey(String scriptId, long scriptVersion) {
        this.scriptId = scriptId;
        this.scriptVersion = scriptVersion;
        this.deletedAt = "NA";
    }

    public ScriptKey(String scriptId, long scriptVersion, String deletedAt) {
        this.scriptId = scriptId;
        this.scriptVersion = scriptVersion;
        this.deletedAt = deletedAt;
    }

@BerrevoetsRobbe
Copy link
Collaborator

remove core/java/iesi-rest-without-microservices/null

@BerrevoetsRobbe
Copy link
Collaborator

@Suyash-d could you verify the scenarios added to the description?
I have a problem with Scenario 2

" and SCRIPT_VRS_NB = " + SQLTools.getStringForSQL(scriptParameterKey.getScriptKey().getScriptVersion()) +
" and SCRIPT_PAR_NM = " + SQLTools.getStringForSQL(scriptParameterKey.getParameterName()) + ";";
" and SCRIPT_PAR_NM = " + SQLTools.getStringForSQL(scriptParameterKey.getParameterName()) +
" and DELETED_AT = 'NA' ;";
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

+ " where SCRIPT_ID = " + SQLTools.getStringForSQL(scriptKey.getScriptId()) +
" and SCRIPT_VRS_NB = " + SQLTools.getStringForSQL(scriptKey.getScriptVersion()) + ";";
" and SCRIPT_VRS_NB = " + SQLTools.getStringForSQL(scriptKey.getScriptVersion()) +
" and DELETED_AT = 'NA' ;";
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

String queryScriptVersion = "select SCRIPT_ID, SCRIPT_VRS_NB, SCRIPT_VRS_DSC, LAST_MODIFIED_BY, LAST_MODIFIED_AT from " + getMetadataRepository().getTableNameByLabel("ScriptVersions")
+ " where SCRIPT_ID = " + SQLTools.getStringForSQL(scriptVersionKey.getScriptKey().getScriptId()) +
String queryScriptVersion = "select SCRIPT_ID, SCRIPT_VRS_NB, SCRIPT_VRS_DSC, DELETED_AT, LAST_MODIFIED_BY, LAST_MODIFIED_AT from " + getMetadataRepository().getTableNameByLabel("ScriptVersions")
+ " where DELETED_AT = 'NA' and SCRIPT_ID = " + SQLTools.getStringForSQL(scriptVersionKey.getScriptKey().getScriptId()) +
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

Copy link
Collaborator

@BerrevoetsRobbe BerrevoetsRobbe left a comment

Choose a reason for hiding this comment

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

part 1

Copy link
Collaborator

@BerrevoetsRobbe BerrevoetsRobbe left a comment

Choose a reason for hiding this comment

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

part 2

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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants