feat: Merge Category Option Combo [DHIS2-18321] #19488
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...is-service-core/src/main/java/org/hisp/dhis/datavalue/hibernate/HibernateDataValueStore.java
Fixed
Show fixed
Hide fixed
|
| } | ||
|
|
||
| @Override | ||
| public int hardDeleteEvents(List<String> eventsToDelete, String eventSelect, String eventDelete) { |
There was a problem hiding this comment.
@ameenhere this method has been extracted from the existing code to allow reuse of hard deleting Events. This ensures that all Event references are handled consistently & appropriately.
| */ | ||
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
this class will be removed in a later PR as we want to merge data values using SQL, not Java. Data Element merge will be updated next, which will remove this code.
| import org.hisp.dhis.common.BaseIdentifiableObject; | ||
| import org.hisp.dhis.dataelement.DataElement; | ||
|
|
||
| public final class DataValueUtil { |
There was a problem hiding this comment.
this class will be removed when the Data Element merge code is updated. It will no longer be required.
| """ | ||
| do | ||
| $$ | ||
| declare |
There was a problem hiding this comment.
The 2 data value SQL scripts are basically the same, but they are tailored for the correct data value field (category option combo & attribute option combo).
It felt wrong to reuse the same script with parameters as the script would then become unreadable (~8 parameters), requiring to scroll to the bottom of the script to see what parameter would go where, losing the ability to read through the script.
dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/test/TestBase.java
Show resolved
Hide resolved
| import org.hisp.dhis.category.CategoryOption; | ||
| import org.hisp.dhis.category.CategoryOptionCombo; | ||
|
|
||
| public record TestCategoryMetadata( |
There was a problem hiding this comment.
Like the record but wonder why you opted to not use List<CategoryCombo> instead of cc1+cc2 and so on. That seems more flexible, future proof and maybe easier to read?
There was a problem hiding this comment.
just went for direct access to a few types as a start. How I would use it v how others would, might be different. It's not just for CategoryCombos. It's primarily to use CategoryOptionCombos. And this makes it easy to use a properly created CategoryOptionCombo. Maybe we can chat about it at the next meeting as well.
| * @param cocs {@link CategoryOptionCombo}s to update | ||
| * @param coc {@link CategoryOptionCombo} to use as the new value | ||
| */ | ||
| void setAttributeOptionCombo(Set<Long> cocs, long coc); |
There was a problem hiding this comment.
IDK much about events but why are we using long IDs here?
There was a problem hiding this comment.
the DB type is bigint which maps to Java long.
This update is done in native SQL, so no COC object is used here. The foreign key ID is used directly.
| @@ -88,4 +90,18 @@ where jsonb_exists_any(e.eventdatavalues, :searchStrings) | |||
| "searchStrings", searchStrings.toArray(String[]::new), StringArrayType.INSTANCE) | |||
| .getResultList(); | |||
| } | |||
|
|
|||
| @Override | |||
| public void setAttributeOptionCombo(Set<Long> cocs, long coc) { | |||
There was a problem hiding this comment.
I think we should stick to update as the verb for this type of thing.
There was a problem hiding this comment.
yep, can update that.
| } | ||
|
|
||
| private void assertWithNewValue( | ||
| @Nonnull DataValue dv, @Nonnull String newValue, @Nonnull String property) { |
There was a problem hiding this comment.
I feel like instead of property this should have been 3 different assert methods if they do a if-else internally anyhow. The shared part then would be another assert method they can call.
There was a problem hiding this comment.
these will be deleted when the data element merge is updated.
|
|
||
| @Test | ||
| @DisplayName("Retrieving CategoryCombos by CategoryOptionCombos returns the expected entries") | ||
| void getCatOptionComboTest() { |
There was a problem hiding this comment.
that setup looks very familiar - isn't that what you did in a helper utility method?
There was a problem hiding this comment.
yes, I need to find places where similar things are done & use the new test method.
|
|
||
| @Test | ||
| @DisplayName("Retrieving CategoryOptions by CategoryOptionCombos returns the expected entries") | ||
| void getCatOptionComboTest() { |
Feature
Adds a new feature, which allows the merging of
CategoryOptionCombos.More details in the Jira issue with full acceptance criteria.
Docs link below also details everything involved in this merge.
Extracted the handling of merging
DataValues into a common handler class as this is used by multiple merge types (DataElement&CategoryOptionCombo).Testing
Automated
Tests added for
Performance
Some performance testing was carried out for types that may have thousands/millions of entries [
dataValue,event].Tests were carried out comparing
JavavSQLversions of data merging for [dataValue,event].Quick summary (results and data further below):
776kDataValues -> completed in3m 40s5 millionDataValues &365kEvents producedOOM ErrorDataValuemerging,5 millionDataValues &365kEvents -> completed in4m 14sDataValue&Eventmerging,5 millionDataValues &365kEvents -> completed in2m 13sDataValue&Eventdeleting,5 millionDataValues &365kEvents -> completed in1m 17sDB: SL DBmachine: macbook pro - m2 pro - 16GBdata:SCENARIO 1
{ "sources": [ "S34ULMcHMca", "wHBMVthqIX4" ], "target": "jOkIbJVhECg", "deleteSources": true, "dataMergeStrategy": "LAST_UPDATED" }Java:
3m 40sSQL (data value):
13sSCENARIO 2
{ "sources": [ "HllvX50cXC0", "iR1xska5MM2" ], "target": "j9PqKr6Djnx", "deleteSources": true, "dataMergeStrategy": "LAST_UPDATED" }Java:
OOM errorSQL (data value):
4m 14sSCENARIO 3
{ "sources": [ "HllvX50cXC0", "iR1xska5MM2" ], "target": "j9PqKr6Djnx", "deleteSources": true, "dataMergeStrategy": "LAST_UPDATED" }SQL (data value & event):
2m 13sSCENARIO 4
{ "sources": [ "HllvX50cXC0", "iR1xska5MM2" ], "target": "j9PqKr6Djnx", "deleteSources": true, "dataMergeStrategy": "DISCARD" }SQL (data value & event):
1m 17sDocs
PR - dhis2/dhis2-docs#1466