#4018 fix: Cypher temporal component access on date/datetime returns null

[robfrank]

Summary

• date().year, date().month, date().day, datetime().year, etc. all returned null instead of the expected numeric component
• Root cause: ChainedPropertyAccessExpression.evaluate() (used when the base of a property access is a function call, not a bare variable) handled Document, Map, and Result types but was missing CypherTemporalValue, LocalDate, and LocalDateTime branches
• Fix: added the three missing temporal instanceof checks in ChainedPropertyAccessExpression, mirroring what PropertyAccessExpression already did correctly

Test plan

• 10 new regression tests added to OpenCypherTemporalFunctionsComprehensiveTest covering date(), datetime(), localdatetime(), localtime(), and time() component accessors on both literal and current-time expressions
• All 10 new tests fail before the fix, pass after
• All 5417 existing OpenCypher tests pass (0 regressions)

Fixes #4018

🤖 Generated with Claude Code

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 8 complexity

Metric	Results
Complexity	8

View in Codacy

🟢 Coverage 33.33% diff coverage · -7.88% coverage variation

Metric	Results
Coverage variation	✅ -7.88% coverage variation
Diff coverage	✅ 33.33% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (7d94afb)	120170	87923	73.17%
Head commit (3e73b4b)	151431 (+31261)	98864 (+10941)	65.29% (-7.88%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4031)	6	2	33.33%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review - #4031: Cypher temporal component access fix

Overview

This PR correctly fixes a null-return bug when accessing temporal components (.year, .month, .day, etc.) on the result of a temporal function call (e.g., date().year, datetime().hour). The root cause - ChainedPropertyAccessExpression missing temporal type handling that PropertyAccessExpression already had - is correctly identified and the fix is minimal and well-targeted.

---

Code Quality

Fix in CypherExpressionBuilder.java (lines 1671-1679)

The three added branches exactly mirror PropertyAccessExpression.evaluate(), which is good. One structural observation: the same type-dispatch logic now lives in two places. If a new temporal type is added in the future, both classes need updating. A shared static helper (e.g., TemporalPropertyAccess.get(Object base, String name)) would eliminate this duplication. Not a blocker for this PR, but worth noting.

Behavioral inconsistency (pre-existing, not introduced here)

PropertyAccessExpression throws CommandExecutionException for unrecognized base types, while ChainedPropertyAccessExpression silently returns null. The new temporal branches make both paths consistent with each other, but the outer fallthrough still differs. Worth a follow-up.

---

Test Coverage

The 10 new regression tests are appropriate and cover the main scenarios. A few observations:

Fully qualified type names instead of imports

The new tests use java.time.LocalDate.now() and java.time.ZonedDateTime.now() as fully qualified names rather than imports. The existing file has no java.time.* imports, so these should be added to the import block for consistency with project style:

// Add to imports:
import java.time.LocalDate;
import java.time.ZonedDateTime;

// Then in tests:
final LocalDate today = LocalDate.now();
final ZonedDateTime now = ZonedDateTime.now();

Theoretical race condition in "current time" tests

dateComponentsFromCurrentDate() and datetimeYearFromCurrentDatetime() capture now before issuing the query. If the test runs right at a day/year boundary the captured value and the query result may disagree. Very unlikely in practice, but literal-based tests (date('2020-01-15').year) are fully deterministic and the pattern used elsewhere in this file. Consider replacing or supplementing the current-time tests with purely deterministic assertions, or at minimum document the accepted flakiness risk.

Minor: Assertions.assertThat() vs static-imported assertThat()

Several new tests use the fully qualified Assertions.assertThat(result.hasNext()).isTrue() alongside the static-import assertThat(...). The rest of the file uses the static import uniformly. The redundant Assertions. qualifier could be dropped.

---

Summary

• Fix is correct, minimal, and well-explained in the PR description.
• Tests demonstrate the regression clearly.
• Suggested improvements: import java.time types instead of fully qualifying them, consider deterministic alternatives to "current time" tests, and watch for future duplication between the two property-access classes.

Overall this is a solid targeted fix - the concerns above are style/maintenance notes rather than blockers.

[gemini-code-assist]

Code Review

This pull request introduces support for accessing temporal components such as year, month, day, hour, and minute in OpenCypher queries. The changes include updates to the CypherExpressionBuilder to handle property access for CypherTemporalValue, LocalDate, and LocalDateTime types, along with a comprehensive suite of tests. Feedback suggests simplifying the type-checking logic in CypherExpressionBuilder by potentially consolidating the instanceof checks or better utilizing the CypherTemporalValue interface to avoid explicit type checks.

[gemini-code-assist]

The temporal type checks can be simplified by using a single instanceof check or a pattern matching switch if the Java version allows, but at minimum, consider consolidating the logic to avoid repeated getTemporalProperty calls if possible, or ensure that the CypherTemporalValue interface is fully utilized to avoid explicit LocalDate and LocalDateTime checks here.

[robfrank]

The three instanceof checks are necessary because LocalDate and LocalDateTime are java.time types that do not implement CypherTemporalValue - they need to be wrapped in CypherDate/CypherLocalDateTime before the property can be resolved. This is the same pattern used in PropertyAccessExpression.evaluate() for the same reason.

