Skip to content

Commit

Permalink
Merge pull request #248 from ukanga/performance-regression-fix
Browse files Browse the repository at this point in the history 
Performance regression fix on using relative paths
  • Loading branch information
@ukanga
ukanga authored Jan 6, 2019
2 parents 464f758 + b499446 commit d40c759
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 45 deletions.
11 changes: 10 additions & 1 deletion CHANGES.txt
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
Pyxform Changelog

v0.12.0, 2018
Unreleased, 2019 01 [xx]
-- Performance Regression fix on using relative paths.
Issue #247, https://github.com/XLSForm/pyxform/issues/247
[ukanga]

-- Additional appearance values are supported with table-list.
Issue #61, https://github.com/XLSForm/pyxform/issues/61
[KeynesYouDigIt]

v0.12.0, 2018 12 09
-- Upgrade to ODK Validate v1.10.3
Issue #225, https://github.com/XLSForm/pyxform/issue/225
[ukanga]
Expand Down
93 changes: 57 additions & 36 deletions pyxform/survey.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
get_languages_with_bad_tags, node, unicode)
from pyxform.validators import enketo_validate, odk_validate

try:
from functools import lru_cache
except ImportError:
from functools32 import lru_cache


def register_nsmap():
"""Function to register NSMAP namespaces with ETree"""
Expand All @@ -33,6 +38,7 @@ def register_nsmap():
register_nsmap()


@lru_cache(maxsize=None)
def is_parent_a_repeat(survey, xpath):
"""
Returns the XPATH of the first repeat of the given xpath in the survey,
Expand All @@ -42,14 +48,13 @@ def is_parent_a_repeat(survey, xpath):
if not parent_xpath:
return False

repeats = [
item for item in survey.iter_descendants()
if item.get_xpath() == parent_xpath and item.type == 'repeat']
if survey.any_repeat(parent_xpath):
return parent_xpath

return parent_xpath \
if any(repeats) else is_parent_a_repeat(survey, parent_xpath)
return is_parent_a_repeat(survey, parent_xpath)


@lru_cache(maxsize=None)
def share_same_repeat_parent(survey, xpath, context_xpath):
"""
Returns a tuple of the number of steps from the context xpath to the shared
Expand Down Expand Up @@ -220,7 +225,8 @@ def _generate_static_instances(list_name, choice_list):
@staticmethod
def _get_dummy_instance():
"""Instance content required by ODK Validate for select inputs."""
return node("root", node("item", node("name", "_"), node("label", "_")))
return node("root",
node("item", node("name", "_"), node("label", "_")))

@staticmethod
def _generate_external_instances(element):
Expand Down Expand Up @@ -340,8 +346,8 @@ def _generate_instances(self):
Validation and business rules for output of instances:
- xml-external item name must be unique across the XForm and the form is
considered invalid if there is a duplicate name. This differs from
- xml-external item name must be unique across the XForm and the form
is considered invalid if there is a duplicate name. This differs from
other item types which allow duplicates if not in the same group.
- for all instance sources, if the same instance name is encountered,
the following rules are used to allow re-using instances but prevent
Expand Down Expand Up @@ -382,13 +388,14 @@ def _generate_instances(self):
if i.name in seen.keys() and seen[i.name].src != i.src:
# Instance id exists with different src URI -> error.
msg = "The same instance id will be generated for different " \
"external instance source URIs. Please check the form. " \
"Instance name: '{i}', Existing type: '{e}', " \
"external instance source URIs. Please check the form." \
" Instance name: '{i}', Existing type: '{e}', " \
"Existing URI: '{iu}', Duplicate type: '{d}', " \
"Duplicate URI: '{du}', Duplicate context: '{c}'.".format(
i=i.name, iu=seen[i.name].src, e=seen[i.name].type,
d=i.type, du=i.src, c=i.context
)
"Duplicate URI: '{du}', Duplicate context: '{c}'."\
.format(
i=i.name, iu=seen[i.name].src, e=seen[i.name].type,
d=i.type, du=i.src, c=i.context
)
raise PyXFormError(msg)
elif i.name in seen.keys() and seen[i.name].src == i.src:
# Instance id exists with same src URI -> ok, don't duplicate.
Expand All @@ -413,7 +420,8 @@ def xml_model(self):
model_children += list(self._generate_instances())
model_children += self.xml_bindings()

