Skip to content

Type-driven date comparison - #51

Merged
nil-malh merged 2 commits into
mainfrom
fix/49
Aug 4, 2026
Merged

Type-driven date comparison#51
nil-malh merged 2 commits into
mainfrom
fix/49

Conversation

@nil-malh

@nil-malh nil-malh commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

📝 Description

AvroUtils.deepEquals() compares an "expected" map (schema-blind, hand-written/JSON via Gson — every value is a plain String) against an "actual" map (schema-aware, deserialized by AvroDeserializer — genuine logical-type fields are already Instant/LocalDateTime/LocalDate).

To reconcile the two, the code tried to guess whether a string "looked like" a date using a broken set of SimpleDateFormat patterns (SUPPORTED_DATE_PATTERNS / isDateString()):

  • A : typo instead of . before fractional seconds meant even standard millisecond ISO dates (...T16:19:14.123Z) never matched.
  • No pattern supported 6-digit microsecond fractions at all.

As a result, valid Avro string fields whose content merely looked like a date (e.g. a business field MyDate declared as ["null","string"]) could be silently mismatched/converted, or genuine timestamp fields pasted from tools like AKHQ would fail to compare correctly.

More fundamentally, content-sniffing can never reliably distinguish "a string field that happens to contain date-like text" from "a real logical-type date/timestamp field represented as a string on the expected side only."

Fix

Replaced content-sniffing with type-driven comparison:

  • compareValues() now checks the actual value's real Java type. If it's an Instant, LocalDateTime, or LocalDate (meaning AvroDeserializer has already resolved a genuine logical-type field) and the expected value is a String, the expected string is parsed into that exact target type (trying millisecond/microsecond/no-fraction precision as needed) and compared as real date/time values.
  • If both sides are plain Strings (no logical type was ever resolved), no date detection/conversion is attempted — they're compared literally, so genuine string schema fields are never misinterpreted.
  • Removed SUPPORTED_DATE_PATTERNS and isDateString() entirely.
  • convertDatesToTimestamps() no longer does any string-to-timestamp guessing; its previously dead/commented-out recursion into nested Map/List structures is now implemented properly (still no value conversion — just consistent, type-aware, structural recursion).

Files changed

  • ktestify-core/src/main/java/io/github/ktestify/utils/serdes/AvroUtils.java
    • Removed SUPPORTED_DATE_PATTERNS, isDateString(), unused SimpleDateFormat import.
    • Added parseExpectedInstant(), parseExpectedLocalDateTime(), parseExpectedLocalDate() helpers.
    • Updated compareValues() with type-driven Instant/LocalDateTime/LocalDate vs String comparison branches.
    • Implemented proper nested Map/List recursion in convertDatesToTimestamps().
  • ktestify-core/src/test/java/io/github/ktestify/utils/serdes/AvroUtilsTest.java
    • Removed tests for the deleted isDateString() / content-sniffing behavior.
    • Updated convertDatesToTimestamps* tests to assert values are left unconverted (no schema info available) while confirming nested recursion produces a correct structural copy.
    • Added a new TypeDrivenDateComparisonTests nested test class covering:
      • Real string fields with date-like content on both sides → compared literally, never converted (regression coverage for [Bug]: Date parsing issue #49).
      • Actual Instant (timestamp-micros) vs. expected microsecond-precision string → match.
      • Actual Instant vs. expected millisecond-precision / no-fraction string → match.
      • Actual LocalDate vs. expected yyyy-MM-dd string → match (and mismatch case).
      • Actual LocalDateTime vs. expected local-timestamp string → match.
      • Nested objects and nested lists containing date fields → type-driven comparison applied recursively.

Behavior preserved

  • No public method signatures changed.
  • excludedKeys handling in deepEquals(...) / performDeepEqualsComparison(...) is untouched.
  • convertDatesToTimestamps() remains public with the same signature, now doing a structural (deep-copy) recursion instead of unreliable content-based conversion.

Resolves #49


🔄 Type of Change

  • ♻️ Refactoring / technical debt — no functional change

✅ Pre-Merge Checklist

Code Quality

  • My code follows the Palantir Java Format style (passes mvn spotless:check)
  • I have added or updated Javadoc on public types and methods

Tests

  • I have added unit tests that cover the new / changed behaviour
  • All existing and new tests pass locally (mvn verify)
  • JaCoCo line coverage ≥ 70 % is maintained
  • Tests using Testcontainers are isolated and do not leave containers running

Licensing

  • Apache 2.0 license header has been added to every new source file (using spotless)

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✅ Test Results

Metric Count
Passed 668
Failed 0
⏭️ Skipped 0
📊 Total 668

✅ Coverage

Type Coverage Covered / Total
📏 Lines 83.3% 1755 / 2106
🌿 Branches 72.7% 537 / 739
🔧 Methods 83.8% 363 / 433

🔄 CI run #150 · Tue, 04 Aug 2026 13:01:11 GMT

@nil-malh
nil-malh merged commit c28df30 into main Aug 4, 2026
8 checks passed
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.

[Bug]: Date parsing issue

1 participant