Skip to content

Sequential parser external loops fix #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loichenninger merged 2 commits into from
Jan 31, 2025
Merged

Sequential parser external loops fix #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63a9a54
test: Add sequential external variable loops parsing fail test
alexisszmundy Jan 22, 2025
421c9c8
fix: Fixed sequential parser crashing when external variable loop pre…
alexisszmundy Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/main/java/fr/insee/genesis/controller/sources/xml/LunaticXmlDataSequentialParser.java
View file
Open in desktop
Copy link

@nsenave nsenave Jan 30, 2025

Choose a reason for hiding this comment

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

Un peu étonné de voir du parsing xml dans le package controller mais c'est juste une opinion.

Par conte je pense que ça serait intéressant de basculer sur une dé-sérialisation basée sur un modèle objet, avec jackson par exemple ; ça fait écrire moins de code "bas niveau" sur les noeuds xml. Et même dans l'idéal avoir cette partie externalisée dans la lib Lunatic-Model

Copy link

@nsenave nsenave Jan 31, 2025

Choose a reason for hiding this comment

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

Après discussion avec @loichenninger il y a bien nécessité d'écrire un peu de code plus bas niveau que juste de la désérialisation de tout le contenu d'un coup. On verra à l'occasion si on peut faire ça avec les fonctions de l'api jackson (➡️ voir si le changement est suffisamment simple à faire pour que ça vaille le coup) à la place de la "vielle" api javax.* (qui sera peut-être dépréciée dans une future version de java)

Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,21 @@ private static Attribute getType(StartElement element) {
return element.getAttributeByName(new QName("type"));
}

private List<ValueType> readValues(XMLEventReader reader, String stateName) throws XMLStreamException {
/**
* Read multiple repeated values in XML (notably when reading loop data)
* @param reader the XML reader used to read XML
* @param elementName the repeated element name
*
* @return a list of ValueType for a specific element
* @throws XMLStreamException if XML reading error
*/
private List<ValueType> readValues(XMLEventReader reader, String elementName) throws XMLStreamException {
List<ValueType> values = new ArrayList<>();

while(reader.hasNext()){
final XMLEvent event = reader.nextEvent();

if (event.isEndElement() && event.asEndElement().getName().getLocalPart().equals(stateName)){
if (event.isEndElement() && event.asEndElement().getName().getLocalPart().equals(elementName)){
// End of variable
return values;
}
Expand All @@ -223,7 +231,6 @@ private List<ValueType> readValues(XMLEventReader reader, String stateName) thro
values.add(new ValueType(value, type));
}
}

return values;
}

Expand All @@ -246,13 +253,21 @@ private List<LunaticXmlOtherData> readCalculatedOrExternal(XMLEventReader reader
LunaticXmlOtherData variable = new LunaticXmlOtherData();
variable.setVariableName(variableName);

String type = getType(element).getValue();
String value = reader.getElementText();

//Read values
List<ValueType> values = new ArrayList<>();
values.add(new ValueType(value, type));
variable.setValues(values);
if(element.isStartElement() && getType(element) != null && !getType(element).getValue().equals("null")) {
//If only 1 value (determined by presence of type)
String type = getType(element).getValue();
String value = reader.getElementText();

values.add(new ValueType(value, type));
variable.setValues(values);
lunaticXmlOtherDataList.add(variable);
continue; //Go to next XML event
}
// Multiple values (loop)
values = readValues(reader, variableName);
variable.setValues(values);
lunaticXmlOtherDataList.add(variable);
}
}
Expand Down
42 changes: 36 additions & 6 deletions ...test/java/fr/insee/genesis/controller/sources/xml/LunaticXmlDataSequentialParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package fr.insee.genesis.controller.sources.xml;

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import javax.xml.stream.XMLStreamException;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -20,8 +21,8 @@ class LunaticXmlDataSequentialParserTest {
static LunaticXmlSurveyUnit surveyUnit;

// Given + When
@BeforeAll
static void setUp() throws Exception {
@BeforeEach
void setUp() throws Exception {
Path path = Path.of("src/test/resources/data_test_parser_xml.xml");
stream = new FileInputStream(path.toFile());
parser = new LunaticXmlDataSequentialParser(path, stream);
Expand Down Expand Up @@ -81,8 +82,37 @@ void checkExternalVariableValue(){
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().getFirst().getValue()).isEqualTo("BOB");
}

@AfterAll
static void closeStream() throws IOException {
@Test
void checkExternalVariableValue_loops() throws IOException, XMLStreamException {
//Given
Path path = Path.of("src/test/resources/data_test_parser_xml_external_loops.xml");
stream = new FileInputStream(path.toFile());
parser = new LunaticXmlDataSequentialParser(path, stream);

//When
campaign = parser.getCampaign();
surveyUnit = parser.readNextSurveyUnit();

//Then
//Not null & volumetry
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst()).isNotNull();
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues()).isNotEmpty();
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues()).hasSize(3);

//Content
//1
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().getFirst().getType()).isEqualTo("string");
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().getFirst().getValue()).isEqualTo("BOB1");
//2
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().get(1).getType()).isEqualTo("string");
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().get(1).getValue()).isEqualTo("BOB2");
//3
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().get(2).getType()).isEqualTo("string");
Assertions.assertThat(surveyUnit.getData().getExternal().getFirst().getValues().get(2).getValue()).isEqualTo("BOB3");
}

@AfterEach
void closeStream() throws IOException {
stream.close();
}

Expand Down
Loading
Loading