if self.submission_url or self.public_key or self.auto_send or self.auto_delete:
if (self.submission_url or self.public_key or self.auto_send or
self.auto_delete):
submission_attrs = dict()
if self.submission_url:
submission_attrs["action"] = self.submission_url
Expand Down Expand Up @@ -485,16 +493,20 @@ def _setup_choice_translations(name, choice_value, itext_id):
self._translations,
[media_type_or_language, itext_id, 'long'], value)

self._translations = defaultdict(dict) # pylint: disable=attribute-defined-outside-init
self._translations = defaultdict(dict) # pylint: disable=W0201
for element in self.iter_descendants():
for d in element.get_translations(self.default_language):
if 'guidance_hint' in d['path']:
hint_path = d['path'].replace('guidance_hint', 'hint')
self._translations[d['lang']][hint_path] = self._translations[d['lang']].get(hint_path, {})
self._translations[d['lang']][hint_path].update({"guidance": d['text']})
self._translations[d['lang']][hint_path] = \
self._translations[d['lang']].get(hint_path, {})
self._translations[d['lang']][hint_path].update(
{"guidance": d['text']})
else:
self._translations[d['lang']][d['path']] = self._translations[d['lang']].get(d['path'], {})
self._translations[d['lang']][d['path']].update({"long": d['text']})
self._translations[d['lang']][d['path']] = \
self._translations[d['lang']].get(d['path'], {})
self._translations[d['lang']][d['path']].update(
{"long": d['text']})

