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

Re-enable support for external secondary CSV instances #448

Closed
wants to merge 8 commits into from
Closed

Re-enable support for external secondary CSV instances #448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
851db21
only build ESI when used by instance function
musamusa Jun 19, 2019
a6c0780
refactored secondary instance analyser
musamusa Jun 20, 2019
dbd9625
replaced local method with javarosa utils method
musamusa Jun 23, 2019
c197934
moved object init to initState method
musamusa Jun 23, 2019
9a974f5
removed unused method and imports
musamusa Jun 24, 2019
4d21b23
added test case
musamusa Jun 24, 2019
629a93c
updated test form
musamusa Jun 25, 2019
799be2e
Merge pull request #8 from eHealthAfrica/OED-2
Jun 26, 2019
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
14 changes: 10 additions & 4 deletions src/main/java/org/javarosa/xform/parse/XFormParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import org.javarosa.core.util.externalizable.PrototypeFactoryDeprecated;
import org.javarosa.model.xform.XPathReference;
import org.javarosa.xform.util.InterningKXmlParser;
import org.javarosa.xform.util.SecondaryInstanceAnalyzer;
import org.javarosa.xform.util.XFormAnswerDataParser;
import org.javarosa.xform.util.XFormSerializer;
import org.javarosa.xform.util.XFormUtils;
Expand Down Expand Up @@ -172,6 +173,7 @@ public class XFormParser implements IXFormParserFunctions {

private final List<WarningCallback> warningCallbacks = new ArrayList<>();
private final List<ErrorCallback> errorCallbacks = new ArrayList<>();
private SecondaryInstanceAnalyzer secondaryInstanceAnalyzer = new SecondaryInstanceAnalyzer();

//incremented to provide unique question ID for each question
private int serialQuestionID = 1;
Expand Down Expand Up @@ -320,6 +322,7 @@ private void initState() {
mainInstanceNode = null;
instanceNodes = new ArrayList<>();
instanceNodeIdStrs = new ArrayList<>();
secondaryInstanceAnalyzer = new SecondaryInstanceAnalyzer();

itextKnownForms = new ArrayList<>(4);
itextKnownForms.add("long");
Expand Down Expand Up @@ -471,9 +474,8 @@ private void parseDoc(Map<String, String> namespacePrefixesByUri, String lastSav
final String instanceId = instanceNodeIdStrs.get(instanceIndex);
final String instanceSrc = parseInstanceSrc(instance, lastSavedSrc);

// Disable jr://file-csv/ support by explicitly only supporting jr://file/
// until https://github.com/opendatakit/javarosa/issues/417 is addressed
if (instanceSrc != null && instanceSrc.toLowerCase().startsWith("jr://file/")) {
// only build when ESI Id is not referenced in pulldata
Copy link
Member

Choose a reason for hiding this comment

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

SecondaryInstanceAnalyzer doesn't do anything relating to pulldata. A more accurate comment might be something like "Only load an external secondary instance file if its ID is used as a parameter to the instance function"

if (instanceSrc != null && instanceSrc.toLowerCase().startsWith("jr://file") && secondaryInstanceAnalyzer.shouldSecondaryInstanceBeParsed(instanceId)) {
Copy link
Member

@lognaturel lognaturel Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This will always be false because the primary instance is loaded after the secondary instances are, right? Doesn't analyzeElement always happen after this has already been done?

final ExternalDataInstance externalDataInstance;
try {
externalDataInstance = ExternalDataInstance.build(instanceSrc, instanceId);
Expand All @@ -493,6 +495,7 @@ private void parseDoc(Map<String, String> namespacePrefixesByUri, String lastSav
}
}
}

//now parse the main instance
if (mainInstanceNode != null) {
FormInstance fi = instanceParser.parseInstance(mainInstanceNode, true,
Expand Down Expand Up @@ -637,6 +640,9 @@ private void parseModel(Element e) {
Element child = (type == Node.ELEMENT ? e.getElement(i) : null);
String childName = (child != null ? child.getName() : null);

if (child != null)
secondaryInstanceAnalyzer.analyzeElement(child);

if ("itext".equals(childName)) {
parseIText(child);
} else if ("instance".equals(childName)) {
Expand Down Expand Up @@ -975,6 +981,7 @@ private QuestionDef parseControl(IFormElement parent, Element e, int controlType

String ref = e.getAttributeValue(null, REF_ATTR);
String bind = e.getAttributeValue(null, BIND_ATTR);
secondaryInstanceAnalyzer.analyzeElement(e);

if (bind != null) {
DataBinding binding = bindingsByID.get(bind);
Expand Down Expand Up @@ -1855,7 +1862,6 @@ private void parseBind(Element element) {

private void addBinding(DataBinding binding) {
bindings.add(binding);

if (binding.getId() != null) {
if (bindingsByID.put(binding.getId(), binding) != null) {
throw new XFormParseException("XForm Parse: <bind>s with duplicate ID: '" + binding.getId() + "'");
Expand Down
41 changes: 41 additions & 0 deletions src/main/java/org/javarosa/xform/util/SecondaryInstanceAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package org.javarosa.xform.util;

import org.kxml2.kdom.Element;

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

import static org.javarosa.xform.util.XFormSerializer.elementToString;


public class SecondaryInstanceAnalyzer {
// a list of SIs that need to be built in memory
private final List<String> inMemorySecondaryInstances;

private final Pattern INSTANCE_FUNCTION_PATTERN = Pattern.compile("instance\\s*\\(\\s*'([^\\s]{1,64})'\\s*");
Copy link
Member

Choose a reason for hiding this comment

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

This regular expression should have some corresponding tests to document its intended behavior. A comment would help too (white space is allowed after the function name, before the argument, etc). In particular, there should be an explanation for the 64 character limit.

Also note that double quotes are allowed for strings as well as single quotes.


public SecondaryInstanceAnalyzer() {
inMemorySecondaryInstances = new ArrayList<>();
}

public void analyzeElement(Element element) {
String elementTagString = elementToString(element);
if (elementTagString == null)
return;

Matcher matcher = INSTANCE_FUNCTION_PATTERN.matcher(elementTagString);
String functionFirstParam = matcher.find() ? matcher.group(1) : null;
if (functionFirstParam != null && !inMemorySecondaryInstances.contains(functionFirstParam))
inMemorySecondaryInstances.add(functionFirstParam);
}

public List<String> getInMemorySecondaryInstances() {
return inMemorySecondaryInstances;
}

public boolean shouldSecondaryInstanceBeParsed(String instanceId) {
return inMemorySecondaryInstances.contains(instanceId);
}
}
15 changes: 15 additions & 0 deletions src/test/java/org/javarosa/xform/parse/XFormParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import java.io.BufferedReader;
import java.io.DataInputStream;
Expand All @@ -30,6 +31,7 @@
import java.text.NumberFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.List;
import org.javarosa.core.model.CoreModelModule;
import org.javarosa.core.model.FormDef;
Expand Down Expand Up @@ -130,6 +132,19 @@ public void parsesPreloadForm() throws IOException {
assertEquals("Sample Form - Preloading", formDef.getTitle());
}

@Test
public void parsesExternalSecondaryInstanceAsFormInstance() throws IOException {
Path form = r("Sample-Preloading.xml");
Copy link
Member

Choose a reason for hiding this comment

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

It would be ideal to have a test that just isolates the case that is trying to be handled here. For example, a very narrow one could define an external CSV instance that is not referred to in an instance() function. A slightly broader test form would have one that is and one that isn't so both of those cases can be verified.

setUpSimpleReferenceManager("file-csv", form.getParent());
FormDef formDef = parse(form);
Enumeration<DataInstance> elements = formDef.getNonMainInstances();

while (elements.hasMoreElements()) {
DataInstance instance = elements.nextElement();
assertTrue(instance instanceof FormInstance);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the opposite of what is desired but perhaps some comments would help clarify. In that form, an instance is declared with <instance id="hhplotdetails" src="jr://file-csv/hhplotdetails.csv">. However, it's never referred to as instance('hhplotdetails') so the file should not be loaded as an in-memory instance.

}
}

@Test
public void parsesSecondaryInstanceForm() throws IOException {
FormDef formDef = parse(SECONDARY_INSTANCE_XML);
Expand Down
43 changes: 43 additions & 0 deletions src/test/java/org/javarosa/xform/util/test/SecondaryInstanceAnalyzerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.javarosa.xform.util.test;

import org.javarosa.xform.parse.XFormParser;
import org.javarosa.xform.util.SecondaryInstanceAnalyzer;
import org.junit.Test;
import org.kxml2.kdom.Document;
import org.kxml2.kdom.Element;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.FileReader;
import java.io.IOException;

import static org.javarosa.test.utils.ResourcePathHelper.r;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertEquals;

public class SecondaryInstanceAnalyzerTest {
private static final Logger logger = LoggerFactory.getLogger(SecondaryInstanceAnalyzer.class);

private void parseElement(Element element, SecondaryInstanceAnalyzer secondaryInstanceAnalyzer) {
for (int i = 0; i < element.getChildCount(); i++) {
if (element.getType(i) == Element.ELEMENT) {
secondaryInstanceAnalyzer.analyzeElement(element);
parseElement(element.getElement(i), secondaryInstanceAnalyzer);
}
}
}

@Test
public void getIDFromAttributes() throws IOException {
String filePath = r("secondary-instance-test.xml").toString();
Document doc = XFormParser.getXMLDocument(new FileReader(filePath));
Element root = doc.getRootElement();
SecondaryInstanceAnalyzer secondaryInstanceAnalyzer = new SecondaryInstanceAnalyzer();
parseElement(root, secondaryInstanceAnalyzer);

assertEquals(secondaryInstanceAnalyzer.getInMemorySecondaryInstances().toArray().length, 2);
assertFalse(secondaryInstanceAnalyzer.shouldSecondaryInstanceBeParsed("country"));
assertTrue(secondaryInstanceAnalyzer.shouldSecondaryInstanceBeParsed("lgas"));
}
}
Loading