Skip to content
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

kie-issues#1807: Test Scenario version attribute is not correctly retrieved #6254

Merged
merged 5 commits into from
Feb 17, 2025
Merged

kie-issues#1807: Test Scenario version attribute is not correctly retrieved #6254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f613e52
Fix + Tests
yesamer Feb 7, 2025
4080323
Fix + Tests1
yesamer Feb 10, 2025
984fcfb
Fix + Tests1
yesamer Feb 13, 2025
27f95a8
Fix
yesamer Feb 13, 2025
fc9cb19
Tests
yesamer Feb 17, 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
1 change: 1 addition & 0 deletions ...simulation-api/src/main/java/org/drools/scenariosimulation/api/utils/ConstantsHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class ConstantsHolder {
public static final String SCENARIO_SIMULATION_MODEL_NODE = "ScenarioSimulationModel";
public static final String SETTINGS_NODE = "settings";
public static final String FACT_MAPPING_VALUE_TYPE_NODE = "factMappingValueType";
public static final String VERSION_ATTRIBUTE = "version";
public static final String NOT_EXPRESSION = "NOT_EXPRESSION";
public static final String EXECUTED = "EXECUTED";
public static final List<String> SETTINGS = Collections.unmodifiableList(Arrays.asList(DMO_SESSION_NODE, "dmnFilePath", "type", "fileName", "kieSession",
Expand Down
21 changes: 9 additions & 12 deletions ...ain/java/org/drools/scenariosimulation/backend/util/ScenarioSimulationXMLPersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
*/
package org.drools.scenariosimulation.backend.util;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

import com.thoughtworks.xstream.XStream;
import com.thoughtworks.xstream.io.xml.DomDriver;
import com.thoughtworks.xstream.security.WildcardTypePermission;
Expand Down Expand Up @@ -48,12 +45,12 @@
import static org.drools.scenariosimulation.api.utils.ConstantsHolder.SETTINGS;
import static org.drools.scenariosimulation.api.utils.ConstantsHolder.SIMULATION_DESCRIPTOR_NODE;
import static org.drools.scenariosimulation.api.utils.ConstantsHolder.SIMULATION_NODE;
import static org.drools.scenariosimulation.api.utils.ConstantsHolder.VERSION_ATTRIBUTE;

public class ScenarioSimulationXMLPersistence {

private static final ScenarioSimulationXMLPersistence INSTANCE = new ScenarioSimulationXMLPersistence();
private static final String CURRENT_VERSION = new ScenarioSimulationModel().getVersion();
private static final Pattern p = Pattern.compile(SCENARIO_SIMULATION_MODEL_NODE + " version=\"([0-9]+\\.[0-9]+)");

private XStream xt;
private MigrationStrategy migrationStrategy = new InMemoryMigrationStrategy();
Expand Down Expand Up @@ -147,7 +144,8 @@ public ScenarioSimulationModel unmarshal(final String rawXml, boolean migrate) t
}

public String migrateIfNecessary(String rawXml) throws Exception {
String fileVersion = extractVersion(rawXml);
Document document = DOMParserUtil.getDocument(rawXml);
String fileVersion = extractVersion(document);
ThrowingConsumer<Document> migrator = getMigrationStrategy().start();
boolean supported;
switch (fileVersion) {
Expand Down Expand Up @@ -179,18 +177,17 @@ public String migrateIfNecessary(String rawXml) throws Exception {
.append(CURRENT_VERSION).toString());
}
migrator = migrator.andThen(getMigrationStrategy().end());
Document document = DOMParserUtil.getDocument(rawXml);
migrator.accept(document);
return DOMParserUtil.getString(document);
}

public String extractVersion(String rawXml) {
Matcher m = p.matcher(rawXml);

if (m.find()) {
return m.group(1);
public String extractVersion(Document document) {
try {
return document.getElementsByTagName(SCENARIO_SIMULATION_MODEL_NODE)
.item(0).getAttributes().getNamedItem(VERSION_ATTRIBUTE).getTextContent();
} catch (Exception e) {
throw new IllegalArgumentException("Impossible to extract version from the file", e);
}
Comment on lines +184 to 190
Copy link
Contributor

Choose a reason for hiding this comment

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

My only point is only about catching an exception and immediately throwing new one. I believe we could use this approach to avoid try catch approach.

https://medium.com/@gurkanucar/how-to-prevent-null-pointer-exceptions-in-java-8d7c894c9bb4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @jomarko, I would argue that the try-catch is present in the approach you shared, It just moved out in another method. In this case, I don't see the need for that, it would create an unnecessary indirection.

Copy link
Contributor

Choose a reason for hiding this comment

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

then, I have nothing to share for this PR

throw new IllegalArgumentException("Impossible to extract version from the file");
}

public MigrationStrategy getMigrationStrategy() {
Expand Down
97 changes: 86 additions & 11 deletions ...java/org/drools/scenariosimulation/backend/util/ScenarioSimulationXMLPersistenceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.drools.scenariosimulation.backend.util;

import java.io.IOException;
import java.util.List;
import java.util.Map;

Expand All @@ -26,6 +27,9 @@
import org.junit.Test;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.xml.sax.SAXException;

import javax.xml.parsers.ParserConfigurationException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand Down Expand Up @@ -349,24 +353,95 @@ public void migrateIfNecessary() throws Exception {
}

@Test
public void extractVersion() {
String version = instance.extractVersion("<ScenarioSimulationModel version=\"1.0\" version=\"1.1\">");
assertThat(version).isEqualTo("1.0");
public void extractVersionNullDocument() {
assertThatThrownBy(() -> instance.extractVersion(null))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void extractVersionMissingAttribute() {
assertThatThrownBy(() -> instance.extractVersion(DOMParserUtil.getDocument("<ScenarioSimulationModel />")))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void extractVersionWhenXmlPrologIsPresent() {
String version = instance.extractVersion("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.1\">");
public void extractVersionMissingVersionAttribute_prolog() {
assertThatThrownBy(() -> instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel />")))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void extractVersionMissingVersionAttribute_prolog_attribute() {
assertThatThrownBy(() -> instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel xmlns=\"whatever\" />")))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void extractVersionWithVersionAttribute() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<ScenarioSimulationModel version=\"1.1\" />"));
assertThat(version).isEqualTo("1.1");
}

@Test
public void extractVersionWhenMoreVersionAttributesArePresent() {
String version = instance.extractVersion("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.2\">\n" +
"<someUnknownTag version=\"1.1\"/>\n" +
"</ScenarioSimulationModel>");
public void extractVersionWithVersionAttribute_prolog() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.2\" />"));
assertThat(version).isEqualTo("1.2");
}

@Test
public void extractVersionWithVersionAttribute_prolog_attributeRight() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.4\" xmlns=\"whatever\" />"));
assertThat(version).isEqualTo("1.4");
}

@Test
public void extractVersionWithVersionAttribute_prolog_attributeLeft() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel xmlns=\"whatever\" version=\"1.5\" />"));
assertThat(version).isEqualTo("1.5");
}

