Skip to content

Commit af7068d

Browse files
committed
fix: Register union operator in BinaryOperatorType
The FHIRPath union operator (|) was fully implemented in UnionOperator but never registered in the operator resolution chain. Parsing | threw UnsupportedFhirPathFeatureError, and the DSL tests that cover it were silently skipped. Adding a UNION enum constant wires the existing implementation into the parser.
1 parent b98cf56 commit af7068d

File tree

7 files changed

+93
-1
lines changed

7 files changed

+93
-1
lines changed

fhirpath/src/main/java/au/csiro/pathling/fhirpath/operator/BinaryOperatorType.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ public enum BinaryOperatorType {
6666
/** Arithmetic division operator. */
6767
DIVISION("/", new MathOperator(MathOperation.DIVISION)),
6868
/** Arithmetic modulus operator. */
69-
MODULUS("mod", new MathOperator(MathOperation.MODULUS));
69+
MODULUS("mod", new MathOperator(MathOperation.MODULUS)),
70+
/** Union operator. */
71+
UNION("|", new UnionOperator());
7072

7173
private final String symbol;
7274

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-02-17
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
## Context
2+
3+
The FHIRPath parser resolves binary operators in `Visitor.visitBinaryOperator()`
4+
through a two-stage lookup:
5+
6+
1. `BINARY_OPERATORS` map (built from `@FhirPathOperator`-annotated methods in
7+
`CollectionOperations`) — contains `in` and `contains`.
8+
2. `BinaryOperatorType.fromSymbol()` enum lookup — contains all arithmetic,
9+
comparison, equality, and boolean operators.
10+
11+
The union operator `|` is not registered in either location.
12+
`UnionOperator` exists with correct logic but is unreachable.
13+
14+
## Goals / Non-goals
15+
16+
**Goals:**
17+
18+
- Make `|` resolve to `UnionOperator` during FHIRPath parsing.
19+
- Ensure the existing `CombiningOperatorsDslTest` tests execute and pass.
20+
21+
**Non-goals:**
22+
23+
- Changing the `UnionOperator` implementation itself.
24+
- Adding new test cases beyond what already exists.
25+
26+
## Decisions
27+
28+
### Register `|` in the `BinaryOperatorType` enum
29+
30+
Add `UNION("|", new UnionOperator())` to the enum. This is consistent with how
31+
all other non-collection operators (arithmetic, comparison, equality, boolean)
32+
are registered.
33+
34+
**Alternative considered:** Register via `CollectionOperations` with a
35+
`@FhirPathOperator(name = "|")` annotated method. Rejected because
36+
`CollectionOperations` uses `MethodDefinedOperator` (method-reflection-based
37+
dispatch), while `UnionOperator` is a class-based operator extending
38+
`SameTypeBinaryOperator`. Mixing the two patterns would be inconsistent.
39+
40+
## Risks / Trade-offs
41+
42+
- **Low risk.** The `UnionOperator` is already tested (tests exist, just
43+
skipped). Wiring it in should cause those tests to pass without any other
44+
changes.
45+
- The test framework's silent skipping of `UnsupportedFhirPathFeatureError`
46+
masked this gap. No change to that framework behaviour is proposed here, but
47+
it is worth noting as a broader concern.
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
## Why
2+
3+
The FHIRPath union operator (`|`) is fully implemented in `UnionOperator` but
4+
never registered in the operator resolution chain. Parsing `|` throws
5+
`UnsupportedFhirPathFeatureError`, and the DSL tests that cover it are silently
6+
skipped by the test framework (which catches that error and converts it to a
7+
JUnit `TestAbortedException`).
8+
9+
## What changes
10+
11+
- Register the `UnionOperator` in `BinaryOperatorType` so that parsing `|`
12+
resolves to the existing implementation.
13+
- Ensure the DSL tests in `CombiningOperatorsDslTest` actually execute rather
14+
than being silently skipped.
15+
16+
## Capabilities
17+
18+
### New capabilities
19+
20+
(none)
21+
22+
### Modified capabilities
23+
24+
(none - this is a bug fix, not a requirement change)
25+
26+
## Impact
27+
28+
- `BinaryOperatorType.java` — add a new enum constant.
29+
- `CombiningOperatorsDslTest` — tests will transition from skipped to passing.
30+
- All contexts that parse FHIRPath (server, library API, fhirpath-lab-api) gain
31+
working union operator support.

openspec/changes/archive/2026-02-17-fix-union-operator-registration/specs/.gitkeep

Whitespace-only changes.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
No specification changes. This is a bug fix that wires existing implementation
2+
to existing parsing infrastructure.
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
## 1. Register the union operator
2+
3+
- [x] 1.1 Add `UNION("|", new UnionOperator())` to the `BinaryOperatorType` enum in `fhirpath/src/main/java/au/csiro/pathling/fhirpath/operator/BinaryOperatorType.java`
4+
5+
## 2. Verify
6+
7+
- [x] 2.1 Run `CombiningOperatorsDslTest` and confirm all 19 tests execute and pass (not skipped)
8+
- [x] 2.2 Run the full fhirpath module test suite to check for regressions

0 commit comments

Comments
 (0)