# This code sets up translations for choices in filtered selects.
for list_name, choice_list in self.choices.items():
Expand All @@ -503,7 +515,8 @@ def _setup_choice_translations(name, choice_value, itext_id):
itext_id = '-'.join(['static_instance', list_name,
str(idx)])
if isinstance(choice_value, dict):
_setup_choice_translations(name, choice_value, itext_id)
_setup_choice_translations(name, choice_value,
itext_id)
elif name == 'label':
self._add_to_nested_dict(
self._translations,
Expand Down Expand Up @@ -538,7 +551,7 @@ def _setup_media(self):
It matches the xform nesting order.
"""
if not self._translations:
self._translations = defaultdict(dict) # pylint: disable=attribute-defined-outside-init
self._translations = defaultdict(dict) # pylint: disable=W0201

for survey_element in self.iter_descendants():

Expand Down Expand Up @@ -625,12 +638,13 @@ def itext(self):
if media_type == "guidance":
itext_nodes.append(
node("value", value,
form='guidance',
toParseString=output_inserted)
form='guidance',
toParseString=output_inserted)
)
else:
itext_nodes.append(
node("value", value, toParseString=output_inserted)
itext_nodes.append(
node("value", value,
toParseString=output_inserted)
)
continue

Expand Down Expand Up @@ -716,26 +730,33 @@ def _var_repl_function(self, matchobj, context, use_current=False):
if self._xpath[name] is None:
raise PyXFormError(intro + " There are multiple survey elements"
" with this name.")
if context:
# if context xpath and target xpath fall under the same repeat use
# relative xpath referencing.
steps, ref_path = share_same_repeat_parent(self, self._xpath[name],
context.get_xpath())
if steps:
ref_path = ref_path if ref_path.endswith(name) else "/%s" % name
prefix = " current()/" if use_current else " "
return prefix + "/".join([".."] * steps) + ref_path + " "
if context and not (context['type'] == 'calculate' and
'indexed-repeat' in context['bind']['calculate']):
xpath, context_xpath = self._xpath[name], context.get_xpath()
# share same root i.e repeat_a from /data/repeat_a/...
if xpath.split('/')[2] == context_xpath.split('/')[2]:
# if context xpath and target xpath fall under the same
# repeat use relative xpath referencing.
steps, ref_path = share_same_repeat_parent(self, xpath,
context_xpath)
if steps:
ref_path = ref_path \
if ref_path.endswith(name) else "/%s" % name
prefix = " current()/" if use_current else " "

return prefix + "/".join([".."] * steps) + ref_path + " "

return " " + self._xpath[name] + " "

def insert_xpaths(self, text, context, use_current=False):
"""
Replace all instances of ${var} with the xpath to var.
"""
bracketed_tag = r"\$\{(.*?)\}"
def _var_repl_function(matchobj):
return self._var_repl_function(matchobj, context, use_current)

bracketed_tag = r"\$\{(.*?)\}"

return re.sub(bracketed_tag, _var_repl_function, unicode(text))

def _var_repl_output_function(self, matchobj, context):
Expand Down
20 changes: 20 additions & 0 deletions pyxform/survey_element.py
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import json

from pyxform import constants
from pyxform.utils import is_valid_xml_tag, node, unicode
from pyxform.xls2json import print_pyobj_to_json
from pyxform.question_type_dictionary import QUESTION_TYPE_DICT
from pyxform.errors import PyXFormError

try:
from functools import lru_cache
except ImportError:
from functools32 import lru_cache


def _overlay(over, under):
if type(under) == dict:
Expand Down Expand Up @@ -69,6 +75,10 @@ def __getattr__(self, key):
return _overlay(over, under)
raise AttributeError(key)

def __hash__(self):
return hash(id(self))

@property
def __name__(self):
return "SurveyElement"

Expand Down Expand Up @@ -145,6 +155,16 @@ def iter_descendants(self):
for f in e.iter_descendants():
yield f

@lru_cache(maxsize=None)
def any_repeat(self, parent_xpath):
"""Return True if there ia any repeat in `parent_xpath`."""
for item in self.iter_descendants():
if item.get_xpath() == parent_xpath and \
item.type == constants.REPEAT:
return True

return False

def get_lineage(self):
"""
Return a the list [root, ..., self._parent, self]
Expand Down
33 changes: 32 additions & 1 deletion pyxform/tests_v1/test_repeat.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def test_calculate_relative_path(self):
title="Paths in a calculate within a repeat are relative.",
md="""
| survey | | | | |
| | type | name | label | calculate |
| | type | name | label | calculation |
| | begin repeat | rep | | |
| | select_one crop_list | crop | Select | |
| | text | a | Verify | name = ${crop} |
Expand Down Expand Up @@ -155,3 +155,34 @@ def test_choice_filter_relative_path(self): # pylint: disable=invalid-name
"""<itemset nodeset="instance('crop_list')/root/item[name = current()/../../crop ]">""", # noqa pylint: disable=line-too-long
],
)

def test_indexed_repeat_relative_path(self):
"""Test relative path not used with indexed-repeat()."""
self.assertPyxformXform(
name="data",
title="Paths in a calculate within a repeat are relative.",
md="""
| survey | | | | |
| | type | name | label | calculation |
| | begin repeat | rep | | |
| | begin repeat | rep2 | | |
| | select_one crop_list | crop | Select | |
| | text | a | Verify | |
| | begin group | group | | |
| | text | b | Verify | |
| | end group | | | |
| | end repeat | | | |
| | calculate | c1 | | indexed-repeat(${a}, ${rep2}, 1) |
| | end repeat | | | |
| | | | | |
| | | | | |
| choices | | | | |
| | list name | name | label | |
| | crop_list | maize | Maize | |
| | crop_list | beans | Beans | |
| | crop_list | kale | Kale | |
""", # noqa pylint: disable=line-too-long
model__contains=[
"""<bind calculate="indexed-repeat( /data/rep/rep2/a , /data/rep/rep2 , 1)" nodeset="/data/rep/c1" type="string"/>""", # noqa pylint: disable=line-too-long
],
)
1 change: 1 addition & 0 deletions requirements.pip
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ traceback2==1.4.0 # via unittest2
unicodecsv==0.14.1
unittest2==1.1.0
xlrd==1.1.0
functools32==3.2.3.post2;python_version<"3.2"
21 changes: 14 additions & 7 deletions setup.py
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
# -*- coding: utf-8 -*-
"""Pyxform - Python library that converts XLSForms to XForms.
"""
from setuptools import setup, find_packages
import sys

from setuptools import find_packages, setup

REQUIRES = [
'xlrd>=1.1.0',
'unicodecsv>=0.14.1',
'formencode',
'unittest2',
]

if sys.version_info < (3, 2):
REQUIRES.append('functools32==3.2.3.post2')

setup(
name='pyxform',
Expand Down Expand Up @@ -30,12 +42,7 @@
url='http://pypi.python.org/pypi/pyxform/',
description='A Python package to create XForms for ODK Collect.',
long_description=open('README.rst', 'rt').read(),
install_requires=[
'xlrd>=1.1.0',
'unicodecsv>=0.14.1',
'formencode',
'unittest2',
],
install_requires=REQUIRES,
entry_points={
'console_scripts': [
'xls2xform=pyxform.xls2xform:main_cli',
Expand Down

0 comments on commit d40c759

Please sign in to comment.