fix: program rule variable name check now also check UID to prevent duplicates [DHIS2-11330] #8220
Conversation
gnespolino
requested review from
ameenhere,
enricocolasante,
abyot,
stian-sandvold,
lucaCambi77 and
zubaira
gnespolino
changed the title
| // since prvWithSameNameAndSameProgram is not empty, fail if it's an | ||
| // insert or if there is more than one prv with same name and same | ||
| // program. | ||
| if ( !UPDATE_STRATEGIES.contains( bundle.getImportMode() ) || prvWithSameNameAndSameProgram.size() > 1 ) |
There was a problem hiding this comment.
If there is more than one in the DB already, doesn't mean that data in DB is wrong?
There was a problem hiding this comment.
that's true, but this specific case means that if INSERT, in DB it is expected to be 0, if UPDATE, in DB there can be at most 1.
when in database there are 2 or more, the check will fail anyway. Or am I missing something ?
There was a problem hiding this comment.
ok maybe I got it... maybe we can have a failing "legal" update just because data in DB is bad. Yes this might happen as well, but it shouldn't honestly. Here I just wanted to make it simpler the following part by assuming that the list only contained one element.
There was a problem hiding this comment.
then what I can do is remove size()>1 condition and since it's an update, ensure at least one in the List have the same UID as the PRV we are processing in the validator, what would you think ?
gnespolino
added
Ready for review
run-api-tests
| { | ||
| Query<ProgramRuleVariable> query = getProgramRuleVariableQuery( programRuleVariable ); | ||
| List<ProgramRuleVariable> prvWithSameNameAndSameProgram = getProgramRuleVariableQuery( programRuleVariable ) | ||
| .getResultList(); |
There was a problem hiding this comment.
I think this approach is too verbose, in my opinion we should simply try to do a query also using prv UID.
After that if it is a create strategy and we get a record, we have an error. We also have an error if we don't get a result and it is an update.
I think this way is much cleaner, less code and maintenable
There was a problem hiding this comment.
you're right, I'll review it
There was a problem hiding this comment.
better thinking, we should
- fail when it's an insert and a PRV with same name/program already exists
- fail when it's an update and a PRV with same name/program/uid already exists
so I think it's not possible to verify the first check adding UID in the query filter
|
Kudos, SonarCloud Quality Gate passed! |
…uplicates [DHIS2-11330] (#8220) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
…uplicates [DHIS2-11330] (#8220) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
…uplicates [DHIS2-11330] (#8220) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
…uplicates [DHIS2-11330] (#8220) (#8365) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
…uplicates [DHIS2-11330] (#8220) (#8367) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
…uplicates [DHIS2-11330] (#8220) (#8366) * fix: program rule variable name check should be now properly implemented * fix: changed check implementation to tolerate bad database data * fix: added test case * style: formatting * fix: compilation issue with JDK 9 * style: formatting (cherry picked from commit c525517)
The case that was not covered in the previous implementation was:
in this case, the check was failing, allowing PRV to be duplicated.
With the current implementation, in case of updates, also UIDs are checked.