From 7f24b390bb26b7902ad7d4555ec8e5b4497c5a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20S=C3=A9nave?= <59770457+nsenave@users.noreply.github.com> Date: Fri, 19 Jan 2024 15:21:42 +0100 Subject: [PATCH] fix: lunatic shape from (#869) * refactor(ddi): add loop references in variable groups mapping * refactor(loops): make some methods re-usable * fix: lunatic shape from property The previous implementation returned the the first variable of the corresponding variable group. Yet the order of variables in a variable group is not strictly enforced in current DDI specification. So this information has to be retrieved another way. * test(shape-from): update unit tests * chore: version 3.15.8 --- .github/workflows/sonar.yml | 3 + build.gradle | 2 +- .../core/model/variable/VariableGroup.java | 32 +++++++ .../steps/lunatic/LunaticLoopResolution.java | 59 +++++++++---- ...mAttributeRetrievalFromVariableGroups.java | 85 +++++++++++++++++-- ...ributeRetrievalFromVariableGroupsTest.java | 53 ++++++++++-- 6 files changed, 204 insertions(+), 30 deletions(-) diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 87844eefd..a6825fd91 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -12,6 +12,9 @@ on: - 'README**.md' - 'Dockerfile' - '.github/**' + - '/build.gradle' + # temporary exclude root build.gradle file from test workflow trigger + # todo: externailze the project version somewhere so to not trigger tests for nothing when only project version has changed jobs: build: diff --git a/build.gradle b/build.gradle index a075276cc..d7e22b726 100644 --- a/build.gradle +++ b/build.gradle @@ -16,7 +16,7 @@ tasks.withType(JavaCompile).configureEach { allprojects { group = 'fr.insee.eno' - version = '3.15.7' + version = '3.15.8' } subprojects { diff --git a/eno-core/src/main/java/fr/insee/eno/core/model/variable/VariableGroup.java b/eno-core/src/main/java/fr/insee/eno/core/model/variable/VariableGroup.java index 815be76a6..891a867cd 100644 --- a/eno-core/src/main/java/fr/insee/eno/core/model/variable/VariableGroup.java +++ b/eno-core/src/main/java/fr/insee/eno/core/model/variable/VariableGroup.java @@ -1,18 +1,32 @@ package fr.insee.eno.core.model.variable; +import fr.insee.eno.core.annotations.Contexts.Context; import fr.insee.eno.core.annotations.DDI; import fr.insee.eno.core.model.EnoObject; +import fr.insee.eno.core.parameter.Format; +import logicalproduct33.VariableGroupType; import lombok.Getter; import lombok.Setter; +import reusable33.ReferenceType; import java.util.ArrayList; import java.util.List; import java.util.Optional; +/** + * A variable group contains all variables that share a common "scope". For collected variables, this means variables + * that are collected in a loop or a dynamic table, and loops that are linked. Example: loop "A" contains variables + * "foo" and "bar", loop "B" is linked to loop "A" and contains the variable "gus", then these two loops corresponds + * to a single variable group that contains variables "foo", "bar" and "gus". Calculated and external are defined at + * a certain level (i.e. scope) in Pogues that corresponds to questionnaire-level, or a main loop, or a dynamic table. + */ @Getter @Setter +@Context(format = Format.DDI, type = VariableGroupType.class) public class VariableGroup extends EnoObject { + public static final String DDI_QUESTIONNAIRE_TYPE = "Questionnaire"; + @DDI("!getVariableGroupNameList().isEmpty ? getVariableGroupNameArray(0).getStringArray(0).getStringValue() : null") private String name; @@ -22,9 +36,27 @@ public class VariableGroup extends EnoObject { @DDI("getVariableReferenceList().![#index.get(#this.getIDArray(0).getStringValue())]") private final List variables = new ArrayList<>(); + /** Ordered list of iterable (i.e. loop or dynamic table) identifiers that correspond to this variable group. + * The first reference is either a "main" loop or a dynamic table, the others are linked loops. + * If the variable group corresponds to questionnaire-level variables, this list is empty. */ + @DDI("T(fr.insee.eno.core.model.variable.VariableGroup).getDDILoopReferences(#this)") + private final List loopReferences = new ArrayList<>(); + public Optional getVariableByName(String variableName) { return variables.stream() .filter(variable -> variable.getName().equals(variableName)) .findFirst(); } + + public static List getDDILoopReferences(VariableGroupType ddiVariableGroup) { + if (DDI_QUESTIONNAIRE_TYPE.equals(ddiVariableGroup.getTypeOfVariableGroup().getStringValue())) + return new ArrayList<>(); + // iterable means either loop or dynamic table + List iterableReferences = new ArrayList<>(); + for (ReferenceType reference : ddiVariableGroup.getBasedOnObject().getBasedOnReferenceList()) { + iterableReferences.add(reference.getIDArray(0).getStringValue()); + } + return iterableReferences; + } + } diff --git a/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/LunaticLoopResolution.java b/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/LunaticLoopResolution.java index bdde69524..7be568953 100644 --- a/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/LunaticLoopResolution.java +++ b/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/LunaticLoopResolution.java @@ -20,6 +20,7 @@ import java.math.BigInteger; import java.util.Iterator; import java.util.List; +import java.util.Optional; /** Lunatic technical processing for loops. * Requires: sorted components, hierarchy. */ @@ -175,25 +176,51 @@ private void setLinkedLoopIterations(Loop lunaticLoop, LinkedLoop enoLinkedLoop) enoLinkedLoop.getId(), reference)); } + /** + * Returns the first response name that belong to the reference loop. + * @param enoLinkedLoop Eno linked loop passed for logging purposes. + * @param enoReferenceLoop Eno reference ("main") loop. + * @param enoIndex Eno index. + * @return The first response name (string) that belong to the reference loop. + */ public static String findFirstVariableOfReference(LinkedLoop enoLinkedLoop, StandaloneLoop enoReferenceLoop, - EnoIndex enoIndex) { - AbstractSequence startSequence = (AbstractSequence) enoIndex.get(enoReferenceLoop.getLoopScope().get(0).getId()); - if (startSequence.getSequenceStructure().isEmpty()) { + EnoIndex enoIndex) { + String contextErrorMessage = String.format( + "Unable to find its first question to compute Lunatic \"iterations\" expression for linked loop '%s'.", + enoLinkedLoop.getId()); + return findFirstResponseNameOfLoop(enoReferenceLoop, enoIndex, contextErrorMessage); + } + + /** + * Returns the first response name that belong to the loop. + * @param enoLoop A eno loop object. + * @param enoIndex Eno index. + * @param contextErrorMessage Context that will be added in the exception message if retrieving the response name fails. + * @return The first response name (string) that belong to the loop. + */ + public static String findFirstResponseNameOfLoop(fr.insee.eno.core.model.navigation.Loop enoLoop, + EnoIndex enoIndex, + String contextErrorMessage) { + AbstractSequence firstSequenceOfLoop = (AbstractSequence) enoIndex.get(enoLoop.getLoopScope().get(0).getId()); + if (firstSequenceOfLoop.getSequenceStructure().isEmpty()) throw new LunaticLoopException(String.format( - "Linked loop '%s' is based on loop '%s'. " + - "This loop is defined to start at sequence '%s', which is empty. " + - "Unable to find its first question to compute Lunatic \"iterations\" expression.", - enoLinkedLoop.getId(), enoReferenceLoop.getId(), startSequence.getId())); - } - String firstQuestionId = findFirstQuestionId(startSequence, enoIndex); - EnoObject firstQuestion = enoIndex.get(firstQuestionId); - if (! (firstQuestion instanceof SingleResponseQuestion)) { + "Loop '%s' is defined to start at sequence '%s', which is empty. %s", + enoLoop.getId(), firstSequenceOfLoop.getId(), contextErrorMessage)); + String firstQuestionId = findFirstQuestionId(firstSequenceOfLoop, enoIndex); + Optional variableName = getVariableNameFromQuestionId(firstQuestionId, enoIndex); + if (variableName.isEmpty()) throw new LunaticLoopException(String.format( - "Linked loop '%s' is based on loop '%s' that starts at sequence '%s'. " + - "This first question of the sequence is not a \"simple\" question.", - enoLinkedLoop.getId(), enoReferenceLoop.getId(), startSequence.getId())); - } - return ((SingleResponseQuestion) firstQuestion).getResponse().getVariableName(); + "Loop '%s' that starts at sequence '%s'. " + + "This first question of the sequence is not a \"simple\" question. %s", + enoLoop.getId(), firstSequenceOfLoop.getId(), contextErrorMessage)); + return variableName.get(); + } + + private static Optional getVariableNameFromQuestionId(String questionId, EnoIndex enoIndex) { + EnoObject firstQuestion = enoIndex.get(questionId); + if (! (firstQuestion instanceof SingleResponseQuestion)) + return Optional.empty(); + return Optional.of(((SingleResponseQuestion) firstQuestion).getResponse().getVariableName()); } /** diff --git a/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroups.java b/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroups.java index b600334a1..99984ca6f 100644 --- a/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroups.java +++ b/eno-core/src/main/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroups.java @@ -1,8 +1,13 @@ package fr.insee.eno.core.processing.out.steps.lunatic.calculatedvariable; +import fr.insee.eno.core.exceptions.technical.MappingException; import fr.insee.eno.core.model.EnoQuestionnaire; +import fr.insee.eno.core.model.navigation.Loop; +import fr.insee.eno.core.model.question.DynamicTableQuestion; import fr.insee.eno.core.model.variable.Variable; import fr.insee.eno.core.model.variable.VariableGroup; +import fr.insee.eno.core.processing.out.steps.lunatic.LunaticLoopResolution; +import fr.insee.eno.core.reference.EnoIndex; import java.util.Optional; @@ -14,23 +19,87 @@ public class ShapefromAttributeRetrievalFromVariableGroups implements ShapefromAttributeRetrieval { /** - * + * Retrieves the Lunatic "shapeFrom" attribute. It is empty if the calculated variable with given name is not in a + * loop. Otherwise, it is the name of the first collected variable that is in the scope of the corresponding loop. + * This method uses the Eno questionnaire to retrieve this information. * @param lunaticCalculatedVariableName name of the calculated variable * @param enoQuestionnaire eno questionnaire * @return the first collected variable if the calculated variable is in a loop, nothing otherwise */ public Optional getShapeFrom(String lunaticCalculatedVariableName, EnoQuestionnaire enoQuestionnaire) { + EnoIndex enoIndex = enoQuestionnaire.getIndex(); + return getShapeFrom(lunaticCalculatedVariableName, enoQuestionnaire, enoIndex); + } + + // Note: the pattern with eno questionnaire and eno index is not satisfying as it currently is. + // It would probably better to separate the two. + // The first method is there so to not change the signature of the "shape from retrieval" interface for now. + + // Note 2: using the eno index as little as possible to improve code maintainability in exchange + // for a slight loss of performance. + + private Optional getShapeFrom(String lunaticCalculatedVariableName, EnoQuestionnaire enoQuestionnaire, + EnoIndex enoIndex) { // retrieve the variable group in which the variable is present, excluding the "Questionnaire" - // variable group which does not corresponds to a "loop" variable group + // variable group which does not correspond to a "loop" variable group Optional variableGroup = enoQuestionnaire.getVariableGroups().stream() - .filter(vGroup -> !vGroup.getType().equals("Questionnaire")) + .filter(vGroup -> !vGroup.getType().equals(VariableGroup.DDI_QUESTIONNAIRE_TYPE)) .filter(vGroup -> vGroup.getVariableByName(lunaticCalculatedVariableName).isPresent()) .findFirst(); - return variableGroup.flatMap( - vGroup -> vGroup.getVariables().stream() - .filter(variable -> variable.getCollectionType().equals(Variable.CollectionType.COLLECTED)) - .findFirst() - ); + if (variableGroup.isEmpty()) + return Optional.empty(); + + // then retrieve the "main" loop or dynamic table that corresponds to this variable group + String mainIterableReference = variableGroup.get().getLoopReferences().get(0); + + // loop case + Optional mainLoop = enoQuestionnaire.getLoops().stream() + .filter(loop -> mainIterableReference.equals(loop.getId())) + .findAny(); + if (mainLoop.isPresent()) + return Optional.of(findFirstCollectedVariableOfLoop(mainLoop.get(), enoQuestionnaire, enoIndex)); + + // dynamic table case + Optional dynamicTable = enoQuestionnaire.getMultipleResponseQuestions().stream() + .filter(DynamicTableQuestion.class::isInstance) + .map(DynamicTableQuestion.class::cast) + .filter(loop -> mainIterableReference.equals(loop.getId())) + .findAny(); + if (dynamicTable.isPresent()) + return Optional.of(findFirstCollectedVariableOfDynamicTable(dynamicTable.get(), enoQuestionnaire)); + + throw new MappingException(String.format( + "Unable to retrieve 'shapeFrom' property for the calculated variable '%s'. " + + "This probably means that some information has not been mapped as expected during mapping.", + lunaticCalculatedVariableName)); } + + // Note: not sure why this method returns a Variable object and not a CollectedVariable, maybe it could be improved + + private Variable findFirstCollectedVariableOfLoop(Loop enoLoop, EnoQuestionnaire enoQuestionnaire, EnoIndex enoIndex) { + String contextErrorMessage = "Unable to find its first question to retrieve Lunatic \"shapeFrom\" property."; + String firstQuestionVariableName = LunaticLoopResolution.findFirstResponseNameOfLoop( + enoLoop, enoIndex, contextErrorMessage); + return getVariableFromName(enoQuestionnaire, firstQuestionVariableName); + } + + private Variable findFirstCollectedVariableOfDynamicTable(DynamicTableQuestion enoDynamicTable, + EnoQuestionnaire enoQuestionnaire) { + String variableName = enoDynamicTable.getVariableNames().get(0); + return getVariableFromName(enoQuestionnaire, variableName); + } + + private static Variable getVariableFromName(EnoQuestionnaire enoQuestionnaire, String variableName) { + Optional searchedVariable = enoQuestionnaire.getVariables() + .stream() + .filter(variable -> variableName.equals(variable.getName())).findAny(); + if (searchedVariable.isEmpty()) + throw new MappingException(String.format( + "Unable to find variable with name '%s' in the Eno questionnaire variables " + + "(while trying to retrieve Lunatic 'shapeFrom' property).", + variableName)); + return searchedVariable.get(); + } + } diff --git a/eno-core/src/test/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroupsTest.java b/eno-core/src/test/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroupsTest.java index 3f08e3a02..37b98dfbb 100644 --- a/eno-core/src/test/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroupsTest.java +++ b/eno-core/src/test/java/fr/insee/eno/core/processing/out/steps/lunatic/calculatedvariable/ShapefromAttributeRetrievalFromVariableGroupsTest.java @@ -1,10 +1,17 @@ package fr.insee.eno.core.processing.out.steps.lunatic.calculatedvariable; import fr.insee.eno.core.model.EnoQuestionnaire; +import fr.insee.eno.core.model.navigation.StandaloneLoop; +import fr.insee.eno.core.model.question.DynamicTableQuestion; +import fr.insee.eno.core.model.question.TextQuestion; +import fr.insee.eno.core.model.response.Response; +import fr.insee.eno.core.model.sequence.Sequence; +import fr.insee.eno.core.model.sequence.StructureItemReference; import fr.insee.eno.core.model.variable.CalculatedVariable; import fr.insee.eno.core.model.variable.CollectedVariable; import fr.insee.eno.core.model.variable.Variable; import fr.insee.eno.core.model.variable.VariableGroup; +import fr.insee.eno.core.reference.EnoIndex; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -28,9 +35,34 @@ class ShapefromAttributeRetrievalFromVariableGroupsTest { void init() { shapefromAttributeRetrieval = new ShapefromAttributeRetrievalFromVariableGroups(); enoQuestionnaire = new EnoQuestionnaire(); + EnoIndex enoIndex = new EnoIndex(); + + StandaloneLoop loop = new StandaloneLoop(); + loop.setId("loop-id"); + loop.getLoopScope().add(StructureItemReference.builder() + .id("sequence-id") + .type(StructureItemReference.StructureItemType.SEQUENCE) + .build()); + Sequence sequence = new Sequence(); + sequence.setId("sequence-id"); + sequence.getSequenceStructure().add(StructureItemReference.builder() + .id("question-id") + .type(StructureItemReference.StructureItemType.QUESTION) + .build()); + TextQuestion textQuestion = new TextQuestion(); + textQuestion.setId("question-id"); + textQuestion.setResponse(new Response()); + textQuestion.getResponse().setVariableName("variable2"); + + enoIndex.put("sequence-id", sequence); + enoIndex.put("question-id", textQuestion); + enoQuestionnaire.setIndex(enoIndex); + enoQuestionnaire.getLoops().add(loop); variableGroup1 = new VariableGroup(); variableGroup1.setType("Loop"); + variableGroup1.getLoopReferences().addAll(List.of("loop-id", "linked-loop-id")); + // (the linked loop reference should be ignored as only the "main" loop counts for retrieving the 'shapeFrom') variable1 = new CalculatedVariable(); variable1.setName("variable1"); variable2 = new CollectedVariable(); @@ -43,14 +75,21 @@ void init() { variable3.setName("variable3"); variableGroup2.getVariables().add(variable3); + DynamicTableQuestion dynamicTableQuestion = new DynamicTableQuestion(); + dynamicTableQuestion.setId("dynamic-table-id"); + dynamicTableQuestion.getVariableNames().add("variable4"); + enoQuestionnaire.getMultipleResponseQuestions().add(dynamicTableQuestion); + variableGroup3 = new VariableGroup(); variableGroup3.setType("Loop"); + variableGroup3.getLoopReferences().add("dynamic-table-id"); variable4 = new CollectedVariable(); variable4.setName("variable4"); variable5 = new CalculatedVariable(); variable5.setName("variable5"); variableGroup3.getVariables().addAll(List.of(variable4, variable5)); + enoQuestionnaire.getVariables().addAll(List.of(variable1, variable2, variable3, variable4, variable5)); enoQuestionnaire.getVariableGroups().addAll(List.of(variableGroup1, variableGroup2, variableGroup3)); } @@ -67,13 +106,17 @@ void whenVariableNotExistingReturnEmpty() { } @Test - void whenVariableInLoopVariableGroupReturnFirstCollectedVariable() { - Optional variable = shapefromAttributeRetrieval.getShapeFrom("variable5", enoQuestionnaire); + void whenVariableInLoopVariableGroup_returnFirstCollectedVariable() { + Optional variable = shapefromAttributeRetrieval.getShapeFrom("variable1", enoQuestionnaire); assertTrue(variable.isPresent()); - assertEquals("variable4", variable.get().getName()); + assertEquals("variable2", variable.get().getName()); + } - variable = shapefromAttributeRetrieval.getShapeFrom("variable1", enoQuestionnaire); + @Test + void whenVariableInDynamicTableVariableGroup_returnFirstCollectedVariable() { + Optional variable = shapefromAttributeRetrieval.getShapeFrom("variable5", enoQuestionnaire); assertTrue(variable.isPresent()); - assertEquals("variable2", variable.get().getName()); + assertEquals("variable4", variable.get().getName()); } + }