Skip to content

Detect non-ambiguous dangling XPath operators - #861

Open
ukanga wants to merge 1 commit into
XLSForm:masterfrom
onaio:investigations
Open

Detect non-ambiguous dangling XPath operators#861
ukanga wants to merge 1 commit into
XLSForm:masterfrom
onaio:investigations

Conversation

@ukanga

@ukanga ukanga commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Closes #860

Why is this the best possible solution? Were any other approaches considered?

Expressions ending with an operator that requires a right-hand operand are
incomplete, but pyxform currently emits them into the generated XForm when
external validation is disabled. The resulting error is only reported later by
ODK Validate or a form engine, without the original XLSForm sheet, row, and
column information.

This change adds targeted validation before XForm generation. It uses the
existing expression lexer to inspect the final non-whitespace token and raises a
PyXFormError for these dangling operators:

  • Comparisons: =, !=, <, >, <=, >=
  • Arithmetic: +, -, div, mod
  • Boolean: and, or
  • Union: |

Terminal * is intentionally excluded because it can be a valid XPath
wildcard, such as /data/*. The keyword operators use token context so XPath
node names such as /data/and remain valid.

The validation covers:

  • Survey bind expressions: relevant, constraint, calculation, required,
    and readonly.
  • choice_filter, repeat count, and dynamic defaults.
  • The settings instance_name expression.
  • Entity entity_id, create_if, update_if, and label expressions.
  • Aliased and normalized headers while retaining original sheet and column
    names in error messages.

It runs when using both convert(validate=False) and
xls2xform --skip_validate.

The targeted lexer check addresses unambiguously dangling operators while
leaving broader XPath validation unchanged.

What are the regression risks?

The main risk is incorrectly rejecting literal values or XPath node names that
contain operator text. This is mitigated by checking lexer tokens and the
surrounding token context rather than using a string suffix check.

Tests verify that:

  • Every included operator is detected with no trailing whitespace, spaces, and
    mixed trailing whitespace.
  • Complete expressions and quoted operator characters remain accepted.
  • Terminal * wildcards and operator-word node names remain accepted.
  • Disabled survey rows are ignored.
  • Static defaults ending in an operator remain accepted.
  • Dynamic defaults and other recognized expression columns are checked.
  • Original sheet, row, and aliased column names are reported.
  • ODK Validate is not invoked after this internal validation fails.

There are no public API or type-signature changes. The intentional behavior
change is that previously emitted invalid expressions now raise a
PyXFormError, including when external validation is skipped.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No documentation updates are required. This is an internal validation
improvement for already invalid expressions.

AI/LLM assistance

This PR was developed with assistance from a coding agent and LLM, including
implementation, tests, and supporting text. The submitted changes remain the
responsibility of the contributor.

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

@lognaturel

Copy link
Copy Markdown
Contributor

Thanks for filing this, @ukanga. I think it's worth doing and the tests look reasonable. My immediate reaction is that it feels like the validation should be possible to do with an existing pass over fields that can have expressions. @lindsay-stevens can you please take a look when you get a chance?

@lognaturel
lognaturel removed their request for review August 20, 2026 22:07
Comment thread pyxform/parsing/expression.py Outdated
),
None,
)
== "OPS_COMP"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why would dangling comparison operators be more likely than any other dangling operator?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably not; I started with an example I encountered. Let me check whether I can identify more examples and whether it makes sense to address them in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems we have ambiguous paths and non-ambiguous paths. For the non-ambiguous, the proposed changes seem to align ok.

The * operator is considered ambiguous and, as such, is not addressed in this change.

I also tested the expression ${q1} = and ${q1} > 0 which is not flagged at any point including with ODK_Validate.jar. Is this valid?

<bind
      nodeset="/data/q2"
      type="int"
      relevant=" /data/q1  = and  /data/q1  &gt; 0"
  />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lognaturel

The check has been expanded to cover arithmetic (+, -, div, mod), boolean (and, or), and union (|) operators in addition to comparisons. Terminal * is excluded because it can be a valid XPath wildcard such as /data/*.

@ukanga ukanga changed the title Detect incomplete XPath comparisons Detect non-ambiguous dangling XPath operators Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject XLSForm expressions ending with non-ambiguous dangling operators

2 participants