feat: Add not-null constraint for column trackedentitytypeid in trackedentity table [DHIS2-15066] #19112
Conversation
zubaira
changed the title
zubaira
changed the title
There was a problem hiding this comment.
We are having discussion about the approach here (that is the same for organisationunitid column in event and enrollment) and we are trying to avoid to create dummy data that are likely entering the DB and never being deleted.
I think we should fix this the same way in all the places where we found it.
As soon as we have a strategy we should go back to this PR and see what we should do.
...rvice-tracker/src/test/java/org/hisp/dhis/tracker/deduplication/DeduplicationHelperTest.java
Outdated
Show resolved
Hide resolved
...rvice-tracker/src/test/java/org/hisp/dhis/tracker/deduplication/DeduplicationHelperTest.java
Outdated
Show resolved
Hide resolved
| femaleA.setTrackedEntityType(trackedEntityType); | ||
| femaleB.setTrackedEntityType(trackedEntityType); |
There was a problem hiding this comment.
setTrackedEntityType
.../dhis/tracker/imports/validation/validator/trackedentity/SecurityOwnershipValidatorTest.java
Show resolved
Hide resolved
.../dhis/tracker/imports/validation/validator/trackedentity/SecurityOwnershipValidatorTest.java
Outdated
Show resolved
Hide resolved
...org/hisp/dhis/tracker/imports/validation/validator/event/SecurityOwnershipValidatorTest.java
Show resolved
Hide resolved
...isp/dhis/tracker/imports/validation/validator/enrollment/SecurityOwnershipValidatorTest.java
Outdated
Show resolved
Hide resolved
...rg/hisp/dhis/tracker/imports/validation/validator/enrollment/DataRelationsValidatorTest.java
Show resolved
Hide resolved
| WHERE trackedentitytypeid IS NULL | ||
| ) | ||
| LOOP | ||
| UPDATE trackedentity |
There was a problem hiding this comment.
Here we shouldn't update any record.
We should only apply the constraint.
The we can suggest such a script in the migration notes
There was a problem hiding this comment.
there is a lower possibility of errors if we handle the task ourselves rather than leaving it to the user. Therefore, we should aim to complete as much of the migration process as possible and leave as little as necessary for the user to manage.
There was a problem hiding this comment.
For this kind of process that deal with data we need to be much more clear with the user and very very careful on any step.
Here you are fixing inconsistent data in a correct way, getting the tracked entity type from any underling enrollment but this is not the only issue that could happen and this SQL is running in a transaction (even without specifically start the transaction as done here, flyway will wrap this in a transaction), so if there is any inconsistent data that cannot be fixed in the way you provided, no data is fixed.
Than the user is sent to the guide and there we are not providing any explanation of what we attempted in the migration and we are not providing any option of fixing the data, only a script to delete tracked entitities and with all the enrollments and events.
To resume:
If the user has a number x of "fixable" tracked entities and one that is not automatically fixable, we are deleting ALL the tracked entities with inconsistent data.
...urces/org/hisp/dhis/db/migration/2.42/V2_42_26__Add_not_null_constraint_to_trackedentity.sql
Outdated
Show resolved
Hide resolved
|
Before we merge this, can we make sure we update the dev db snapshot to have this "fixing scripts" run? So that we don't break dev again for Phil to fix later. |
...urces/org/hisp/dhis/db/migration/2.42/V2_42_26__Add_not_null_constraint_to_trackedentity.sql
Outdated
Show resolved
Hide resolved
| WHERE trackedentitytypeid IS NULL | ||
| ) | ||
| LOOP | ||
| UPDATE trackedentity |
There was a problem hiding this comment.
For this kind of process that deal with data we need to be much more clear with the user and very very careful on any step.
Here you are fixing inconsistent data in a correct way, getting the tracked entity type from any underling enrollment but this is not the only issue that could happen and this SQL is running in a transaction (even without specifically start the transaction as done here, flyway will wrap this in a transaction), so if there is any inconsistent data that cannot be fixed in the way you provided, no data is fixed.
Than the user is sent to the guide and there we are not providing any explanation of what we attempted in the migration and we are not providing any option of fixing the data, only a script to delete tracked entitities and with all the enrollments and events.
To resume:
If the user has a number x of "fixable" tracked entities and one that is not automatically fixable, we are deleting ALL the tracked entities with inconsistent data.
| ALTER TABLE IF EXISTS trackedentity ALTER COLUMN trackedentitytypeid SET NOT NULL; | ||
| ELSE | ||
| RAISE NOTICE 'No inconsistencies found, trackedentitytypeid is already populated.'; | ||
| ALTER TABLE IF EXISTS trackedentity ALTER COLUMN trackedentitytypeid SET NOT NULL; |
There was a problem hiding this comment.
I did not understand why do we have two ALTER COLUMN SET NOT NULL on the same table-column combination?
There was a problem hiding this comment.
If the initial migration fails due to invalid data, it will raise an exception, prompting the user to correct or remove the problematic data. Once the issues are resolved, the same migration can be reapplied, and the ELSE condition will enforce the NOT NULL constraint on the column.
There was a problem hiding this comment.
Then, can we move the ALTER TABLE to outside the IF ELSE condition, since it is executed at the end anyway?
|
|
Any outstanding review comments for this? @enricocolasante @zubaira (Couldn't go through it completely) |
I think here we are done or almost done, but there is more refining to do in the release notes. |
DHIS2-15066
createTrackedEntitymethod signature inTestBase.javato make sure user provide a non-nullTrackedEntityTypetoTrackedEntity.