Consolidating into a single check would require either making java.time types implement CypherTemporalValue (a larger structural change) or adding a wrapper method - both are beyond the scope of this targeted fix. The duplication between the two expression classes is noted in the Claude bot review as a future improvement.

[claude]

Code Review

Overview

This PR fixes a real bug: ChainedPropertyAccessExpression (used when the base of a property access is a function call like date().year) was missing the temporal type branches that PropertyAccessExpression already handled correctly. The root cause analysis is accurate, the fix is minimal and correct.

---

Code Quality

The fix is correct and follows the existing pattern. The three added branches in ChainedPropertyAccessExpression.evaluate() are a direct mirror of what PropertyAccessExpression already does.

One concern worth raising: the duplicated logic now lives in two places. If a new temporal type is added (e.g., a future ZonedDateTime direct return from a function), it will need to be added to both PropertyAccessExpression and ChainedPropertyAccessExpression. A shared static helper would make this easier to maintain:

// e.g., in a shared TemporalPropertyAccessUtil or on CypherTemporalValue itself
static Object getTemporalProperty(final Object value, final String propertyName) {
    if (value instanceof CypherTemporalValue ctv)
        return ctv.getTemporalProperty(propertyName);
    if (value instanceof LocalDate ld)
        return new CypherDate(ld).getTemporalProperty(propertyName);
    if (value instanceof LocalDateTime ldt)
        return new CypherLocalDateTime(ldt).getTemporalProperty(propertyName);
    return null;
}

This is a suggestion for follow-up, not a blocker.

---

Behavior Asymmetry (pre-existing, worth documenting)

PropertyAccessExpression throws CommandExecutionException for unrecognized types (e.g., accessing .foo on an Integer). ChainedPropertyAccessExpression silently returns null for the same case. This asymmetry pre-exists this PR, but since this PR touches the file, it is worth a comment in the code or a follow-up issue. The return null; on the last line of ChainedPropertyAccessExpression.evaluate() will silently swallow type errors like date().year on a non-temporal type.

---

Performance

For the LocalDate / LocalDateTime branches, each property access allocates a new CypherDate / CypherLocalDateTime wrapper just to call getTemporalProperty. This matches what PropertyAccessExpression already does, so it is not a regression. However, if these paths are hot (e.g., in a large RETURN over many rows), the allocation could be avoided by extracting the property lookup directly. Low priority, but worth a note.

---

Test Coverage

The 10 new regression tests are well-chosen and cover:

• Literal date/datetime/localdatetime/localtime/time component access
• Current date/datetime component access (live values)

One potential flakiness risk: datetimeYearFromCurrentDatetime captures ZonedDateTime.now() before the query executes. If this test runs at exactly midnight on a year boundary, the year could differ between the Java capture and the query result. The probability is negligible, but if this becomes a flaky CI test, that is why. Replacing with a fixed literal (as done in the other tests) would fully eliminate the risk.

---

Minor

• The section comment // ==================== Temporal component accessor tests (issue #4018) ==================== uses a banner style not present elsewhere in the test class. Prefer a plain blank line + @Test to stay consistent with the surrounding style.
• The var row = result.next() usage is fine for Java 21.

---

Summary

The fix is correct and the test coverage is solid. Two actionable suggestions:

1. Consider a shared helper to avoid the duplicated temporal instanceof chain between PropertyAccessExpression and ChainedPropertyAccessExpression (follow-up OK).
2. Replace the live-time test (datetimeYearFromCurrentDatetime) with a fixed literal to avoid a theoretical midnight flake.

Neither is a blocker. Good find and clean fix.

[robfrank]

Addressed the review feedback in the second commit:

• Added import java.time.LocalDate and import java.time.ZonedDateTime - replaced inline FQNs in test methods with imported short names
• Dropped redundant Assertions. qualifier on hasNext() assertions - the static assertThat(boolean) overload is unambiguous

Not changed:

• Three instanceof checks - LocalDate/LocalDateTime don't implement CypherTemporalValue, so consolidation isn't possible without structural changes outside this fix's scope (explained in inline thread)
• Current-time tests (dateComponentsFromCurrentDate, datetimeYearFromCurrentDatetime) - accepted the theoretical boundary risk; tests complete in milliseconds and the pattern matches the rest of the file
• Shared static helper for temporal dispatch between PropertyAccessExpression and ChainedPropertyAccessExpression - valid future improvement, tracked separately

[robfrank]

On the banner comment style: the // ==================== ... ==================== pattern is used throughout this file already (see // ==================== duration() Tests ==================== at line 55, // ==================== duration.between() Tests ==================== at line 96, and others). The new section follows that convention.

On the datetimeYearFromCurrentDatetime flakiness concern: dateComponentsFromCurrentDate uses the identical capture-before-query pattern and is already in the file. Accepting the same risk there.

Everything else (shared helper, behavior asymmetry, performance) already acknowledged as follow-up.

[codecov]

Codecov Report

❌ Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.26%. Comparing base (7d94afb) to head (3e73b4b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ery/opencypher/parser/CypherExpressionBuilder.java	16.66%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4031   +/-   ##
=======================================
  Coverage   64.25%   64.26%           
=======================================
  Files        1597     1597           
  Lines      120170   120176    +6     
  Branches    25589    25592    +3     
=======================================
+ Hits        77220    77228    +8     
- Misses      32278    32279    +1     
+ Partials    10672    10669    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.