@Test
public void extractVersionWithVersionAttribute_prolog_attributesMiddle() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel attribute2=\"whatever\" version=\"1.6\" attribute1=\"whatever\" />"));
assertThat(version).isEqualTo("1.6");
}

@Test
public void extractVersionWithVersionAttribute_prolog_attributesRight() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel attribute2=\"whatever\" attribute1=\"whatever\" version=\"1.7\" />"));
assertThat(version).isEqualTo("1.7");
}

@Test
public void extractVersionWithVersionAttribute_prolog_attributesLeft() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.8\" attribute2=\"whatever\" attribute1=\"whatever\" />"));
assertThat(version).isEqualTo("1.8");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using a parameterized test here - and possibly add an explanation string, to explain the different use cases we are testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @pibizza, that was my first intent, but scesim module is still based on JUnit 4, so I preferred to don't introduce Junit4 Parametrized tests in this class. About the explanation string, which approach do you suggest, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yesamer I would suggest something that describe your use case. In Junit 5 you can add a string for a specific test name. In this case add some infos in the name and restructure grouping for cases. So you can have a test where you have one entry (version or attribute), one case where you have two entries (version and attribute, times the various possible cases good or bad) and finally one case where you have three entries (version and two attributes, times the two possible values.) I expect order is not relevant, so it should not be present in the tests. at this point the name of the test could be extractXXX_version, extract XXX_version_attribute, extractXXX_version_twoattributes. You can go for different groupings, if you prefer, but that is the general idea.


@Test
public void extractVersionWhenMoreVersionAttributesArePresent() throws ParserConfigurationException, IOException, SAXException {
String version = instance.extractVersion(DOMParserUtil.getDocument("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<ScenarioSimulationModel version=\"1.2\">\n" +
"<someTag version=\"1.1\"/>\n" +
"</ScenarioSimulationModel>"));
assertThat(version).isEqualTo("1.2");
}

Expand Down