From a44ec20b4bec9e3604122d56cda8a0ed46e485d3 Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 09:45:13 +0200 Subject: [PATCH 1/6] fix(avro): resolve a field's schema name by NAME when the codec proves it (#95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.field(_.x)` resolved `x` to the schema field at its DECLARATION INDEX and nothing else: `AvroWalk.fieldNameAt`'s whole body was `fields.get(declIdx).name`, guarded only against a non-record parent and an index past the end. The field's NAME, its TYPE and the record's ARITY were never consulted. That is sound exactly while the codec's schema is positionally 1:1 with the case class — true by construction for every kindlings-derived codec, and NOT true for a hand-written or `vulcan.Codec` field list, which can add a computed column, drop one, or reorder. On those the optic silently targets the WRONG SLOT: schema {a, computed, b, c} vs case class Three(a, b, c) codecPrism[Three].field(_.b).getOption(bytes) // Some("A0|B0") <- `computed` codecPrism[Three].field(_.b).replace("B1")(bytes) // rewrites `computed` and produces valid Avro bytes with wrong content. Nothing catches it: get-put, put-get, put-put and modify-fusion all HOLD on a mis-targeted optic (it is a perfectly lawful Optional onto the wrong field), round-trips hold, and every fixture in `AvroSpecFixtures` is kindlings-derived, hence 1:1, hence blind. One function, six call sites: `.field`, `selectDynamic`, `.fields`, `.each.field`, `.each.fields`, and every deeper hop after a `.union[B]`. The fix adds one rung ABOVE position, and only fires when the codec has proved the whole correspondence: 1. NOMINAL, all-or-nothing. If EVERY case field maps to a DISTINCT schema field — exactly, or uniquely up to `_`/`-`/`.` and case — the map is total and injective, so it is the codec's own answer, not a guess. 2. POSITIONAL (issue #35), unchanged. This is where a name transform lands, because a transform REMOVES the literal Scala name by construction. All-or-nothing is the load-bearing part, and it is measured, not assumed. The per-field form of rung 1 ("does THIS field's name appear?") INTRODUCES corruption: `FpVisit(userId, user)` against legacy columns `{uid, user_id}` is correct by position today, and a per-field rung re-aims `userId` at `user_id`. Requiring totality makes the rung abstain there — `user` matches nothing — and position stays right. An arity gate was rejected on the same evidence: it refuses 13 of 28 legitimate call sites (a trailing `ingested_at`, a dropped `cachedHash`, a trailing checksum) to buy 3 cells. Per-operation cost is zero: resolution runs once, at prism construction, off the cached schema. `to`, `from`, `scan`, `spliceAff`, `spliceFoci` and both erased bridges contain no reference to it on either carrier. Known residual, pinned as specs rather than prose: a codec that both renames beyond recognition AND reorders (equal arity, no name hit) still resolves by position and is still wrong. Known behaviour change: a codec that PERMUTES the Scala names was right by position and is now wrong by name — no transform can produce that shape, only a hand-written field list. Back-compat: every touched signature is `private[avro]`, but `transparent inline` bakes the accessor into CALLER bytecode, so downstream must recompile, not re-jar. Tests: - `DivergentCodecs` (new) — hand-written vulcan codecs whose schema is not positionally 1:1. The standing gap: every existing fixture is derived. - `AvroNominalResolutionSpec` (new) — the 9 repro cells (computed field on the byte and record faces, the leaf past the divergence, `.fields` grouped read+write, `.each.field`, a nested record's own divergence, a reversed field list, snake_case + a computed field) plus the two false-positive controls. Every repro cell failed before this change; both controls passed. - `ResolutionFalsePositiveSpec` (new) — 28 legitimate, currently-working call sites scored against SLOT TRUTH. 28/28 CORRECT before and after: that scorecard not moving is the entire safety argument for the mechanism. - `AvroWalkSpec` — three direct `resolveFieldName` / `fieldNameAt` calls take the new case-name list (mechanical, `Nil`). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- .../dev/constructive/eo/avro/AvroPrism.scala | 23 +- .../constructive/eo/avro/AvroPrismMacro.scala | 55 ++- .../constructive/eo/avro/AvroTraversal.scala | 12 +- .../dev/constructive/eo/avro/AvroWalk.scala | 121 +++++- .../constructive/eo/avro/AvroWalkSpec.scala | 6 +- .../vulcan/AvroNominalResolutionSpec.scala | 164 ++++++++ .../eo/avro/vulcan/DivergentCodecs.scala | 183 +++++++++ .../vulcan/ResolutionFalsePositiveSpec.scala | 361 ++++++++++++++++++ 8 files changed, 899 insertions(+), 26 deletions(-) create mode 100644 avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala create mode 100644 avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala create mode 100644 avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala index d66ad3d1..8e067aca 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala @@ -241,16 +241,24 @@ final class AvroPrism[A] private[avro] ( // ---- Path widening (used by macro extensions) --------------------- /** Extend the Leaf path by a field step. Used by [[field]] / `selectDynamic`. `scalaName` is the - * case-class field name and `declIdx` its declaration index; the actual schema field name (which - * may differ under a snake/kebab/custom transform or vulcan overrides) is resolved off the - * cached schema by position — see [[AvroWalk.resolveFieldName]] (issue #35). + * case-class field name, `declIdx` its declaration index and `caseNames` the parent's whole + * case-field list; the actual schema field name (which may differ under a snake/custom transform + * or vulcan overrides) is resolved off the cached schema by the name-then-position rule — see + * [[AvroWalk.fieldNameAt]] (issues #35 and #95). */ - private[avro] def widenPath[B](scalaName: String, declIdx: Int)(using + private[avro] def widenPath[B](scalaName: String, declIdx: Int, caseNames: List[String])(using codecB: AvroCodec[B] ): AvroPrism[B] = widenPathStep[B]( PathStep.Field( - AvroWalk.resolveFieldName(rootSchemaCached, path, scalaName, declIdx, "AvroPrism.field") + AvroWalk.resolveFieldName( + rootSchemaCached, + path, + scalaName, + declIdx, + caseNames, + "AvroPrism.field", + ) ) ) @@ -289,9 +297,10 @@ final class AvroPrism[A] private[avro] ( private[avro] def toFieldsPrism[B]( scalaNames: Array[String], declIdxs: Array[Int], + caseNames: List[String], )(using codecB: AvroCodec[B]): AvroPrism[B] = new AvroPrism[B]( - new AvroFocus.Fields[B](path, resolveFieldNames(scalaNames, declIdxs), codecB), + new AvroFocus.Fields[B](path, resolveFieldNames(scalaNames, declIdxs, caseNames), codecB), rootSchemaCached, ) @@ -301,6 +310,7 @@ final class AvroPrism[A] private[avro] ( private def resolveFieldNames( scalaNames: Array[String], declIdxs: Array[Int], + caseNames: List[String], ): Array[String] = Array.tabulate(scalaNames.length)(i => AvroWalk.resolveFieldName( @@ -308,6 +318,7 @@ final class AvroPrism[A] private[avro] ( path, scalaNames(i), declIdxs(i), + caseNames, "AvroPrism.fields", ) ) diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala index 1113f12a..458a30ad 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala @@ -31,7 +31,11 @@ object AvroPrismMacro: } '{ - $parent.widenPath[B](${ Expr(name) }, ${ Expr(declIndexOf[A](name)) })(using $codecB) + $parent.widenPath[B]( + ${ Expr(name) }, + ${ Expr(declIndexOf[A](name)) }, + ${ Expr(caseNamesOf[A]) }, + )(using $codecB) } /** Drives `codecPrism[Person].name`. Looks `name` up on `A`'s schema, summons `AvroCodec[B]`, @@ -45,7 +49,13 @@ object AvroPrismMacro: "AvroPrism selectDynamic", nameE, ) { [b] => (name: String, declIdx: Int, codecB: Expr[AvroCodec[b]]) => - '{ $parent.widenPath[b](${ Expr(name) }, ${ Expr(declIdx) })(using $codecB) } + '{ + $parent.widenPath[b]( + ${ Expr(name) }, + ${ Expr(declIdx) }, + ${ Expr(caseNamesOf[A]) }, + )(using $codecB) + } } /** Macro for `.at(i)`. Verifies `A <: Iterable`, extracts the element type, summons the codec, @@ -204,7 +214,11 @@ object AvroPrismMacro: } '{ - $parent.widenSuffix[B](${ Expr(name) }, ${ Expr(declIndexOf[A](name)) })(using $codecB) + $parent.widenSuffix[B]( + ${ Expr(name) }, + ${ Expr(declIndexOf[A](name)) }, + ${ Expr(caseNamesOf[A]) }, + )(using $codecB) } /** Traversal counterpart to [[atImpl]] — extends the suffix by an array index. */ @@ -235,7 +249,14 @@ object AvroPrismMacro: namesExpr: Expr[Array[String]], declIdxsExpr: Expr[Array[Int]], codecNT: Expr[AvroCodec[nt]], - ) => '{ $parent.toFieldsPrism[nt]($namesExpr, $declIdxsExpr)(using $codecNT) } + ) => + '{ + $parent.toFieldsPrism[nt]( + $namesExpr, + $declIdxsExpr, + ${ Expr(caseNamesOf[A]) }, + )(using $codecNT) + } } /** Traversal counterpart to [[fieldsImpl]]. */ @@ -248,7 +269,14 @@ object AvroPrismMacro: namesExpr: Expr[Array[String]], declIdxsExpr: Expr[Array[Int]], codecNT: Expr[AvroCodec[nt]], - ) => '{ $parent.toFieldsTraversal[nt]($namesExpr, $declIdxsExpr)(using $codecNT) } + ) => + '{ + $parent.toFieldsTraversal[nt]( + $namesExpr, + $declIdxsExpr, + ${ Expr(caseNamesOf[A]) }, + )(using $codecNT) + } } /** Traversal counterpart to [[selectFieldImpl]] — drives Dynamic sugar by extending the suffix. @@ -261,7 +289,13 @@ object AvroPrismMacro: "AvroTraversal selectDynamic", nameE, ) { [b] => (name: String, declIdx: Int, codecB: Expr[AvroCodec[b]]) => - '{ $parent.widenSuffix[b](${ Expr(name) }, ${ Expr(declIdx) })(using $codecB) } + '{ + $parent.widenSuffix[b]( + ${ Expr(name) }, + ${ Expr(declIdx) }, + ${ Expr(caseNamesOf[A]) }, + )(using $codecB) + } } /** Shared backbone for [[fieldsImpl]] / [[fieldsTraversalImpl]] — validation + SELECTOR-order @@ -318,6 +352,15 @@ object AvroPrismMacro: import quotes.reflect.* TypeRepr.of[A].typeSymbol.caseFields.indexWhere(_.name == name) + /** `A`'s case-field names in declaration order (`Nil` when `A` isn't a case class — a NamedTuple + * parent, say). Emitted as a compile-time literal list beside the declaration index so + * construction-time resolution can check whether the codec's schema names the WHOLE case-field + * list before it trusts any one of them (issue #95). `Nil` abstains, leaving position in charge. + */ + private def caseNamesOf[A: Type](using q: Quotes): List[String] = + import quotes.reflect.* + TypeRepr.of[A].typeSymbol.caseFields.map(_.name) + /** Summon `AvroCodec[B]` with a caller-supplied error message. */ private def summonCodec[B: Type]( errorMsg: String diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala index e6aa7d6b..50be1ca0 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala @@ -158,10 +158,17 @@ final class AvroTraversal[A] private[avro] ( private[avro] def widenSuffix[B]( scalaName: String, declIdx: Int, + caseNames: List[String], )(using codecB: AvroCodec[B]): AvroTraversal[B] = widenSuffixStep[B]( PathStep.Field( - AvroWalk.fieldNameAt(suffixParentRecord, scalaName, declIdx, "AvroTraversal.field") + AvroWalk.fieldNameAt( + suffixParentRecord, + scalaName, + declIdx, + caseNames, + "AvroTraversal.field", + ) ) ) @@ -219,10 +226,11 @@ final class AvroTraversal[A] private[avro] ( private[avro] def toFieldsTraversal[B]( scalaNames: Array[String], declIdxs: Array[Int], + caseNames: List[String], )(using codecB: AvroCodec[B]): AvroTraversal[B] = val parent = suffixParentRecord val resolved = Array.tabulate(scalaNames.length)(i => - AvroWalk.fieldNameAt(parent, scalaNames(i), declIdxs(i), "AvroTraversal.fields") + AvroWalk.fieldNameAt(parent, scalaNames(i), declIdxs(i), caseNames, "AvroTraversal.fields") ) new AvroTraversal[B]( prefix, diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala index 5d501781..2c061148 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala @@ -379,10 +379,9 @@ private[avro] object AvroWalk: case other => Left(s"step $i: expected an array/map but found $other") loop(0, root) - /** The schema field name for the case-class field at declaration index `declIdx` inside the - * record reached by [[schemaAt]]`(root, parentPath)`. Position resolution: the i-th case field - * maps to the i-th schema field, honouring any codec name transform (identity / snake / kebab / - * custom / vulcan overrides), because the name is read back OUT of the schema. + /** The schema field name for the case-class field `scalaName` — declaration index `declIdx` among + * the case fields `caseNames` — inside the record reached by [[schemaAt]]`(root, parentPath)`. + * See [[fieldNameAt]] for the resolution rule. * * `declIdx < 0` (index undeterminable — a non-case-class parent such as a NamedTuple) falls back * to the literal `scalaName`, preserving the pre-#35 behaviour for those parents. Every other @@ -395,6 +394,7 @@ private[avro] object AvroWalk: parentPath: Array[PathStep], scalaName: String, declIdx: Int, + caseNames: List[String], who: String, ): String = if declIdx < 0 then scalaName @@ -406,12 +406,46 @@ private[avro] object AvroWalk: + " Navigate by explicit schema name with .fieldNamed(\"\")." ) case Right(parent) => - fieldNameAt(parent, scalaName, declIdx, who) + fieldNameAt(parent, scalaName, declIdx, caseNames, who) - /** Schema field name at declaration index `declIdx` of `record` (which must be a RECORD). Split - * out so [[AvroTraversal]] can resolve against an element record it computed itself. + /** Schema field name for case field `scalaName` (declaration index `declIdx` among `caseNames`) + * in `record`, which must be a RECORD. Split out so [[AvroTraversal]] can resolve against an + * element record it computed itself. + * + * '''Two rungs, tried in order (issue #95).''' + * + * 1. NOMINAL, all-or-nothing. If EVERY case field in `caseNames` maps to a DISTINCT schema + * field — exactly, or uniquely up to `_`/`-`/`.` and case — then the codec has told us the + * whole correspondence and `declIdx`'s answer is read off that map. Partial or colliding + * coverage is no signal at all: the rung abstains for every field rather than trusting one + * lucky match (see [[totalNominalIndex]] for why the per-field form is unsound). + * 1. POSITIONAL (issue #35): the i-th case field is the i-th schema field. This is the rung a + * name transform lands on — `withSnakeCaseFieldNames`, a custom `transformFieldNames`, a + * vulcan override map — because a transform REMOVES the literal Scala name by construction, + * so rung 1 cannot have fired. It is also the only rung that can be wrong, and it is right + * exactly when the codec's schema is positionally 1:1 with the case class. + * + * Nominal before positional is safe precisely because the two rungs see disjoint populations. + * What nominal DOES see is the codec whose schema is not positionally 1:1 — a computed/derived + * schema field, a dropped one, a reordered hand-written or `vulcan.Codec` field list — where + * position silently targets the wrong slot and produces valid wire bytes with wrong content. + * + * '''Known residual''': a codec that both renames beyond recognition AND reorders (equal arity, + * no name hit) still resolves by position, and is still wrong — nothing about the names or the + * shape can see it. Use [[AvroPrism.fieldNamed]] there. + * + * '''Known behaviour change''': a codec that PERMUTES the Scala names (writes case field `a` + * into a schema field literally named `b`, and vice versa) resolved correctly by position and + * now resolves by name, i.e. wrongly. No name transform can produce that shape — a transform is + * a function of the name alone — but a hand-written field list can. */ - def fieldNameAt(record: Schema, scalaName: String, declIdx: Int, who: String): String = + def fieldNameAt( + record: Schema, + scalaName: String, + declIdx: Int, + caseNames: List[String], + who: String, + ): String = if record.getType != Schema.Type.RECORD then throw new IllegalArgumentException( s"$who('$scalaName'): parent focus is a ${record.getType}, not a record." @@ -419,16 +453,85 @@ private[avro] object AvroWalk: ) else val fields = record.getFields - if declIdx >= fields.size then + val nominal = totalNominalIndex(record, caseNames, declIdx) + if nominal >= 0 then fields.get(nominal).name + else if declIdx >= fields.size then throw new IllegalArgumentException( s"$who('$scalaName'): case field #$declIdx has no matching schema field —" + s" record '${record.getFullName}' has ${fields.size} field(s): " + fields.asScala.map(_.name).mkString(", ") + + s", none named '$scalaName'" + ". For a reordered hand-written codec, navigate by explicit schema name with" + " .fieldNamed(\"\")." ) else fields.get(declIdx).name + /** ALL-OR-NOTHING nominal resolution: the schema-field position for the case field at `declIdx`, + * or `-1` to abstain and let position decide. + * + * The per-field form of this rung — "does THIS case field's name appear in the schema?" — is + * unsound, and measurably so: a schema field can bear a name that resembles a DIFFERENT case + * field. `FpVisit(userId, user)` against legacy columns `{uid, user_id}` writes `userId` into + * `uid`, and a per-field rung matches `userId` against `user_id` and re-aims a currently-correct + * call site at the wrong column. Requiring the WHOLE case-field list to map to DISTINCT schema + * fields (total and injective) before trusting any single answer removes that class: `user` + * matches nothing, the map is not total, the rung abstains, position stays right. + * + * An empty `caseNames` (a NamedTuple parent, which has no case fields) abstains too. + */ + private def totalNominalIndex(record: Schema, caseNames: List[String], declIdx: Int): Int = + if caseNames.isEmpty || declIdx < 0 || declIdx >= caseNames.size then -1 + else + val out = new Array[Int](caseNames.size) + @tailrec def loop(i: Int, rest: List[String], seen: Set[Int]): Boolean = + rest match + case Nil => true + case n :: t => + val idx = nominalIndex(record, n) + if idx < 0 || seen.contains(idx) then false + else + out(i) = idx + loop(i + 1, t, seen + idx) + if loop(0, caseNames, Set.empty) then out(declIdx) else -1 + + /** Position of the schema field naming `scalaName` — exactly, else uniquely up to separators and + * case. `-1` when no field matches or when more than one does: an ambiguous signal is no signal. + */ + private def nominalIndex(record: Schema, scalaName: String): Int = + val exact = record.getField(scalaName) + if exact != null then exact.pos + else + val fields = record.getFields + @tailrec def loop(i: Int, found: Int): Int = + if i >= fields.size then found + else if sameFieldName(fields.get(i).name, scalaName) then + if found >= 0 then -1 else loop(i + 1, i) + else loop(i + 1, found) + loop(0, -1) + + /** `a` and `b` name the same field up to `_` / `-` / `.` and case: `landingPageId`, + * `landing_page_id`, `LANDING_PAGE_ID` and `landing-page-id` all match. Two cursors rather than + * two normalised copies — this runs once per candidate schema field per drilled hop, at + * construction time, and has no business allocating. + * + * NOT a transform inverter: it recognises only the letter-preserving transform family, which is + * exactly why an unrecognised transform falls through to the positional rung (issue #35) instead + * of being guessed at. + */ + private def sameFieldName(a: String, b: String): Boolean = + @tailrec def skip(s: String, i: Int): Int = + if i >= s.length then i + else + val c = s.charAt(i) + if c == '_' || c == '-' || c == '.' then skip(s, i + 1) else i + @tailrec def loop(i: Int, j: Int): Boolean = + val x = skip(a, i) + val y = skip(b, j) + if x >= a.length || y >= b.length then x >= a.length && y >= b.length + else if Character.toLowerCase(a.charAt(x)) != Character.toLowerCase(b.charAt(y)) then false + else loop(x + 1, y + 1) + loop(0, 0) + /** Union-branch name for a runtime value. Schema-driven where possible — the named leaves * (records, enums, fixed) answer with their schema's FULL name, matching how union branches are * declared; generic-decoded `bytes` arrive as `ByteBuffer`. Falls back to the raw class name diff --git a/avro/src/test/scala/dev/constructive/eo/avro/AvroWalkSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/AvroWalkSpec.scala index dc26aaa3..6f99e80a 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/AvroWalkSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/AvroWalkSpec.scala @@ -337,7 +337,7 @@ class AvroWalkSpec extends Specification: // consulted, proving the -1 branch short-circuits before that) "AvroWalk.resolveFieldName: declIdx < 0 short-circuits to the literal scalaName" >> { val stringSchema = Schema.create(Schema.Type.STRING) - AvroWalk.resolveFieldName(stringSchema, Array.empty, "literalName", -1, "test") === + AvroWalk.resolveFieldName(stringSchema, Array.empty, "literalName", -1, Nil, "test") === "literalName" } @@ -347,13 +347,13 @@ class AvroWalkSpec extends Specification: val stringSchema = Schema.create(Schema.Type.STRING) val nonRecordThrows = try - AvroWalk.fieldNameAt(stringSchema, "x", 0, "test") + AvroWalk.fieldNameAt(stringSchema, "x", 0, Nil, "test") false catch case _: IllegalArgumentException => true val outOfRangeThrows = try - AvroWalk.fieldNameAt(personSchema, "x", 99, "test") + AvroWalk.fieldNameAt(personSchema, "x", 99, Nil, "test") false catch case _: IllegalArgumentException => true diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala new file mode 100644 index 00000000..cc7e56b5 --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala @@ -0,0 +1,164 @@ +package dev.constructive.eo.avro.vulcan + +import scala.language.implicitConversions + +import dev.constructive.eo.avro.{codecPrism, AvroCodec} +import org.apache.avro.generic.{GenericRecord, IndexedRecord} +import org.specs2.mutable.Specification + +/** The gate for schema-field resolution (`AvroWalk.fieldNameAt`) — issue #95. + * + * Issue #35 resolved `.field(_.x)` to the schema field at `x`'s DECLARATION INDEX, with no name + * cross-check at all. That is sound only while the codec's schema is positionally 1:1 with the + * case class — true by construction for every kindlings-derived codec, and NOT true for a + * hand-written or `vulcan.Codec` field list, which can add a computed field, drop one, or reorder. + * On those the optic targets the WRONG SLOT and produces valid wire bytes with wrong content: no + * law sees it (a mis-targeted optic is a perfectly lawful `Optional` onto the wrong field), no + * round-trip sees it, and no existing fixture could have seen it because every fixture in the + * suite is derived. + * + * Every example below scores SLOT TRUTH — which schema slot a write actually touched — never + * `copy(...)`-truth, which cannot tell a wrong slot from a stale derived one. + * + * The false-positive controls at the bottom are as load-bearing as the repros: issue #35's + * motivating case (a name transform, which by definition destroys the literal Scala name) MUST + * keep resolving by position, and `AvroFieldNamingSpec` is the snake_case half of that guard. + */ +class AvroNominalResolutionSpec extends Specification: + + import DivergentCodecs.* + + private val threeSchema = summon[AvroCodec[Three]].schema + private val three = Three("A0", "B0", 7L) + private val threeBytes = encodeBytes(three) + + // ---- cells 1-3: the reported bug, byte face ------------------------- + // schema {a, computed, b, c} vs case class {a, b, c}: one computed field shifts every later slot. + + "a computed schema field does not shift the READ onto the wrong slot" >> { + codecPrism[Three].field(_.b).getOption(threeBytes) must beSome("B0") + } + + "a computed schema field does not shift the WRITE onto the wrong slot" >> { + val after = slots(codecPrism[Three].field(_.b).replace("B1")(threeBytes), threeSchema) + // Exactly the intended slot moved. `computed` keeps its stale value — a splice optic never + // recomputes a derived field, which is by design and is NOT a targeting failure. + (after("b") === "B1") + .and(after("a") === "A0") + .and(after("c") === "7") + .and(after("computed") === "A0|B0") + } + + "a leaf PAST the divergence still round-trips (the wrong slot does not even decode)" >> { + val after = slots(codecPrism[Three].field(_.c).replace(9L)(threeBytes), threeSchema) + (codecPrism[Three].field(_.c).getOption(threeBytes) must beSome(7L)) + .and(after("c") === "9") + .and(after("b") === "B0") + } + + // ---- cell 4: the record face resolves through the same function ----- + + "the record face resolves through the same resolver, read and write" >> { + val rec = summon[AvroCodec[Three]].encode(three).asInstanceOf[IndexedRecord] + val out = codecPrism[Three].field(_.b).record.modifyUnsafe(_ + "!")(rec) + (codecPrism[Three].field(_.b).record.getOption(rec) must beSome("B0")) + .and(String.valueOf(out.asInstanceOf[GenericRecord].get("b")) === "B0!") + .and(String.valueOf(out.asInstanceOf[GenericRecord].get("computed")) === "A0|B0") + } + + // ---- cell 5: `.fields` — one grouped call used to damage two slots -- + + "`.fields` resolves every selector independently (one call used to damage two slots)" >> { + import ThreeNt.given + val read = codecPrism[Three].fields(_.a, _.b).getOption(threeBytes) + val after = slots( + codecPrism[Three].fields(_.a, _.b).replace((a = "A1", b = "B1"): ThreeNt.AB)(threeBytes), + threeSchema, + ) + (read must beSome((a = "A0", b = "B0"): ThreeNt.AB)) + .and(after("a") === "A1") + .and(after("b") === "B1") + .and(after("c") === "7") + } + + // ---- cell 6: the traversal suffix (`.each.field`) -------------------- + + "a traversal suffix resolves through the same resolver, read and write" >> { + val basket = Basket(List(Inner("X0", 1L))) + val basketSchema = summon[AvroCodec[Basket]].schema + val basketBytes = encodeBytes(basket) + val basketRec = summon[AvroCodec[Basket]].encode(basket).asInstanceOf[IndexedRecord] + val optic = codecPrism[Basket].field(_.items).each.field(_.x) + val out = optic.modify(_ + "@")(basketBytes) + val item = recordOf(out, basketSchema) + .get("items") + .asInstanceOf[java.util.List[GenericRecord]] + .get(0) + (optic.record.getAllUnsafe(basketRec) === Vector("X0")) + .and(String.valueOf(item.get("x")) === "X0@") + // the inner codec's derived slot `c` was written as "X0!" and must be untouched + .and(String.valueOf(item.get("c")) === "X0!") + } + + "a nested record's own divergence is resolved one hop down" >> { + val outerSchema = summon[AvroCodec[Outer]].schema + val outerBytes = encodeBytes(Outer("O0", Inner("X0", 1L))) + val optic = codecPrism[Outer].field(_.inner).field(_.x) + val inner = recordOf(optic.replace("X1")(outerBytes), outerSchema) + .get("inner") + .asInstanceOf[GenericRecord] + (optic.getOption(outerBytes) must beSome("X0")) + .and(String.valueOf(inner.get("x")) === "X1") + .and(String.valueOf(inner.get("c")) === "X0!") + } + + // ---- cell 7: reversed field order, both String ---------------------- + // Equal arity, identical leaf types: no shape signal and no decode failure exist. Names only. + + "a reordered field list reads and writes the named slot, not the i-th one" >> { + val pairSchema = summon[AvroCodec[Pair]].schema + val bytes = encodeBytes(Pair("ALPHA0", "BETA0")) + val after = slots(codecPrism[Pair].field(_.alpha).replace("ALPHA1")(bytes), pairSchema) + (codecPrism[Pair].field(_.alpha).getOption(bytes) must beSome("ALPHA0")) + .and(after("alpha") === "ALPHA1") + .and(after("beta") === "BETA0") + } + + // ---- cell 8: snake_case schema PLUS a computed field ---------------- + // No literal Scala name survives, so exact-name matching alone sends this to the wrong slot. + + "a snake_case schema with a computed field still resolves to the right slot" >> { + val schema = summon[AvroCodec[SnakeComputed]].schema + val bytes = encodeBytes(SnakeComputed("c0", 7L)) + val optic = codecPrism[SnakeComputed].field(_.landingPageId) + val after = slots(optic.replace(8L)(bytes), schema) + (optic.getOption(bytes) must beSome(7L)) + .and(after("landing_page_id") === "8") + .and(after("click_id") === "c0") + .and(after("computed") === "c0!") + } + + // ---- FALSE-POSITIVE CONTROLS: issue #35's own population ------------ + // A name transform REMOVES the literal Scala name by definition, so no nominal rung can fire and + // POSITION must stay in charge. Breaking either of these breaks what #35 shipped for. + + "FALSE-POSITIVE CONTROL: a hostile custom name transform still resolves by POSITION" >> { + import HostileTransform.* + val schema = summon[AvroCodec[Hostile]].schema + val bytes = encodeBytes(Hostile("A0", 1L)) + val after = slots(codecPrism[Hostile].field(_.alpha).replace("A1")(bytes), schema) + (codecPrism[Hostile].field(_.alpha).getOption(bytes) must beSome("A0")) + .and(after("x_ahpla") === "A1") + .and(after("x_ateb") === "1") + } + + "FALSE-POSITIVE CONTROL: a vulcan rename with preserved order still resolves by POSITION" >> { + val schema = summon[AvroCodec[NPair]].schema + val bytes = encodeBytes(NPair("ALPHA0", "BETA0")) + val after = slots(codecPrism[NPair].field(_.alpha).replace("ALPHA1")(bytes), schema) + (codecPrism[NPair].field(_.alpha).getOption(bytes) must beSome("ALPHA0")) + .and(after("alpha_name") === "ALPHA1") + .and(after("beta_name") === "BETA0") + } + +end AvroNominalResolutionSpec diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala new file mode 100644 index 00000000..8cb9aa80 --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala @@ -0,0 +1,183 @@ +package dev.constructive.eo.avro.vulcan + +import scala.jdk.CollectionConverters.* + +import _root_.vulcan.Codec as VCodec +import cats.syntax.all.* +import dev.constructive.eo.avro.AvroCodec +import hearth.kindlings.avroderivation.{AvroConfig, AvroDecoder, AvroEncoder, AvroSchemaFor} +import org.apache.avro.Schema +import org.apache.avro.generic.GenericRecord + +/** Hand-written vulcan codecs whose Avro schema is NOT positionally 1:1 with the case class — the + * population the position-resolution design (issue #35) silently mis-targets. + * + * This is the standing gap these fixtures close: every fixture in `AvroSpecFixtures` is + * kindlings-derived, hence 1:1 by construction, so no existing example could have seen the + * hazard. `vulcan.Codec` is the realistic carrier for the divergent shapes: `Codec.record`'s field + * list names each schema field independently of the case field it accesses, so order, arity and + * naming can all diverge — and the bridge (`AvroVulcan.codec`) can recover none of it. + * + * Evidence rule for every fixture: the payload is produced by the codec ITSELF, so the value in + * schema field `f` is by construction whatever the codec decided to write there. That makes SLOT + * TRUTH (not `copy(...)`-truth) checkable without guessing. + */ +object DivergentCodecs: + + def encodeBytes[A](a: A)(using c: AvroCodec[A]): Array[Byte] = + AvroCodec.encodeValue(a).fold(f => throw new RuntimeException(f.toString), identity) + + def recordOf(bytes: Array[Byte], schema: Schema): GenericRecord = + AvroCodec + .decodeRecord(bytes, schema) + .fold(f => throw new RuntimeException(f.toString), _.asInstanceOf[GenericRecord]) + + /** Every top-level schema slot of `bytes`, stringified, keyed by SCHEMA field name. The ground + * truth a write is scored against: exactly one slot may change, and it must be the intended one. + */ + def slots(bytes: Array[Byte], schema: Schema): Map[String, String] = + AvroCodec + .decodeRecord(bytes, schema) + .fold( + f => Map("DECODE-FAILED" -> f.toString), + r => schema.getFields.asScala.map(f => f.name -> String.valueOf(r.get(f.pos))).toMap, + ) + +end DivergentCodecs + +// ---- shape 1b: a COMPUTED schema field BETWEEN the case fields ------------------------------ +// schema {a, computed, b, c} vs case class {a, b, c}. The reporter's motivating case (issue #95). +final case class Three(a: String, b: String, c: Long) + +object Three: + + given VCodec[Three] = VCodec.record("Three", "eo.hazard") { f => + ( + f("a", _.a), + f("computed", (t: Three) => t.a + "|" + t.b), + f("b", _.b), + f("c", _.c), + ).mapN((a, _, b, c) => Three(a, b, c)) + } + +/** NamedTuple codec for the `.fields(_.a, _.b)` shape — kindlings-derived, since there is no + * `vulcan.Codec` for a NamedTuple. Its own schema is 1:1 with the tuple; only the PARENT (`Three`) + * diverges, which is exactly the shape that corrupts two slots per grouped write. + */ +object ThreeNt: + + type AB = NamedTuple.NamedTuple[("a", "b"), (String, String)] + + given AvroEncoder[AB] = AvroEncoder.derived + given AvroDecoder[AB] = AvroDecoder.derived + given AvroSchemaFor[AB] = AvroSchemaFor.derived + +end ThreeNt + +// ---- shape 2: schema field ORDER reversed, both fields String -------------------------------- +// Equal arity and both leaves the same type, so nothing about the shape or a decode failure can +// betray the mis-aim: only the NAMES can. +final case class Pair(alpha: String, beta: String) + +object Pair: + + given VCodec[Pair] = VCodec.record("Pair", "eo.hazard") { f => + (f("beta", _.beta), f("alpha", _.alpha)).mapN((b, a) => Pair(a, b)) + } + +// ---- tier-2 shape: a name TRANSFORM *and* a computed field ----------------------------------- +// schema {click_id, computed, landing_page_id} vs case class {clickId, landingPageId}. +// No literal Scala name survives in the schema, so exact-name resolution alone cannot see it. +final case class SnakeComputed(clickId: String, landingPageId: Long) + +object SnakeComputed: + + given VCodec[SnakeComputed] = VCodec.record("SnakeComputed", "eo.hazard") { f => + ( + f("click_id", _.clickId), + f("computed", (s: SnakeComputed) => s.clickId + "!"), + f("landing_page_id", _.landingPageId), + ).mapN((c, _, l) => SnakeComputed(c, l)) + } + +// ---- shape 7c / 11: the INNER record carries the computed field ------------------------------ +// The outer record is 1:1; the divergence is one hop down, and reachable both by `.field(_.inner)` +// and by a traversal suffix (`.field(_.items).each`). +final case class Inner(x: String, y: Long) + +object Inner: + + given VCodec[Inner] = VCodec.record("Inner", "eo.hazard") { f => + ( + f("c", (i: Inner) => i.x + "!"), + f("x", _.x), + f("y", _.y), + ).mapN((_, x, y) => Inner(x, y)) + } + +final case class Outer(id: String, inner: Inner) + +object Outer: + + given VCodec[Outer] = VCodec.record("Outer", "eo.hazard") { f => + (f("id", _.id), f("inner", _.inner)).mapN(Outer.apply) + } + +final case class Basket(items: List[Inner]) + +object Basket: + + given VCodec[Basket] = VCodec.record("Basket", "eo.hazard") { f => + f("items", _.items).map(Basket.apply) + } + +// ---- FALSE-POSITIVE CONTROL: vulcan rename map only, ORDER PRESERVED ------------------------- +// Issue #35's population. The names carry no recoverable relation to the Scala names, so only +// POSITION can resolve them; any mechanism that breaks this row has broken #35's motivating case. +final case class NPair(alpha: String, beta: String) + +object NPair: + + given VCodec[NPair] = VCodec.record("NPair", "eo.hazard") { f => + (f("alpha_name", _.alpha), f("beta_name", _.beta)).mapN(NPair.apply) + } + +/** FALSE-POSITIVE CONTROL — a kindlings-derived codec under a deliberately hostile custom name + * transform (`n => "x_" + n.reverse`). The #35 worst case: the schema names bear no recoverable + * relation to the Scala names, so position must stay in charge. + */ +object HostileTransform: + + given AvroConfig = AvroConfig().withTransformFieldNames(n => "x_" + n.reverse) + + final case class Hostile(alpha: String, beta: Long) + + object Hostile: + given AvroEncoder[Hostile] = AvroEncoder.derived + given AvroDecoder[Hostile] = AvroDecoder.derived + given AvroSchemaFor[Hostile] = AvroSchemaFor.derived + +end HostileTransform + +// ---- PINNED REGRESSION shape: a PERMUTING rename --------------------------------------------- +// The codec writes case field `alpha` into the schema field literally named "beta" and vice versa. +// Position happens to be right here, and any name-first rule is wrong. No name TRANSFORM can +// produce this shape (a transform is a function of the name alone); only a hand-written field list +// can. +final case class PermPair(alpha: String, beta: String) + +object PermPair: + + given VCodec[PermPair] = VCodec.record("PermPair", "eo.hazard") { f => + (f("beta", _.alpha), f("alpha", _.beta)).mapN((a, b) => PermPair(a, b)) + } + +// ---- PINNED RESIDUAL shape 5a: a vulcan override map AND a reorder --------------------------- +// Equal arity, no name hit, so neither the names nor the shape can see the mis-aim. +final case class RPair(alpha: String, beta: String) + +object RPair: + + given VCodec[RPair] = VCodec.record("RPair", "eo.hazard") { f => + (f("beta_name", _.beta), f("alpha_name", _.alpha)).mapN((b, a) => RPair(a, b)) + } diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala new file mode 100644 index 00000000..2474daad --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala @@ -0,0 +1,361 @@ +package dev.constructive.eo.avro.vulcan + +import scala.jdk.CollectionConverters.* +import scala.language.implicitConversions +import scala.util.control.NonFatal + +import _root_.vulcan.Codec as VCodec +import cats.syntax.all.* +import dev.constructive.eo.avro.{codecPrism, AvroCodec} +import hearth.kindlings.avroderivation.{AvroConfig, AvroDecoder, AvroEncoder, AvroSchemaFor} +import org.apache.avro.Schema +import org.apache.avro.generic.GenericRecord +import org.specs2.mutable.Specification + +// ============================================================================================== +// LEGITIMATE, CURRENTLY-WORKING user code. Every fixture in this file is code a real user can +// write today and get CORRECT behaviour from. It is the counterweight to the repro corpus: a +// resolver that fixes the hazard by refusing or re-aiming ordinary codecs has not fixed anything. +// Any cell that stops being CORRECT here is a FALSE POSITIVE (a loud refusal on working code) or +// an INTRODUCED CORRUPTION (a silently different slot) — the latter is what killed the per-field +// nominal rung, which resolved FP2 below onto the wrong column. +// ============================================================================================== + +// ---- FP1: abbreviated schema names + a TRAILING server-added column --------------------------- +// The single most common hand-written Avro shape: short wire names, plus one derived/audit field +// the case class does not carry. Arity 3 vs 2, and no schema name resembles a Scala name. Both +// leaves resolve by position and are CORRECT, because the extra field is at the TAIL and shifts +// nothing — which is why a global arity gate was rejected: it refuses this. +final case class FpClick(clickId: String, landingPageId: Long) + +object FpClick: + + given VCodec[FpClick] = VCodec.record("FpClick", "eo.fp") { f => + ( + f("id", _.clickId), + f("lp", _.landingPageId), + f("ingested_at", (c: FpClick) => c.clickId.length.toLong), + ).mapN((i, l, _) => FpClick(i, l)) + } + +// ---- FP2: legacy column names that COLLIDE with a sibling's Scala name ------------------------- +// Arity 2 vs 2, order preserved, so position is right. `userId` is written to `uid`; `user_id` is +// a DIFFERENT, older column holding `user`. A per-field nominal rung matches `userId` against +// `user_id` and lands on the wrong column — an INTRODUCED corruption. The all-or-nothing rule +// abstains instead, because `user` maps to no schema field so the mapping is not total. +final case class FpVisit(userId: String, user: String) + +object FpVisit: + + given VCodec[FpVisit] = VCodec.record("FpVisit", "eo.fp") { f => + (f("uid", _.userId), f("user_id", _.user)).mapN(FpVisit.apply) + } + +// ---- FP3: the codec DROPS a derived/transient field -------------------------------------------- +// "don't serialise the cache" — schema arity 2, case arity 3, abbreviated names. +final case class FpRow(id: String, name: String, cachedHash: Long) + +object FpRow: + + given VCodec[FpRow] = VCodec.record("FpRow", "eo.fp") { f => + (f("i", _.id), f("n", _.name)).mapN((i, n) => FpRow(i, n, 0L)) + } + +// ---- FP4 (control): vulcan rename map only, order preserved ------------------------------------ +final case class FpKeep(alpha: String, beta: String) + +object FpKeep: + + given VCodec[FpKeep] = VCodec.record("FpKeep", "eo.fp") { f => + (f("alpha_name", _.alpha), f("beta_name", _.beta)).mapN(FpKeep.apply) + } + +// ---- FP5: `.each.field` under an element codec with a trailing computed field ------------------- +final case class FpItem(sku: String, qty: Long) + +object FpItem: + + given VCodec[FpItem] = VCodec.record("FpItem", "eo.fp") { f => + ( + f("s", _.sku), + f("q", _.qty), + f("chk", (i: FpItem) => i.sku.length.toLong), + ).mapN((s, q, _) => FpItem(s, q)) + } + +final case class FpCart(owner: String, items: List[FpItem]) + +object FpCart: + + given VCodec[FpCart] = VCodec.record("FpCart", "eo.fp") { f => + (f("owner", _.owner), f("items", _.items)).mapN(FpCart.apply) + } + +// ---- FP6: nested record whose INNER codec carries a trailing computed field --------------------- +final case class FpInner(x: String, y: Long) + +object FpInner: + + given VCodec[FpInner] = VCodec.record("FpInner", "eo.fp") { f => + ( + f("x0", _.x), + f("y0", _.y), + f("digest", (i: FpInner) => i.x + "!"), + ).mapN((x, y, _) => FpInner(x, y)) + } + +final case class FpOuter(id: String, inner: FpInner) + +object FpOuter: + + given VCodec[FpOuter] = VCodec.record("FpOuter", "eo.fp") { f => + (f("id", _.id), f("inner", _.inner)).mapN(FpOuter.apply) + } + +// ---- kindlings-derived controls: the issue-#35 population ---------------------------------------- +object FpSnake: + + given AvroConfig = AvroConfig().withSnakeCaseFieldNames + + final case class SnakeClick(clickId: String, landingPageId: Long) + + object SnakeClick: + given AvroEncoder[SnakeClick] = AvroEncoder.derived + given AvroDecoder[SnakeClick] = AvroDecoder.derived + given AvroSchemaFor[SnakeClick] = AvroSchemaFor.derived + +end FpSnake + +object FpHostile: + + // Deliberately unrecognisable, letter-scrambling transform: the #35 worst case. + given AvroConfig = AvroConfig().withTransformFieldNames(n => "x_" + n.reverse) + + final case class HostileClick(clickId: String, landingPageId: Long) + + object HostileClick: + given AvroEncoder[HostileClick] = AvroEncoder.derived + given AvroDecoder[HostileClick] = AvroDecoder.derived + given AvroSchemaFor[HostileClick] = AvroSchemaFor.derived + +end FpHostile + +object FpPlain: + + final case class Bag(tags: Map[String, Long], total: Int) + + object Bag: + given AvroEncoder[Bag] = AvroEncoder.derived + given AvroDecoder[Bag] = AvroDecoder.derived + given AvroSchemaFor[Bag] = AvroSchemaFor.derived + +end FpPlain + +/** The false-positive tripwire for schema-field resolution (issue #95). 28 cells, each a + * legitimate call site that behaves CORRECTLY on published 0.15.1; the whole safety argument for + * changing the resolver is that this scorecard does not move. + * + * Verdicts are scored against SLOT TRUTH — which schema slot a write actually touched — so a + * write that lands on the wrong column is distinguishable from one that lands on the right column + * and leaves a derived sibling stale. + * + * One cell is expected to change: `hatch-record-probe-absent`, where `.fieldNamed` on a name the + * reader schema does not carry used to return `None` at runtime and is now refused at + * construction. That is the deliberate behaviour change; everything else must stay CORRECT. + */ +class ResolutionFalsePositiveSpec extends Specification: + + private val verdicts = scala.collection.mutable.LinkedHashMap.empty[String, String] + private val report = scala.collection.mutable.ListBuffer.empty[String] + + private def slots(bytes: Array[Byte], schema: Schema): Map[String, String] = + AvroCodec + .decodeRecord(bytes, schema) + .fold( + f => Map("DECODE-FAILED" -> f.toString), + r => schema.getFields.asScala.map(f => f.name -> String.valueOf(r.get(f.pos))).toMap, + ) + + /** Which top-level schema slots a write actually touched. Never `copy(...)`, which cannot + * distinguish a wrong slot from a stale derived one. + */ + private def changed(before: Array[Byte], after: Array[Byte], schema: Schema): String = + val b = slots(before, schema) + val a = slots(after, schema) + val d = b.keySet.filter(k => b(k) != a.getOrElse(k, "")).toList.sorted + if d.isEmpty then "" else d.mkString(",") + + private def bytesOf[A](a: A)(using c: AvroCodec[A]): Array[Byte] = + AvroCodec.encodeValue(a).fold(f => throw new RuntimeException(f.toString), identity) + + private def cell(id: String, expect: String)(thunk: => String): Unit = + val got = + try thunk + catch case NonFatal(t) => "LOUD:" + t.getMessage.take(240).replace('\n', ' ') + val verdict = + if got == expect then "CORRECT" + else if got.startsWith("LOUD") then "LOUD-REFUSAL" + else if got == "None" || got == "" then "SILENT-MISS" + else "SILENT-WRONG" + verdicts += (id -> verdict) + report += s"FP-PROBE\t$id\t$verdict\texpect=$expect\tgot=$got" + + // ---- FP1 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpClick]] + val v = FpClick("C0", 7L) + val b = bytesOf(v) + cell("fp1-read-clickId", "Some(C0)")(codecPrism[FpClick].field(_.clickId).getOption(b).toString) + cell("fp1-read-lp", "Some(7)")(codecPrism[FpClick].field(_.landingPageId).getOption(b).toString) + cell("fp1-write-clickId", "id")( + changed(b, codecPrism[FpClick].field(_.clickId).replace("C1")(b), c.schema) + ) + cell("fp1-write-lp", "lp")( + changed(b, codecPrism[FpClick].field(_.landingPageId).replace(99L)(b), c.schema) + ) + cell("fp1-record-read-clickId", "Some(C0)") { + val rec = c.encode(v).asInstanceOf[GenericRecord] + codecPrism[FpClick].field(_.clickId).record.getOptionUnsafe(rec).toString + } + } + + // ---- FP2 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpVisit]] + val b = bytesOf(FpVisit("U0", "NAME0")) + cell("fp2-read-userId", "Some(U0)")(codecPrism[FpVisit].field(_.userId).getOption(b).toString) + cell("fp2-read-user", "Some(NAME0)")(codecPrism[FpVisit].field(_.user).getOption(b).toString) + cell("fp2-write-userId", "uid")( + changed(b, codecPrism[FpVisit].field(_.userId).replace("U1")(b), c.schema) + ) + cell("fp2-write-user", "user_id")( + changed(b, codecPrism[FpVisit].field(_.user).replace("NAME1")(b), c.schema) + ) + } + + // ---- FP3 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpRow]] + val b = bytesOf(FpRow("I0", "N0", 5L)) + cell("fp3-read-id", "Some(I0)")(codecPrism[FpRow].field(_.id).getOption(b).toString) + cell("fp3-read-name", "Some(N0)")(codecPrism[FpRow].field(_.name).getOption(b).toString) + cell("fp3-write-id", "i")(changed(b, codecPrism[FpRow].field(_.id).replace("I1")(b), c.schema)) + cell("fp3-write-name", "n")( + changed(b, codecPrism[FpRow].field(_.name).replace("N1")(b), c.schema) + ) + } + + // ---- FP4 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpKeep]] + val b = bytesOf(FpKeep("A0", "B0")) + cell("fp4-read-alpha", "Some(A0)")(codecPrism[FpKeep].field(_.alpha).getOption(b).toString) + cell("fp4-write-alpha", "alpha_name")( + changed(b, codecPrism[FpKeep].field(_.alpha).replace("A1")(b), c.schema) + ) + } + + // ---- FP5 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpCart]] + val v = FpCart("ada", List(FpItem("S0", 1L), FpItem("S1", 2L))) + val b = bytesOf(v) + cell("fp5-each-read-sku", "Some(Vector(S0, S1))") { + val rec = c.encode(v).asInstanceOf[GenericRecord] + codecPrism[FpCart].field(_.items).each.field(_.sku).record.getAll(rec).toOption.toString + } + cell("fp5-each-write-sku", "items") { + changed(b, codecPrism[FpCart].field(_.items).each.field(_.sku).modify(_ + "!")(b), c.schema) + } + } + + // ---- FP6 ------------------------------------------------------------------------------------ + locally { + val c = summon[AvroCodec[FpOuter]] + val b = bytesOf(FpOuter("O0", FpInner("X0", 3L))) + cell("fp6-read-inner-x", "Some(X0)")( + codecPrism[FpOuter].field(_.inner).field(_.x).getOption(b).toString + ) + cell("fp6-write-inner-x", "inner")( + changed(b, codecPrism[FpOuter].field(_.inner).field(_.x).replace("X1")(b), c.schema) + ) + } + + // ---- kindlings controls ----------------------------------------------------------------------- + locally { + import FpSnake.* + val c = summon[AvroCodec[SnakeClick]] + val b = bytesOf(SnakeClick("C0", 7L)) + cell("ctl-snake-read", "Some(C0)")( + codecPrism[SnakeClick].field(_.clickId).getOption(b).toString + ) + cell("ctl-snake-write", "click_id")( + changed(b, codecPrism[SnakeClick].field(_.clickId).replace("C1")(b), c.schema) + ) + } + + locally { + import FpHostile.* + val c = summon[AvroCodec[HostileClick]] + val b = bytesOf(HostileClick("C0", 7L)) + cell("ctl-hostile-read", "Some(C0)")( + codecPrism[HostileClick].field(_.clickId).getOption(b).toString + ) + cell("ctl-hostile-write", "x_dIkcilc")( + changed(b, codecPrism[HostileClick].field(_.clickId).replace("C1")(b), c.schema) + ) + } + + // ---- map keys and the `.fieldNamed` hatch -------------------------------------------------- + locally { + import FpPlain.* + val v = Bag(Map("t1" -> 1L, "t2" -> 2L), 9) + val b = bytesOf(v) + val bagRec = summon[AvroCodec[Bag]].encode(v).asInstanceOf[GenericRecord] + // Map KEYS are data, not schema fields — `.fieldNamed` is the only way to address one, and the + // existence check must carve them out entirely. + cell("hatch-map-key-present", "Some(1)")( + codecPrism[Bag].field(_.tags).fieldNamed[Long]("t1").record.getOptionUnsafe(bagRec).toString + ) + cell("hatch-map-key-absent", "None")( + codecPrism[Bag].field(_.tags).fieldNamed[Long]("t9").record.getOptionUnsafe(bagRec).toString + ) + cell("hatch-map-key-write", "{t2=2, t1=9}") { + val out = codecPrism[Bag] + .field(_.tags) + .fieldNamed[Long]("t1") + .record + .placeUnsafe(9L)(bagRec) + .asInstanceOf[GenericRecord] + String.valueOf(out.get("tags")) + } + cell("hatch-record-probe-present", "Some(9)")( + codecPrism[Bag].fieldNamed[Int]("total").getOption(b).toString + ) + // The ONE deliberate change: a record-level `.fieldNamed` for a name the reader schema does not + // carry. `None` on 0.15.1 (the shape the hatch exists to avoid), a construction-time refusal + // after commit C. + cell("hatch-record-probe-absent", "None")( + codecPrism[Bag].fieldNamed[Long]("no_such_field").getOption(b).toString + ) + } + + private lazy val scorecard: String = + verdicts.values.groupBy(identity).map((k, v) => s"$k=${v.size}").toList.sorted.mkString(" ") + + "TRIPWIRE: no legitimate call site is silently mis-targeted" >> { + report.foreach(println) + println(s"FP-PROBE\tSCORECARD\t${verdicts.size} cells\t$scorecard") + verdicts.collect { case (id, "SILENT-WRONG") => id }.toList === Nil + } + + "TRIPWIRE: no legitimate call site silently stops resolving" >> { + verdicts.collect { case (id, "SILENT-MISS") => id }.toList === Nil + } + + "every cell keeps the verdict it had on published 0.15.1" >> { + (verdicts.size === 28) + .and(verdicts.collect { case (id, v) if v != "CORRECT" => id }.toList === Nil) + } + +end ResolutionFalsePositiveSpec From 39e2d862a37c01641c9559682ce881cebde8f200 Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 09:49:48 +0200 Subject: [PATCH 2/6] docs(avro): state the resolution rule, its precondition, and what it misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scaladoc that shipped with issue #35 was wrong in three ways, and each one is a reason the hazard in #95 went unnoticed for a release: - It warned only about schema field ORDER divergence. Order is not the shape that bit: a COMPUTED/derived schema column with no case-class parameter (or a dropped one) shifts every later slot on a codec whose field order is perfectly sensible. - It never said the rule governs `.fields`, `selectDynamic` or `.each.field`. They share one resolver, and a grouped `.fields` write on a divergent parent damaged two slots and silently no-op'd a third. - It advertised kebab-case as a supported transform. Kebab cannot produce a legal Avro schema at all — `SchemaParseException: Illegal character in: click-id` — so the claim was never true. Rewritten to state the actual ladder (name-when-total, else position), the precondition the positional rung needs, the three shapes that stay wrong after it, and the one accepted behaviour change. Same pass over the docs site's "Field navigation is by SCHEMA name" section and `AvroWalk`'s internal banner, plus a CHANGELOG entry carrying the recompile-not-rejar note. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- CHANGELOG.md | 36 +++++++++++ .../dev/constructive/eo/avro/AvroPrism.scala | 41 +++++++++--- .../dev/constructive/eo/avro/AvroWalk.scala | 15 ++--- .../eo/avro/vulcan/DivergentCodecs.scala | 8 +-- .../vulcan/ResolutionFalsePositiveSpec.scala | 12 ++-- site/docs/integrations/avro.md | 62 ++++++++++++++----- 6 files changed, 134 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a61143a1..4f8503a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,42 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- **`.field(_.x)` no longer targets the wrong schema field when the codec's schema is not + positionally 1:1 with the case class** (#95): resolution was `fields.get(declIdx).name` and + nothing else — the field's NAME, its TYPE and the record's ARITY were never consulted. Sound for + every kindlings-derived codec (1:1 by construction), unsound for a hand-written or `vulcan.Codec` + field list, which can add a computed column, drop one, or reorder. On those, `.field(_.b)` against + a schema `{a, computed, b, c}` read and wrote `computed`, producing **valid Avro bytes with wrong + content** — and no law could see it, because a mis-targeted optic is a perfectly lawful `Optional` + onto the wrong field. The same resolver backs `.fields`, `selectDynamic`, `.each.field` and + `.each.fields`, so all six sites were affected. Resolution now tries a NAME rung first, and only + when the codec has named the WHOLE case-field list onto distinct schema fields (total and + injective, exact or up to `_`/`-`/`.` and case); otherwise it abstains and declaration position + decides exactly as before — which is what keeps every name-transform codec (issue #35's + population) resolving correctly. Still construction-time only: zero per-operation cost. + +### Changed + +- **Behaviour change, avro**: a hand-written codec that PERMUTES the Scala names (writes case field + `a` into a schema field literally named `b`, and vice versa) resolved correctly by position and + now resolves by name, i.e. wrongly. No name transform can produce that shape — a transform is a + function of the name alone — but a hand-written field list can. Use `.fieldNamed` there. +- **Recompile, do not re-jar**: the avro resolution signatures are `private[avro]`, but + `transparent inline` bakes the accessor into CALLER bytecode, so downstream projects must + recompile against this release rather than swapping the jar. + +### Known limitations + +Three codec shapes are still resolved to the wrong schema field, unchanged from 0.15.1 and pinned +as executable examples in `ResolutionResidualSpec`: a field list that both renames beyond +recognition and reorders; a schema column that bears a case field's name but holds a different +value (a derived public id, a stale legacy column); and two columns whose names normalise alike. +`.fieldNamed("schema_name")` reaches all of them. + ## [0.15.1] - 2026-08-20 ### Fixed diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala index 8e067aca..aa817c09 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala @@ -42,15 +42,38 @@ import org.apache.avro.Schema * drilled-prism given is the evidence that instantiates their `T` at `Array[Byte]`. See the * docs page's migration recipe for the runnable shape. * - * '''Field navigation honours the SCHEMA field name (issue #35).''' `.field(_.x)` (and `.fields`, - * `selectDynamic`, the traversal siblings) resolve the case-class field `x` to whatever schema - * field the codec actually emitted for it — under any name transform (kindlings snake / kebab / - * custom `transformFieldNames`, or a vulcan per-field override map) — by DECLARATION POSITION: the - * i-th case field maps to the i-th schema field, read back out of the cached schema at - * construction time (zero per-operation cost). The rare hand-written codec whose schema field - * ORDER diverges from declaration order needs [[AvroPrism.fieldNamed]]`("schema_name")` to - * navigate by the explicit schema name instead. Map keys are data, not schema-named fields, and - * keep their literal key. + * '''Field navigation honours the SCHEMA field name (issues #35, #95).''' `.field(_.x)` — and + * equally `.fields(...)`, `selectDynamic` and the `.each.field` / `.each.fields` traversal + * siblings, which all share one resolver — maps the case-class field `x` to whatever schema field + * the codec actually emitted for it. Resolution happens ONCE, at prism construction, off the + * cached schema (zero per-operation cost), by two rungs: + * + * 1. '''By NAME, all-or-nothing.''' If EVERY case field of the parent maps to a DISTINCT schema + * field — exactly, or uniquely up to `_` / `-` / `.` and case — then the codec has named the + * whole correspondence and `x`'s answer is read off that map. Partial or colliding coverage + * is treated as no signal at all and the rung abstains for every field, because one lucky + * name match on a schema whose OTHER columns are legacy is how a working call site gets + * re-aimed at the wrong column. + * 1. '''By DECLARATION POSITION''' — the i-th case field is the i-th schema field. This is where + * a name transform lands (a kindlings snake-case config, a custom `transformFieldNames`, a + * vulcan per-field override map), because a transform REMOVES the literal Scala name by + * construction, so rung 1 cannot have fired. + * + * '''The positional rung is only right when the codec's schema is positionally 1:1 with the case + * class.''' Kindlings-derived codecs are, by construction. A hand-written or `vulcan.Codec` field + * list need not be: a COMPUTED/derived schema column, a dropped field, or a reordered field list + * all break it. The name rung recovers most of that population; what it cannot recover is a field + * list that both renames beyond recognition AND reorders (equal arity, no name hit) — that + * resolves by position, silently, and is wrong. Two more shapes stay wrong for the same reason: a + * schema column that BEARS a case field's name but HOLDS a different value (a derived public id, a + * stale legacy column), and two columns whose names normalise alike. Navigate all of them with + * [[AvroPrism.fieldNamed]]`("schema_name")`, which bypasses resolution entirely and is itself + * checked against the schema; `ResolutionResidualSpec` pins each shape's exact behaviour. + * + * Behaviour change against 0.15.1, for the release notes: a hand-written codec that PERMUTES the + * Scala names (writes case field `a` into a schema field literally named `b`, and vice versa) + * resolved correctly by position and now resolves by name, i.e. wrongly. No name transform can + * produce that shape. Map keys are data, not schema-named fields, and keep their literal key. * * Two sibling surfaces, one mechanism each (deliberately NOT duplicated here): * diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala index 2c061148..26c0c79b 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala @@ -334,15 +334,16 @@ private[avro] object AvroWalk: loop(0) fresh - // ---- Schema-name resolution (issue #35) ---------------------------- + // ---- Schema-name resolution (issues #35, #95) ---------------------- // // Field navigation must honour the schema's field name, not the raw Scala field name: a codec - // built with a name transform (kindlings snake/kebab/custom) or vulcan overrides emits schema - // fields whose names differ from the case-class fields. The `.field(_.x)` macros know `x`'s - // DECLARATION index; these helpers walk the cached schema at prism-construction time and read - // back the actual schema field name at that position, so the stored PathStep.Field carries the - // schema name and the (unchanged) runtime walkers hit it. Resolution is construction-time only — - // zero per-operation cost. + // built with a name transform (a kindlings snake-case or custom config) or vulcan overrides emits + // schema fields whose names differ from the case-class fields. The `.field(_.x)` macros know + // `x`'s DECLARATION index AND the parent's whole case-field list; these helpers walk the cached + // schema at prism-construction time and read back the actual schema field name — by NAME when the + // codec named the whole case-field list (issue #95), else by that declaration position (issue + // #35). The stored PathStep.Field carries the schema name and the (unchanged) runtime walkers hit + // it. Resolution is construction-time only — zero per-operation cost. /** Walk `root` along `steps` and return the terminal schema, or a diagnostic. The Field steps * carry already-resolved schema names, so `getField` hits; UnionBranch unwraps to the branch, diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala index 8cb9aa80..91e0a7a6 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala @@ -13,10 +13,10 @@ import org.apache.avro.generic.GenericRecord * population the position-resolution design (issue #35) silently mis-targets. * * This is the standing gap these fixtures close: every fixture in `AvroSpecFixtures` is - * kindlings-derived, hence 1:1 by construction, so no existing example could have seen the - * hazard. `vulcan.Codec` is the realistic carrier for the divergent shapes: `Codec.record`'s field - * list names each schema field independently of the case field it accesses, so order, arity and - * naming can all diverge — and the bridge (`AvroVulcan.codec`) can recover none of it. + * kindlings-derived, hence 1:1 by construction, so no existing example could have seen the hazard. + * `vulcan.Codec` is the realistic carrier for the divergent shapes: `Codec.record`'s field list + * names each schema field independently of the case field it accesses, so order, arity and naming + * can all diverge — and the bridge (`AvroVulcan.codec`) can recover none of it. * * Evidence rule for every fixture: the payload is produced by the codec ITSELF, so the value in * schema field `f` is by construction whatever the codec decided to write there. That makes SLOT diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala index 2474daad..bce864ed 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala @@ -151,13 +151,13 @@ object FpPlain: end FpPlain -/** The false-positive tripwire for schema-field resolution (issue #95). 28 cells, each a - * legitimate call site that behaves CORRECTLY on published 0.15.1; the whole safety argument for - * changing the resolver is that this scorecard does not move. +/** The false-positive tripwire for schema-field resolution (issue #95). 28 cells, each a legitimate + * call site that behaves CORRECTLY on published 0.15.1; the whole safety argument for changing the + * resolver is that this scorecard does not move. * - * Verdicts are scored against SLOT TRUTH — which schema slot a write actually touched — so a - * write that lands on the wrong column is distinguishable from one that lands on the right column - * and leaves a derived sibling stale. + * Verdicts are scored against SLOT TRUTH — which schema slot a write actually touched — so a write + * that lands on the wrong column is distinguishable from one that lands on the right column and + * leaves a derived sibling stale. * * One cell is expected to change: `hatch-record-probe-absent`, where `.fieldNamed` on a name the * reader schema does not carry used to return `None` at runtime and is now refused at diff --git a/site/docs/integrations/avro.md b/site/docs/integrations/avro.md index 1d015c36..a2fa6794 100644 --- a/site/docs/integrations/avro.md +++ b/site/docs/integrations/avro.md @@ -122,20 +122,54 @@ streetP.record.modifyUnsafe(_.toUpperCase)(stump).asInstanceOf[GenericRecord].ge ## Field navigation is by SCHEMA name — `.fieldNamed` is the escape hatch -`.field(_.x)` (and the `.fields(...)` / dynamic-sugar siblings) -resolve the case-class field `x` to whatever schema field the codec -actually emitted for it — by declaration position: the i-th case -field maps to the i-th schema field, read off the cached schema at -construction time, at zero per-operation cost. That makes -navigation robust under any field-name transform — a kindlings -snake / kebab / custom `transformFieldNames` config, or a vulcan -per-field override map — so a renamed schema field is never a miss -cause on a derived codec. The one shape position resolution cannot -handle is a hand-written codec whose schema field **order** -diverges from case-class declaration order: there, drill with -`.fieldNamed[B]("schema_name")`, which navigates by the explicit -schema name and bypasses position resolution entirely. Map keys are -data, not schema-named fields — they keep their literal key. +`.field(_.x)` — and equally `.fields(...)`, the dynamic sugar and +the `.each.field` traversal siblings, which all share one resolver — +maps the case-class field `x` to whatever schema field the codec +actually emitted for it. Resolution happens once, at prism +construction, off the cached schema, at zero per-operation cost, by +two rungs: + +1. **By name, all-or-nothing.** If every case field of the parent + maps to a distinct schema field — exactly, or uniquely up to + `_` / `-` / `.` and case — the codec has named the whole + correspondence, so `x`'s answer is read off that map. Partial or + colliding coverage is treated as no signal and the rung abstains + for every field: one lucky match on a schema whose other columns + are legacy is how a working call site gets re-aimed at the wrong + column. +2. **By declaration position** — the i-th case field is the i-th + schema field. This is where a name transform lands (a kindlings + snake-case or custom `transformFieldNames` config, a vulcan + per-field override map), because a transform removes the literal + Scala name by construction. + +The positional rung is right exactly when the codec's schema is +**positionally 1:1** with the case class. Kindlings-derived codecs +are, by construction. A hand-written or `vulcan.Codec` field list +need not be — a computed/derived column, a dropped field or a +reordered list all break it — and before the name rung existed +every `.field(_.x)` from the divergence onward read and wrote the +**wrong slot**, silently, on both faces. + +What the name rung still cannot see, and where you must reach for +`.fieldNamed[B]("schema_name")` (which bypasses resolution entirely +and is itself checked against the schema): + +- a field list that both renames beyond recognition **and** + reorders — equal arity, no name hit; +- a schema column that *bears* a case field's name but *holds* a + different value (a derived public id, a stale legacy column); +- two columns whose names normalise alike (`userId_` and + `user_id`), which is ambiguity, hence no signal. + +Behaviour change in this release: a hand-written codec that +*permutes* the Scala names (writes case field `a` into a schema +field literally named `b`, and vice versa) resolved correctly by +position and now resolves by name, i.e. wrongly. No name transform +can produce that shape; a hand-written field list can. + +Map keys are data, not schema-named fields — they keep their +literal key. ## Array indexing From 38b97fbc853f85458a027b5445ef59149573056b Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 09:50:02 +0200 Subject: [PATCH 3/6] test(avro): pin the resolution residuals as executable known limitations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three codec shapes are still resolved to the wrong schema field after the nominal rung, and the decision memo asserted them by INSPECTING the rule rather than running it. This runs them. All eight cells behave exactly as predicted, so the ledger below is measured, not argued: cause 1, no name signal (the names are unrecoverable AND the list is permuted, so the rung abstains and position decides, wrongly) 5a {beta_name, alpha_name} vs {alpha, beta} -> reads beta for alpha b2 {seq_no, event_ts} vs {occurredAt, seqNo} -> reads the sequence number (a timestamp-millis logical type annotates the same physical long, so it cannot disambiguate either) b6 compensating arity: one case field dropped, one computed column added, counts equal, position wrong from the insertion point on at3 `userId_` and `user_id` both normalise to `userid` — ambiguity is no signal, so the rung disqualifies itself and position lands on the stale duplicate cause 2, a MISLEADING name signal (a column BEARS a case field's name but HOLDS a different value; the map is total and injective, so it is trusted) at1 {digest, id, raw_ident, payload} — `id` is a derived public identifier at2 {USER_ID, user_ident, balance} — `USER_ID` is a stale legacy column Both cause-2 cells were wrong on 0.15.1 too (they read `digest` / position 0), with one real cost: the corrupt value is now more PLAUSIBLE, `pub-REAL-ID` rather than a digest length. Note at1's `payload` is FIXED by the change — it read `id` before. Also pins the accepted regression (a PERMUTING rename was right by position and is now wrong by name) and asserts that `.fieldNamed` reaches every residual, which is what the error messages and the docs both point at. Reaching cause 1 needs a differential probe of the codec itself (encode sentinels, observe which slot they land in) or an explicit declaration; no amount of name matching helps. Filed as a follow-up, not attempted here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- .../avro/vulcan/ResolutionResidualSpec.scala | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala new file mode 100644 index 00000000..24f8fb7a --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala @@ -0,0 +1,191 @@ +package dev.constructive.eo.avro.vulcan + +import scala.language.implicitConversions + +import _root_.vulcan.Codec as VCodec +import cats.syntax.all.* +import dev.constructive.eo.avro.codecPrism +import org.apache.avro.{LogicalTypes, Schema} +import org.specs2.mutable.Specification + +// ============================================================================================== +// KNOWN LIMITATIONS, pinned as executable examples rather than prose. Every codec below is still +// resolved to the WRONG schema field after issue #95's fix. They are not regressions — each one is +// wrong on published 0.15.1 too — but the next change to this resolver has to look at them, and a +// reader deciding whether to trust `.field(_.x)` against a hand-written codec deserves the exact +// shapes spelled out. +// +// Two distinct causes, and the distinction matters when choosing a follow-up mechanism: +// +// (1) NO NAME SIGNAL — the schema names are unrecoverable AND the field list is permuted, so the +// nominal rung abstains and position decides, wrongly. Reachable only by a differential probe +// of the codec itself (encode sentinels, observe which slot they land in) or an explicit +// declaration; no amount of name matching helps. +// (2) A MISLEADING NAME SIGNAL — a schema field BEARS a case field's name but HOLDS a different +// value (a derived public id, a stale legacy column). The name map is total and injective, so +// the resolver trusts it. Same verdict as 0.15.1, with one real cost: the corrupt value is +// now more PLAUSIBLE (`pub-REAL-ID` rather than a digest length). +// ============================================================================================== + +// ---- b2: two same-physical-type columns, renamed AND reversed (cause 1) ------------------------ +// A logical type is an annotation on the same physical `long`, so it does not disambiguate. +final case class Ev(occurredAt: Long, seqNo: Long) + +object Ev: + + private val tsLong: Schema = + LogicalTypes.timestampMillis().addToSchema(Schema.create(Schema.Type.LONG)) + + given VCodec[Ev] = VCodec.record("Ev", "eo.residual") { f => + ( + f("seq_no", _.seqNo), + f("event_ts", _.occurredAt, default = None), + ).mapN((s, t) => Ev(t, s)) + } + + def tsSchema: Schema = tsLong + +// ---- b6: COMPENSATING arity — one case field dropped, one computed added (cause 1) ------------- +// 3 case fields, 3 schema fields, so no count discrepancy exists; the transform hides every name; +// and position is wrong from the insertion point on. +final case class Comp(a: String, b: String, c: String) + +object Comp: + + given VCodec[Comp] = VCodec.record("Comp", "eo.residual") { f => + ( + f("x_a", _.a), + f("x_digest", (m: Comp) => m.a + "/" + m.b), + f("x_c", _.c), + ).mapN((a, _, c) => Comp(a, "B-NOT-STORED", c)) + } + +// ---- at1: a schema field literally named after a case field, holding something else (cause 2) -- +// `id` in the SCHEMA is a derived public identifier; the case field `id`'s real value is in +// `raw_ident`. The map {id -> id, payload -> payload} is total and injective, so it is trusted. +final case class Doc(id: String, payload: String) + +object Doc: + + given VCodec[Doc] = VCodec.record("Doc", "eo.residual") { f => + ( + f("digest", (d: Doc) => d.payload.length.toString), + f("id", (d: Doc) => "pub-" + d.id), + f("raw_ident", _.id), + f("payload", _.payload), + ).mapN((_, _, i, p) => Doc(i, p)) + } + +// ---- at2: the same, reached through the separator/case-insensitive rung (cause 2) -------------- +// `USER_ID` is a stale legacy column; the live value is in `user_ident`. +final case class Acct(userId: String, balance: Long) + +object Acct: + + given VCodec[Acct] = VCodec.record("Acct", "eo.residual") { f => + ( + f("USER_ID", (_: Acct) => "STALE-LEGACY"), + f("user_ident", _.userId), + f("balance", _.balance), + ).mapN((_, u, b) => Acct(u, b)) + } + +// ---- at3: TWO schema fields normalise alike — ambiguity is no signal, so position decides ------ +final case class Tw(userId: String, tag: String) + +object Tw: + + given VCodec[Tw] = VCodec.record("Tw", "eo.residual") { f => + ( + f("userId_", (_: Tw) => "STALE-DUP"), + f("user_id", _.userId), + ).mapN((_, u) => Tw(u, "TAG-DEFAULT")) + } + +/** The residual-risk ledger for issue #95's resolution change. Each example asserts what the + * shipped resolver ACTUALLY does on a shape it cannot resolve — so the limitation is a fact under + * test, not a claim in a doc comment. + */ +class ResolutionResidualSpec extends Specification: + + import DivergentCodecs.* + + // ---- cause 1: no name signal, position decides and is wrong ------------------------------- + + "RESIDUAL 5a — a vulcan rename map PLUS a reorder still mis-targets" >> { + // {beta_name, alpha_name} vs {alpha, beta}: equal arity, no recoverable name, permuted. + val bytes = encodeBytes(RPair("ALPHA0", "BETA0")) + codecPrism[RPair].field(_.alpha).getOption(bytes) must beSome("BETA0") + } + + "RESIDUAL b2 — two same-physical-type columns, renamed and reversed, still mis-target" >> { + // `occurredAt` reads back the sequence number. A timestamp-millis logical type annotates the + // same physical `long`, so it cannot disambiguate either. + val bytes = encodeBytes(Ev(1700000000000L, 7L)) + (codecPrism[Ev].field(_.occurredAt).getOption(bytes) must beSome(7L)) + .and(codecPrism[Ev].field(_.seqNo).getOption(bytes) must beSome(1700000000000L)) + .and(Ev.tsSchema.getLogicalType.getName === "timestamp-millis") + } + + "RESIDUAL b6 — compensating arity (one field dropped, one computed added) still mis-targets" >> { + // The field COUNTS agree, so nothing about the shape is suspicious; only `b` is wrong. + val bytes = encodeBytes(Comp("A0", "B0", "C0")) + (codecPrism[Comp].field(_.a).getOption(bytes) must beSome("A0")) + .and(codecPrism[Comp].field(_.b).getOption(bytes) must beSome("A0/B0")) + .and(codecPrism[Comp].field(_.c).getOption(bytes) must beSome("C0")) + } + + "RESIDUAL at3 — two schema fields normalising alike is ambiguity, so position decides" >> { + // `userId_` and `user_id` both normalise to `userid`. An ambiguous name signal is no signal: + // the nominal rung disqualifies itself rather than guessing, and position lands on the stale + // duplicate. A refusal here would be defensible; today it is silent. + val bytes = encodeBytes(Tw("U-REAL", "T0")) + codecPrism[Tw].field(_.userId).getOption(bytes) must beSome("STALE-DUP") + } + + // ---- cause 2: a misleading name signal, trusted because the map is total ------------------- + + "RESIDUAL at1 — a schema field bearing a case field's name but holding a derived value" >> { + // {digest, id, raw_ident, payload}: `id` is a public identifier, the case field's real value is + // in `raw_ident`. Wrong on 0.15.1 too (it read `digest`), but the wrong value is now PLAUSIBLE. + // Note `payload` is FIXED by the change — it read `id` before. + val bytes = encodeBytes(Doc("REAL-ID", "P0")) + (codecPrism[Doc].field(_.id).getOption(bytes) must beSome("pub-REAL-ID")) + .and(codecPrism[Doc].field(_.payload).getOption(bytes) must beSome("P0")) + } + + "RESIDUAL at2 — the same, reached through the separator/case-insensitive rung" >> { + // {USER_ID, user_ident, balance}: `USER_ID` normalises to `userid` and is a stale legacy copy. + val bytes = encodeBytes(Acct("U-REAL", 10L)) + (codecPrism[Acct].field(_.userId).getOption(bytes) must beSome("STALE-LEGACY")) + .and(codecPrism[Acct].field(_.balance).getOption(bytes) must beSome(10L)) + } + + // ---- the one deliberate BEHAVIOUR CHANGE --------------------------------------------------- + + "REGRESSION (accepted) — a PERMUTING rename was right by position and is now wrong by name" >> { + // The codec writes case field `alpha` into the schema field literally NAMED "beta". No name + // transform can produce this shape — a transform is a function of the name alone — but a + // hand-written field list can, and this is the one place the change makes things worse. + val bytes = encodeBytes(PermPair("ALPHA0", "BETA0")) + (codecPrism[PermPair].field(_.alpha).getOption(bytes) must beSome("BETA0")) + .and(codecPrism[PermPair].field(_.beta).getOption(bytes) must beSome("ALPHA0")) + } + + // ---- and the hatch that reaches every one of them ------------------------------------------ + + "every residual above is reachable with `.fieldNamed`, which bypasses resolution entirely" >> { + val rpairBytes = encodeBytes(RPair("ALPHA0", "BETA0")) + val docBytes = encodeBytes(Doc("REAL-ID", "P0")) + val acctBytes = encodeBytes(Acct("U-REAL", 10L)) + val compBytes = encodeBytes(Comp("A0", "B0", "C0")) + (codecPrism[RPair].fieldNamed[String]("alpha_name").getOption(rpairBytes) must beSome("ALPHA0")) + .and(codecPrism[Doc].fieldNamed[String]("raw_ident").getOption(docBytes) must beSome("REAL-ID")) + .and( + codecPrism[Acct].fieldNamed[String]("user_ident").getOption(acctBytes) must beSome("U-REAL") + ) + // `Comp.b` is genuinely not stored, so no name reaches it — the hatch cannot invent data. + .and(codecPrism[Comp].fieldNamed[String]("x_digest").getOption(compBytes) must beSome("A0/B0")) + } + +end ResolutionResidualSpec From 950149111bdb4eaf3df4305860e284b5d0a9af30 Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 09:54:53 +0200 Subject: [PATCH 4/6] fix(avro): check `.fieldNamed` against the schema at construction (#95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.fieldNamed("schema_name")` appended the literal to the path with NO schema lookup at all — `widenPathNamed` was one `widenPathStep(PathStep.Field(name))` — although the prism already holds the schema. A typo, or the Scala field name passed where the schema name was meant, therefore produced a runtime SILENT MISS: reads return `None`, writes hand back the payload unchanged and report success. That is the same failure class the hatch exists to avoid, and it is the failure class every error message in this resolver points AT: "navigate by explicit schema name with .fieldNamed(...)". Sending someone from a loud construction failure to a silent runtime one is the wrong direction. Two deliberate carve-outs: - a MAP parent. `.fieldNamed` is also how a map KEY is addressed, keys are data rather than schema fields, and feature-detecting an absent key is legitimate (`hatch-map-key-present` / `-absent` / `-write` in the false-positive spec pin all three). - an unresolvable parent path. The walk already reports that at runtime, and refusing here would change the meaning of a prism deliberately built against a drifted root schema. Record-level feature detection ("does this schema carry X?") moves to the schema, where it belongs: `codec.schema.getField(name)`. Two existing examples change, both deliberately: - `AvroFieldNamingSpec`'s "a bad explicit .fieldNamed misses (None), it does not corrupt" PINNED the defect. Rewritten to assert the refusal. - `AvroBytesSpec` used `.fieldNamed("name")` on a deliberately narrowed root schema to reach the walker's `PathMissing` arm. The coverage is kept by storing the `PathStep.Field("name")` through the internal constructor that example already uses two lines above. Also amends the normative laws bullet, which promised "structurally drifted payloads Miss silently" without distinguishing PAYLOAD drift (still a runtime miss, still undetectable) from a name absent from the READER schema (now a construction-time refusal on both `.field` and `.fieldNamed`). The false-positive scorecard moves by exactly one cell of 28 — `hatch-record-probe-absent`, None -> refusal — and by no other. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- CHANGELOG.md | 8 +++++ .../dev/constructive/eo/avro/AvroPrism.scala | 33 ++++++++++++------- .../constructive/eo/avro/AvroTraversal.scala | 6 ++-- .../dev/constructive/eo/avro/AvroWalk.scala | 32 ++++++++++++++++++ .../constructive/eo/avro/AvroBytesSpec.scala | 13 +++++--- .../eo/avro/AvroFieldNamingSpec.scala | 12 +++++-- .../vulcan/AvroNominalResolutionSpec.scala | 30 +++++++++++++++++ .../vulcan/ResolutionFalsePositiveSpec.scala | 8 +++-- .../avro/vulcan/ResolutionResidualSpec.scala | 8 +++-- 9 files changed, 126 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f8503a1..ca264e3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 injective, exact or up to `_`/`-`/`.` and case); otherwise it abstains and declaration position decides exactly as before — which is what keeps every name-transform codec (issue #35's population) resolving correctly. Still construction-time only: zero per-operation cost. +- **`.fieldNamed("typo")` is refused at construction instead of silently missing at runtime** + (#95): the explicit-schema-name escape hatch appended the literal with no schema lookup at all, + although the schema was in hand, so a typo — or the Scala field name passed where the schema name + was meant — read `None` and wrote the payload back unchanged while reporting success. That is the + same failure class the hatch exists to avoid, and it is what every "navigate by explicit schema + name with `.fieldNamed`" error message points at. Map parents are carved out: `.fieldNamed` is + also how a map KEY is addressed, and an absent key is data. Feature-detecting a RECORD field is + still available via `codec.schema.getField(name)`. ### Changed diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala index aa817c09..8a3c6550 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrism.scala @@ -98,14 +98,16 @@ import org.apache.avro.Schema * `.replace` onto a span whose current value doesn't decode as `A` — or a `.union[B]` focus * sitting on a different runtime branch — is a Miss pass-through. [[graftBytes]] is the * decode-free write (and the only one that can SWITCH union branches). - * - '''The payload must be encoded under exactly this prism's reader schema.''' The byte walk - * performs no writer/reader schema resolution: structurally drifted payloads Miss silently, - * and a same-typed field REORDER between writer and reader is undetectable from the bytes — - * the walk reads the wrong field with full confidence. Confluent-framed payloads are handled - * by composing [[ConfluentWire.confluent]] (a byte Prism that strips the header, resolves the - * writer schema, and fingerprint-gates) BEFORE this optic — `confluent.andThen(thisWalk)`; - * past a fingerprint mismatch a mixed-schema topic still needs a resolving decode (the record - * face with the right schema per payload). + * - '''The payload must be encoded under exactly this prism's reader schema.''' This is about + * PAYLOAD drift — a name absent from the READER schema is a construction-time refusal on both + * `.field` and `.fieldNamed`, not a runtime miss. The byte walk performs no writer/reader + * schema resolution: structurally drifted payloads Miss silently, and a same-typed field + * REORDER between writer and reader is undetectable from the bytes — the walk reads the wrong + * field with full confidence. Confluent-framed payloads are handled by composing + * [[ConfluentWire.confluent]] (a byte Prism that strips the header, resolves the writer + * schema, and fingerprint-gates) BEFORE this optic — `confluent.andThen(thisWalk)`; past a + * fingerprint mismatch a mixed-schema topic still needs a resolving decode (the record face + * with the right schema per payload). * - Dynamic field sugar is shadowed by real members: an Avro field named like a member of this * class (`record`, `field`, `at`, `union`, `each`, `fields`, …) must be drilled with the * explicit `.field(_.record)` form. @@ -292,6 +294,7 @@ final class AvroPrism[A] private[avro] ( private[avro] def widenPathNamed[B](schemaName: String)(using codecB: AvroCodec[B] ): AvroPrism[B] = + AvroWalk.requireFieldNamed(rootSchemaCached, path, schemaName, "AvroPrism.fieldNamed") widenPathStep[B](PathStep.Field(schemaName)) /** Extend by an array-index step. Used by [[at]]. */ @@ -373,10 +376,16 @@ object AvroPrism: )(using codecB: AvroCodec[B]): AvroPrism[B] = ${ AvroPrismMacro.fieldImpl[A, B]('o, 'selector, 'codecB) } - /** `.fieldNamed[B]("schema_name")` — drill by the EXPLICIT schema field name, bypassing position - * resolution. The escape hatch (issue #35) for a hand-written codec whose schema field order - * diverges from case-class declaration order; the common (derived / order-preserving) codecs - * need `.field(_.x)` instead, which resolves the name for you. + /** `.fieldNamed[B]("schema_name")` — drill by the EXPLICIT schema field name, bypassing + * resolution entirely. The escape hatch for a hand-written codec the resolver cannot read (a + * field list that both renames beyond recognition and reorders, a column bearing another field's + * name, two columns normalising alike); the common (derived / order-preserving / + * name-transformed) codecs need `.field(_.x)` instead, which resolves the name for you. + * + * The name is CHECKED against the schema it will be looked up in, at construction (issue #95): a + * name the record does not carry throws rather than Missing silently at runtime. A MAP parent is + * carved out — `.fieldNamed` is also how a map KEY is addressed, and an absent key is data, not + * a mistake. To feature-detect a RECORD field, ask the schema: `codec.schema.getField(name)`. */ extension [A](o: AvroPrism[A]) diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala index 50be1ca0..cc30ae56 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroTraversal.scala @@ -178,6 +178,7 @@ final class AvroTraversal[A] private[avro] ( private[avro] def widenSuffixNamed[B]( schemaName: String )(using codecB: AvroCodec[B]): AvroTraversal[B] = + AvroWalk.requireFieldIn(suffixParentRecord, schemaName, "AvroTraversal.fieldNamed") widenSuffixStep[B](PathStep.Field(schemaName)) /** The record schema the per-element suffix currently points at: walk the prefix to the array @@ -250,8 +251,9 @@ object AvroTraversal: )(using codecB: AvroCodec[B]): AvroTraversal[B] = ${ AvroPrismMacro.fieldTraversalImpl[A, B]('t, 'selector, 'codecB) } - /** `.fieldNamed[B]("schema_name")` — drill by EXPLICIT schema field name (issue #35 escape - * hatch), the traversal counterpart of [[AvroPrism.fieldNamed]]. + /** `.fieldNamed[B]("schema_name")` — drill by EXPLICIT schema field name, the traversal + * counterpart of [[AvroPrism.fieldNamed]], and checked against the element record the same way + * (issue #95). */ extension [A](t: AvroTraversal[A]) diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala index 26c0c79b..b18e54e7 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala @@ -467,6 +467,38 @@ private[avro] object AvroWalk: ) else fields.get(declIdx).name + /** Validate an EXPLICIT schema field name — the `.fieldNamed` escape hatch — against the record + * it will be looked up in, at CONSTRUCTION time. Without this a typo, or the Scala name passed + * where the schema name was meant, is a runtime SILENT MISS: reads return `None` and writes pass + * the payload through unchanged while reporting success. That is precisely the failure class the + * hatch exists to avoid, and the schema was in hand all along. + * + * Deliberately silent in two cases. A MAP parent: `.fieldNamed` is also how a map KEY is + * addressed, and keys are data, not schema fields — a key absent from the payload is an ordinary + * `None`, and feature-detecting one is legitimate. An unresolvable parent path: the walk already + * reports that at runtime, and refusing here would change the meaning of a prism deliberately + * built against a drifted root schema. + */ + def requireFieldNamed( + root: Schema, + parentPath: Array[PathStep], + schemaName: String, + who: String, + ): Unit = + schemaAt(root, parentPath) match + case Right(parent) => requireFieldIn(parent, schemaName, who) + case Left(_) => () + + /** [[requireFieldNamed]] against an already-resolved parent record — the traversal's entry. */ + def requireFieldIn(parent: Schema, schemaName: String, who: String): Unit = + if parent.getType == Schema.Type.RECORD && parent.getField(schemaName) == null then + throw new IllegalArgumentException( + s"$who('$schemaName'): record '${parent.getFullName}' has no field of that name — " + + parent.getFields.asScala.map(_.name).mkString(", ") + + ". `.fieldNamed` takes the SCHEMA field name; for the case-class field name use" + + " .field(_.x)." + ) + /** ALL-OR-NOTHING nominal resolution: the schema-field position for the case field at `declIdx`, * or `-1` to abstain and let position decide. * diff --git a/avro/src/test/scala/dev/constructive/eo/avro/AvroBytesSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/AvroBytesSpec.scala index 8eaa4485..0dc8cc8a 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/AvroBytesSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/AvroBytesSpec.scala @@ -282,13 +282,18 @@ class AvroBytesSpec extends Specification with ScalaCheck: // `codecPrism` no longer takes an explicit schema (the reader schema always matches the // codec's). To exercise the walker on a ROOT schema that drifted from the codec — bytes under a // narrower schema than `Person`'s — build the prism directly with the internal constructor. - val ageOnlyPrism = - new AvroPrism[Person]( - new AvroFocus.Leaf[Person](Array.empty[PathStep], summon[AvroCodec[Person]]), + // `.fieldNamed("name")` would now be REFUSED at construction (issue #95 — the hatch checks the + // name against the schema it will be looked up in), so the drilled path is stored directly. + val namePathPrism = + new AvroPrism[String]( + new AvroFocus.Leaf[String]( + Array[PathStep](PathStep.Field("name")), + summon[AvroCodec[String]], + ), ageOnlySchema, ) val missingOk = - ageOnlyPrism.fieldNamed[String]("name").sliceBytes(ageOnlyBytes) match + namePathPrism.sliceBytes(ageOnlyBytes) match case Left(failure) => failure === AvroFailure.PathMissing(PathStep.Field("name")) case other => diff --git a/avro/src/test/scala/dev/constructive/eo/avro/AvroFieldNamingSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/AvroFieldNamingSpec.scala index af6754cd..1e3d0c7e 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/AvroFieldNamingSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/AvroFieldNamingSpec.scala @@ -124,8 +124,16 @@ class AvroFieldNamingSpec extends Specification: codecPrism[Click].fieldNamed[String]("click_id").getOption(clickBytes) must beSome("abc") } - "a bad explicit .fieldNamed misses (None), it does not corrupt" >> { - codecPrism[Click].fieldNamed[String]("no_such_field").getOption(clickBytes) must beNone + // This example used to assert `None` — "a bad explicit .fieldNamed misses, it does not corrupt". + // That PINNED the defect (issue #95): a silent miss on a name the reader schema never carried, + // decided at run time although the schema was available at construction. It is now a refusal. + "a bad explicit .fieldNamed is refused at construction, not missed at runtime" >> { + codecPrism[Click].fieldNamed[String]("no_such_field") must + throwAn[IllegalArgumentException].like { + case e => + (e.getMessage must contain("click_id, landing_page_id")) + .and(e.getMessage must contain("no field of that name")) + } } end AvroFieldNamingSpec diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala index cc7e56b5..d3aabfbd 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala @@ -161,4 +161,34 @@ class AvroNominalResolutionSpec extends Specification: .and(after("beta_name") === "BETA0") } + // ---- the `.fieldNamed` escape hatch is itself checked ------------- + // Every error message and every doc paragraph above points at `.fieldNamed`. It appended the + // literal with NO schema lookup at all, although the schema was in hand, so a typo — or the Scala + // name passed where the schema name was meant — read `None` and wrote the payload back unchanged + // while reporting success. That is the same silent-miss class the hatch exists to avoid. + + ".fieldNamed with a name the schema lacks is refused at construction, not missed at runtime" >> { + codecPrism[Three].fieldNamed[String]("nope") must + throwAn[IllegalArgumentException].like { + case e => + (e.getMessage must contain("a, computed, b, c")) + .and(e.getMessage must contain("AvroPrism.fieldNamed")) + .and(e.getMessage must contain(".field(_.x)")) + } + } + + ".fieldNamed on a traversal suffix is checked against the element record" >> { + codecPrism[Basket].field(_.items).each.fieldNamed[String]("nope") must + throwAn[IllegalArgumentException].like { + case e => + (e.getMessage must contain("c, x, y")) + .and(e.getMessage must contain("AvroTraversal.fieldNamed")) + } + } + + ".fieldNamed with a name the schema HAS still builds, and reaches the residuals" >> { + val bytes = encodeBytes(Pair("ALPHA0", "BETA0")) + codecPrism[Pair].fieldNamed[String]("beta").getOption(bytes) must beSome("BETA0") + } + end AvroNominalResolutionSpec diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala index bce864ed..7e031444 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala @@ -353,9 +353,13 @@ class ResolutionFalsePositiveSpec extends Specification: verdicts.collect { case (id, "SILENT-MISS") => id }.toList === Nil } - "every cell keeps the verdict it had on published 0.15.1" >> { + "the ONLY verdict change against 0.15.1 is the `.fieldNamed` existence check" >> { (verdicts.size === 28) - .and(verdicts.collect { case (id, v) if v != "CORRECT" => id }.toList === Nil) + .and( + verdicts.collect { case (id, v) if v != "CORRECT" => id }.toList === + List("hatch-record-probe-absent") + ) + .and(verdicts("hatch-record-probe-absent") === "LOUD-REFUSAL") } end ResolutionFalsePositiveSpec diff --git a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala index 24f8fb7a..220f890e 100644 --- a/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala @@ -180,12 +180,16 @@ class ResolutionResidualSpec extends Specification: val acctBytes = encodeBytes(Acct("U-REAL", 10L)) val compBytes = encodeBytes(Comp("A0", "B0", "C0")) (codecPrism[RPair].fieldNamed[String]("alpha_name").getOption(rpairBytes) must beSome("ALPHA0")) - .and(codecPrism[Doc].fieldNamed[String]("raw_ident").getOption(docBytes) must beSome("REAL-ID")) + .and( + codecPrism[Doc].fieldNamed[String]("raw_ident").getOption(docBytes) must beSome("REAL-ID") + ) .and( codecPrism[Acct].fieldNamed[String]("user_ident").getOption(acctBytes) must beSome("U-REAL") ) // `Comp.b` is genuinely not stored, so no name reaches it — the hatch cannot invent data. - .and(codecPrism[Comp].fieldNamed[String]("x_digest").getOption(compBytes) must beSome("A0/B0")) + .and( + codecPrism[Comp].fieldNamed[String]("x_digest").getOption(compBytes) must beSome("A0/B0") + ) } end ResolutionResidualSpec From 5419f69f0e8fe534dcd66bba6c6142bfa153f20c Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 10:02:31 +0200 Subject: [PATCH 5/6] fix(generics): a nested `.field(_.a.b)` selector must not parse as `b` (#95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MacroSelectors.extractFieldName` — the selector parser shared by every cursor macro — matched `Lambda(_, Select(_, name))` with ANY receiver. A nested path `_.inner.y` is `Select(Select(Ident(_), "inner"), "y")`, so it matched, yielded the bare name `"y"`, and the macro resolved `y` on the PARENT record. Where the parent carries a field of that name — and a record holding a nested record often does — that is not a miss. It is a well-typed, perfectly lawful optic aimed at the wrong field: NOuter(inner: NInner(x, y), y) codecPrism[NOuter].field(_.inner.y).getOption(bytes) // Some("OUTER_Y") codecPrism[NOuter].field(_.inner).field(_.y) // Some("INNER_Y") Silent corruption on a perfectly 1:1, derived codec, with no schema divergence involved at all — a second, independent hazard from the resolution one, and 100% decidable at compile time. The macros' own "nested paths are not yet supported inside a single call; chain them" abort was UNREACHABLE for exactly the shape it was written for. `LensMacro` never had this: it uses the strict `extractSingleFieldName` (which requires the `Select` receiver to be the lambda parameter) plus a `knownFields.contains` check. The cursor macros simply never got the same treatment. Both halves land here: - `extractFieldName` now unwraps `Inlined` / `Typed` around the lambda, around its `Select` body AND around the `Select`'s RECEIVER, then requires that receiver to be an `Ident`. That keeps the wrapper tolerance the cursor macros need (which is the only reason this function exists beside the strict one) and drops the receiver looseness, which was never intentional. - `requireCaseField[A]` aborts when a single-hop selector names something that is not a case field of the parent. Those used to be passed through as a literal field name and miss at runtime; the declaration index comes back `-1` for them, which is also the legitimate "NamedTuple parent" signal, so the two cannot be told apart downstream. They are told apart here. Skipped when `A` has no case fields at all, which is the shape the literal fallback exists for. Five call sites, all covered: `AvroPrismMacro.fieldImpl` / `fieldTraversalImpl`, `JsonPrismMacro.fieldImpl` / `fieldTraversalImpl` (eo-circe), and `JsoniterPrismMacro.fieldName` (eo-jsoniter, shared by prism and traversal). The two traversal messages also gained the "chain them" hint their prism twins already carried. `grep` finds zero nested-selector call sites in the repo, and the circe, jsoniter, avro and generics suites all pass untouched — this closes a hole, it does not move any working code. Tests: `NestedSelectorMacroErrorSpec` in avro, circe and jsoniter — the nested prism selector, the nested traversal selector, the not-a-case-field selector, and a runtime row asserting the CHAINED form reaches the inner field. Every compile-error row produced NO error at all before this change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- CHANGELOG.md | 11 ++ .../constructive/eo/avro/AvroPrismMacro.scala | 6 +- .../avro/NestedSelectorMacroErrorSpec.scala | 103 ++++++++++++++++++ .../eo/circe/JsonPrismMacro.scala | 6 +- .../circe/NestedSelectorMacroErrorSpec.scala | 92 ++++++++++++++++ .../eo/generics/MacroSelectors.scala | 60 ++++++++-- .../eo/jsoniter/JsoniterPrismMacro.scala | 4 +- .../NestedSelectorMacroErrorSpec.scala | 87 +++++++++++++++ 8 files changed, 354 insertions(+), 15 deletions(-) create mode 100644 avro/src/test/scala/dev/constructive/eo/avro/NestedSelectorMacroErrorSpec.scala create mode 100644 circe/src/test/scala/dev/constructive/eo/circe/NestedSelectorMacroErrorSpec.scala create mode 100644 jsoniter/src/test/scala/dev/constructive/eo/jsoniter/NestedSelectorMacroErrorSpec.scala diff --git a/CHANGELOG.md b/CHANGELOG.md index ca264e3c..f0e2ace3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 name with `.fieldNamed`" error message points at. Map parents are carved out: `.fieldNamed` is also how a map KEY is addressed, and an absent key is data. Feature-detecting a RECORD field is still available via `codec.schema.getField(name)`. +- **A nested selector `.field(_.a.b)` is now a compile error** (#95): the selector parser shared by + every cursor macro matched `Lambda(_, Select(_, name))` with ANY receiver, so `_.inner.y` parsed + as the bare name `y` and the macro resolved it on the PARENT. Where the parent carries a field of + that name — and a record holding a nested record often does — the result was a well-typed, + perfectly lawful optic aimed at the wrong field: silent corruption on a 1:1, derived codec, with + no schema divergence involved, and the macros' own "nested paths … chain them" abort unreachable + for exactly the shape it was written for. A single-hop selector naming something that is not a + case field of the parent (a no-arg `def`) is now a compile error too, instead of a literal field + name that misses at runtime. `AvroPrism` / `AvroTraversal`, `JsonPrism` / `JsonTraversal` + (eo-circe) and `JsoniterPrism` / `JsoniterTraversal` (eo-jsoniter) all share the parser, so all + six surfaces are covered. ### Changed diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala index 458a30ad..4f2cd017 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala @@ -25,10 +25,11 @@ object AvroPrismMacro: report.errorAndAbort( "AvroPrism.field: selector must be a single-field accessor like `_.fieldName`.\n" + "Nested paths are not yet supported inside a single call;\n" - + "chain them: `_.field(_.a).field(_.b)`.\n" + + "chain them: `.field(_.a).field(_.b)`.\n" + s"Got: ${selector.asTerm.show}" ) } + MacroSelectors.requireCaseField[A]("AvroPrism.field", name) '{ $parent.widenPath[B]( @@ -209,9 +210,12 @@ object AvroPrismMacro: val name: String = MacroSelectors.extractFieldName(selector.asTerm).getOrElse { report.errorAndAbort( "AvroTraversal.field: selector must be a single-field accessor like `_.fieldName`.\n" + + "Nested paths are not yet supported inside a single call;\n" + + "chain them: `.field(_.a).field(_.b)`.\n" + s"Got: ${selector.asTerm.show}" ) } + MacroSelectors.requireCaseField[A]("AvroTraversal.field", name) '{ $parent.widenSuffix[B]( diff --git a/avro/src/test/scala/dev/constructive/eo/avro/NestedSelectorMacroErrorSpec.scala b/avro/src/test/scala/dev/constructive/eo/avro/NestedSelectorMacroErrorSpec.scala new file mode 100644 index 00000000..5fa98dfb --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/NestedSelectorMacroErrorSpec.scala @@ -0,0 +1,103 @@ +package dev.constructive.eo.avro + +import scala.compiletime.testing.typeCheckErrors +import scala.language.implicitConversions + +import hearth.kindlings.avroderivation.{AvroDecoder, AvroEncoder, AvroSchemaFor} +import org.specs2.mutable.Specification + +// Fixture ADTs at top level (the eo-generics convention for macro-facing test types). The SHAPE is +// the hazard: the inner record and the outer record BOTH carry a field named `y`, so a selector +// that walks one hop too far (`_.inner.y`) parses as the bare name `y` and lands on the OUTER +// record's `y` — a well-typed, lawful optic aimed at the wrong field. +final case class NInner(x: String, y: String) + +object NInner: + given AvroEncoder[NInner] = AvroEncoder.derived + given AvroDecoder[NInner] = AvroDecoder.derived + given AvroSchemaFor[NInner] = AvroSchemaFor.derived + +final case class NOuter(inner: NInner, y: String): + // A no-arg member that is NOT a case field — the second half of the selector rung. `_.hashCode` + // will not do: it is a Java no-arg method, so it eta-expands to `Apply(Select(_, …), Nil)` and is + // already caught by the single-field-accessor rule. + def derivedTag: String = y.toUpperCase + +object NOuter: + given AvroEncoder[NOuter] = AvroEncoder.derived + given AvroDecoder[NOuter] = AvroDecoder.derived + given AvroSchemaFor[NOuter] = AvroSchemaFor.derived + +final case class NBasket(items: List[NInner]) + +object NBasket: + given AvroEncoder[NBasket] = AvroEncoder.derived + given AvroDecoder[NBasket] = AvroDecoder.derived + given AvroSchemaFor[NBasket] = AvroSchemaFor.derived + +/** A NESTED selector must not compile — issue #95's second, independent hazard. + * + * `MacroSelectors.extractFieldName` matched `Lambda(_, Select(_, name))` with ANY receiver, so + * `.field(_.inner.y)` parsed as the single name `"y"` and resolved it on the PARENT record. On + * `NOuter` above that reads and writes the outer `y` while the call site plainly says "the inner + * one" — silent corruption on a perfectly 1:1, kindlings-derived codec, with no schema divergence + * involved at all. The macro's own "nested paths … chain them" abort was unreachable for exactly + * the shape it was written for. + * + * The fix is in the extractor (`extractFieldName` now requires the `Select` receiver to be the + * lambda parameter), so the same row holds for every cursor macro that shares it — eo-circe's + * `JsonPrism` / `JsonTraversal` and eo-jsoniter's `JsoniterPrism` / `JsoniterTraversal` carry the + * mirror of this spec. + * + * The second half of the rung is the known-field check: a single-hop selector that names something + * that is not a case field of the parent (`_.hashCode`) used to be passed through as a literal + * schema-field name and silently Missed at runtime. It is now a compile error too. + */ +class NestedSelectorMacroErrorSpec extends Specification: + + private def anyMatches(msgs: List[String], fragment: String): Boolean = + msgs.exists(_.contains(fragment)) + + "a nested `.field(_.a.b)` selector is a compile error naming the chained form" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.avro.{codecPrism, NOuter} + codecPrism[NOuter].field(_.inner.y) + """).map(_.message) + + (anyMatches(msgs, "AvroPrism.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + .and(anyMatches(msgs, ".field(_.a).field(_.b)") must beTrue) + } + + "a nested selector on a TRAVERSAL suffix is a compile error too" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.avro.{codecPrism, NBasket} + codecPrism[NBasket].field(_.items).each.field(_.x.length) + """).map(_.message) + + (anyMatches(msgs, "AvroTraversal.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + } + + "a single-hop selector that is not a case field is a compile error, not a runtime miss" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.avro.{codecPrism, NOuter} + codecPrism[NOuter].field(_.derivedTag) + """).map(_.message) + + (anyMatches(msgs, "is not a case field of") must beTrue) + .and(anyMatches(msgs, "Known fields: inner, y") must beTrue) + } + + "the CHAINED form is what actually reaches the inner field" >> { + val bytes = AvroCodec + .encodeValue(NOuter(NInner("INNER_X", "INNER_Y"), "OUTER_Y")) + .fold(f => throw new RuntimeException(f.toString), identity) + + (codecPrism[NOuter].field(_.inner).field(_.y).getOption(bytes) must beSome("INNER_Y")) + .and(codecPrism[NOuter].field(_.y).getOption(bytes) must beSome("OUTER_Y")) + } + +end NestedSelectorMacroErrorSpec diff --git a/circe/src/main/scala/dev/constructive/eo/circe/JsonPrismMacro.scala b/circe/src/main/scala/dev/constructive/eo/circe/JsonPrismMacro.scala index 78080325..ec7918ac 100644 --- a/circe/src/main/scala/dev/constructive/eo/circe/JsonPrismMacro.scala +++ b/circe/src/main/scala/dev/constructive/eo/circe/JsonPrismMacro.scala @@ -29,10 +29,11 @@ object JsonPrismMacro: report.errorAndAbort( "JsonPrism.field: selector must be a single-field accessor like `_.fieldName`.\n" + "Nested paths are not yet supported inside a single call;\n" - + "chain them: `_.field(_.a).field(_.b)`.\n" + + "chain them: `.field(_.a).field(_.b)`.\n" + s"Got: ${selector.asTerm.show}" ) } + MacroSelectors.requireCaseField[A]("JsonPrism.field", name) '{ $parent.widenPath[B](${ Expr(name) })(using $encB, $decB) @@ -94,9 +95,12 @@ object JsonPrismMacro: val name: String = MacroSelectors.extractFieldName(selector.asTerm).getOrElse { report.errorAndAbort( "JsonTraversal.field: selector must be a single-field accessor like `_.fieldName`.\n" + + "Nested paths are not yet supported inside a single call;\n" + + "chain them: `.field(_.a).field(_.b)`.\n" + s"Got: ${selector.asTerm.show}" ) } + MacroSelectors.requireCaseField[A]("JsonTraversal.field", name) '{ $parent.widenSuffix[B](${ Expr(name) })(using $encB, $decB) diff --git a/circe/src/test/scala/dev/constructive/eo/circe/NestedSelectorMacroErrorSpec.scala b/circe/src/test/scala/dev/constructive/eo/circe/NestedSelectorMacroErrorSpec.scala new file mode 100644 index 00000000..fba19878 --- /dev/null +++ b/circe/src/test/scala/dev/constructive/eo/circe/NestedSelectorMacroErrorSpec.scala @@ -0,0 +1,92 @@ +package dev.constructive.eo.circe + +import scala.compiletime.testing.typeCheckErrors +import scala.language.implicitConversions + +import hearth.kindlings.circederivation.KindlingsCodecAsObject +import io.circe.Codec +import org.specs2.mutable.Specification + +/** The eo-circe mirror of `avro.NestedSelectorMacroErrorSpec` — the cursor macros share one + * selector parser (`generics.MacroSelectors.extractFieldName`), so the hazard and its fix are + * carrier-wide. + * + * `.field(_.inner.y)` used to parse as the bare name `"y"` and resolve it on the PARENT object, + * because the parser matched a `Select` with any receiver. On the fixture below — where the outer + * and inner objects both carry a `y` — that is a well-typed, lawful optic aimed at the wrong JSON + * key, and the macro's own "nested paths … chain them" abort never fired. + */ +object NestedSelectorMacroErrorSpec: + + case class NInner(x: String, y: String) + + object NInner: + given Codec.AsObject[NInner] = KindlingsCodecAsObject.derived + + case class NOuter(inner: NInner, y: String): + // A no-arg member that is NOT a case field. `_.hashCode` will not do: it is a Java no-arg + // method, so it eta-expands to `Apply(Select(_, …), Nil)` and the single-field-accessor rule + // already catches it. + def derivedTag: String = y.toUpperCase + + object NOuter: + given Codec.AsObject[NOuter] = KindlingsCodecAsObject.derived + + case class NBasket(items: Vector[NInner]) + + object NBasket: + given Codec.AsObject[NBasket] = KindlingsCodecAsObject.derived + +end NestedSelectorMacroErrorSpec + +class NestedSelectorMacroErrorSpec extends Specification: + + private def anyMatches(msgs: List[String], fragment: String): Boolean = + msgs.exists(_.contains(fragment)) + + "a nested `.field(_.a.b)` selector is a compile error naming the chained form" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.circe.codecPrism + import dev.constructive.eo.circe.NestedSelectorMacroErrorSpec.NOuter + codecPrism[NOuter].field(_.inner.y) + """).map(_.message) + + (anyMatches(msgs, "JsonPrism.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + .and(anyMatches(msgs, ".field(_.a).field(_.b)") must beTrue) + } + + "a nested selector on a TRAVERSAL suffix is a compile error too" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.circe.codecPrism + import dev.constructive.eo.circe.NestedSelectorMacroErrorSpec.NBasket + codecPrism[NBasket].field(_.items).each.field(_.x.length) + """).map(_.message) + + (anyMatches(msgs, "JsonTraversal.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + } + + "a single-hop selector that is not a case field is a compile error, not a runtime miss" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.circe.codecPrism + import dev.constructive.eo.circe.NestedSelectorMacroErrorSpec.NOuter + codecPrism[NOuter].field(_.derivedTag) + """).map(_.message) + + (anyMatches(msgs, "is not a case field of") must beTrue) + .and(anyMatches(msgs, "Known fields: inner, y") must beTrue) + } + + "the CHAINED form is what actually reaches the inner field" >> { + import NestedSelectorMacroErrorSpec.* + import io.circe.syntax.* + val json = NOuter(NInner("INNER_X", "INNER_Y"), "OUTER_Y").asJson + + (codecPrism[NOuter].field(_.inner).field(_.y).getOption(json) must beSome("INNER_Y")) + .and(codecPrism[NOuter].field(_.y).getOption(json) must beSome("OUTER_Y")) + } + +end NestedSelectorMacroErrorSpec diff --git a/generics/src/main/scala/dev/constructive/eo/generics/MacroSelectors.scala b/generics/src/main/scala/dev/constructive/eo/generics/MacroSelectors.scala index 1ca78f3f..aff7bd35 100644 --- a/generics/src/main/scala/dev/constructive/eo/generics/MacroSelectors.scala +++ b/generics/src/main/scala/dev/constructive/eo/generics/MacroSelectors.scala @@ -1,5 +1,6 @@ package dev.constructive.eo.generics +import scala.annotation.tailrec import scala.quoted.* /** Quote-context selector-AST helpers shared between [[LensMacro]] and the `JsonPrismMacro` / @@ -8,21 +9,56 @@ import scala.quoted.* */ object MacroSelectors: - /** Loose variant of [[extractSingleFieldName]] — strips `Inlined` / `Typed` wrappers around the - * lambda AND around its `Select` body, and does NOT require the `Select` receiver to be the bare - * lambda parameter. Used by the cursor macros' `.field(_.x)` sugar, whose selectors are always - * single-hop but may arrive wrapped. Kept distinct from [[extractSingleFieldName]] (which - * rejects nested chains by construction) so callers that need the strict form still have it. + /** Wrapper-tolerant variant of [[extractSingleFieldName]] for the cursor macros' `.field(_.x)` + * sugar: it strips `Inlined` / `Typed` around the lambda, around its `Select` body, AND around + * the `Select`'s RECEIVER, which the strict form does not. + * + * '''Receiver-is-the-lambda-parameter is load-bearing (issue #95).''' This used to match + * `Lambda(_, Select(_, name))` with ANY receiver, so a nested path `_.inner.y` — + * `Select(Select(Ident(_), "inner"), "y")` — parsed as the single name `"y"` and every cursor + * macro then resolved `y` on the PARENT. On a record that happens to carry a field of that name + * (and an inner record always might) the result is a well-typed, perfectly lawful optic aimed at + * the wrong field: silent corruption with no schema divergence involved, and the macros' own + * "nested paths … chain them" abort unreachable for exactly the shape it was written for. + * Rejecting the nested receiver here makes that abort fire. */ def extractFieldName(using Quotes)(t: quotes.reflect.Term): Option[String] = import quotes.reflect.* - t match - case Inlined(_, _, inner) => extractFieldName(inner) - case Typed(inner, _) => extractFieldName(inner) - case Lambda(_, Select(_, name)) => Some(name) - case Lambda(_, Inlined(_, _, Select(_, name))) => Some(name) - case Lambda(_, Typed(Select(_, name), _)) => Some(name) - case _ => None + @tailrec def unwrap(term: Term): Term = + term match + case Inlined(_, _, inner) => unwrap(inner) + case Typed(inner, _) => unwrap(inner) + case other => other + unwrap(t) match + case Lambda(_, body) => + unwrap(body) match + case Select(receiver, name) => + unwrap(receiver) match + case Ident(_) => Some(name) + case _ => None + case _ => None + case _ => None + + /** Abort unless `name` is a case field of `A`. + * + * The other half of the cursor macros' selector rung (issue #95): a single-hop selector naming + * something that is not a case field — `_.hashCode`, or a no-arg method on the case class — used + * to be passed through as a LITERAL field name and become a runtime miss on the wire format. The + * declaration index the resolvers ask for comes back `-1` for exactly those, and `-1` is also + * the legitimate "this parent has no case fields" signal, so the two cannot be told apart + * downstream. They are told apart here instead. + * + * Skipped when `A` has no case fields at all (a NamedTuple parent, say), which is the shape the + * literal-name fallback exists for. + */ + def requireCaseField[A: Type](using Quotes)(who: String, name: String): Unit = + import quotes.reflect.* + val known = TypeRepr.of[A].typeSymbol.caseFields.map(_.name) + if known.nonEmpty && !known.contains(name) then + report.errorAndAbort( + s"$who: '$name' is not a case field of ${Type.show[A]}." + + s" Known fields: ${known.mkString(", ")}." + ) /** The single element type of a Scala collection type `A` (via its `Iterable` base type), or * abort with a `who`-tagged error. Shared by the cursor macros' `.at` / `.each` sugar. diff --git a/jsoniter/src/main/scala/dev/constructive/eo/jsoniter/JsoniterPrismMacro.scala b/jsoniter/src/main/scala/dev/constructive/eo/jsoniter/JsoniterPrismMacro.scala index 1e0db481..5b478138 100644 --- a/jsoniter/src/main/scala/dev/constructive/eo/jsoniter/JsoniterPrismMacro.scala +++ b/jsoniter/src/main/scala/dev/constructive/eo/jsoniter/JsoniterPrismMacro.scala @@ -98,7 +98,7 @@ object JsoniterPrismMacro: selector: Expr[A => B], )(using q: Quotes): String = import quotes.reflect.* - MacroSelectors.extractFieldName(selector.asTerm).getOrElse { + val name = MacroSelectors.extractFieldName(selector.asTerm).getOrElse { report.errorAndAbort( s"$who: selector must be a single-field accessor like `_.fieldName`.\n" + "Nested paths are not yet supported inside a single call;\n" @@ -106,6 +106,8 @@ object JsoniterPrismMacro: + s"Got: ${selector.asTerm.show}" ) } + MacroSelectors.requireCaseField[A](who, name) + name /** Shared backbone for the Dynamic sugar — name validation + field lookup live in the shared * `MacroSelectors.caseFieldType` (eo-generics); this stub owns the `JsonValueCodec` summon. diff --git a/jsoniter/src/test/scala/dev/constructive/eo/jsoniter/NestedSelectorMacroErrorSpec.scala b/jsoniter/src/test/scala/dev/constructive/eo/jsoniter/NestedSelectorMacroErrorSpec.scala new file mode 100644 index 00000000..d59e6641 --- /dev/null +++ b/jsoniter/src/test/scala/dev/constructive/eo/jsoniter/NestedSelectorMacroErrorSpec.scala @@ -0,0 +1,87 @@ +package dev.constructive.eo.jsoniter + +import scala.compiletime.testing.typeCheckErrors +import scala.language.implicitConversions + +import com.github.plokhotnyuk.jsoniter_scala.core.{writeToArray, JsonValueCodec} +import com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker +import dev.constructive.eo.optics.Optic.* +import org.specs2.mutable.Specification + +// Fixture ADTs at top level (the convention for macro-facing test types in this module). The outer +// and inner objects both carry a `y`, which is what turns a one-hop-too-far selector into a +// well-typed optic aimed at the wrong JSON key rather than a miss. +final case class NsInner(x: String, y: String) + +final case class NsOuter(inner: NsInner, y: String): + // A no-arg member that is NOT a case field. `_.hashCode` will not do: it is a Java no-arg method, + // so it eta-expands to `Apply(Select(_, …), Nil)` and the single-field-accessor rule catches it. + def derivedTag: String = y.toUpperCase + +final case class NsBasket(items: List[NsInner]) + +object NsCodecs: + given JsonValueCodec[String] = JsonCodecMaker.make + given JsonValueCodec[Int] = JsonCodecMaker.make + given JsonValueCodec[NsInner] = JsonCodecMaker.make + given JsonValueCodec[List[NsInner]] = JsonCodecMaker.make + given JsonValueCodec[NsOuter] = JsonCodecMaker.make + given JsonValueCodec[NsBasket] = JsonCodecMaker.make +end NsCodecs + +/** The eo-jsoniter mirror of `avro.NestedSelectorMacroErrorSpec` — the cursor macros share one + * selector parser (`generics.MacroSelectors.extractFieldName`), so the hazard and its fix are + * carrier-wide. `.field(_.inner.y)` used to parse as the bare name `"y"` and resolve it on the + * PARENT object. + */ +class NestedSelectorMacroErrorSpec extends Specification: + + import NsCodecs.given + + private def anyMatches(msgs: List[String], fragment: String): Boolean = + msgs.exists(_.contains(fragment)) + + "a nested `.field(_.a.b)` selector is a compile error naming the chained form" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.jsoniter.{JsoniterPrism, NsOuter} + import dev.constructive.eo.jsoniter.NsCodecs.given + JsoniterPrism[NsOuter].field(_.inner.y) + """).map(_.message) + + (anyMatches(msgs, "JsoniterPrism.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + .and(anyMatches(msgs, ".field(_.a).field(_.b)") must beTrue) + } + + "a nested selector on a TRAVERSAL suffix is a compile error too" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.jsoniter.{JsoniterPrism, NsBasket} + import dev.constructive.eo.jsoniter.NsCodecs.given + JsoniterPrism[NsBasket].field(_.items).each.field(_.x.length) + """).map(_.message) + + (anyMatches(msgs, "JsoniterTraversal.field") must beTrue) + .and(anyMatches(msgs, "single-field accessor") must beTrue) + .and(anyMatches(msgs, "Nested paths") must beTrue) + } + + "a single-hop selector that is not a case field is a compile error, not a runtime miss" >> { + val msgs = typeCheckErrors(""" + import dev.constructive.eo.jsoniter.{JsoniterPrism, NsOuter} + import dev.constructive.eo.jsoniter.NsCodecs.given + JsoniterPrism[NsOuter].field(_.derivedTag) + """).map(_.message) + + (anyMatches(msgs, "is not a case field of") must beTrue) + .and(anyMatches(msgs, "Known fields: inner, y") must beTrue) + } + + "the CHAINED form is what actually reaches the inner field" >> { + val bytes = writeToArray(NsOuter(NsInner("INNER_X", "INNER_Y"), "OUTER_Y")) + + (JsoniterPrism[NsOuter].field(_.inner).field(_.y).getOption(bytes) must beSome("INNER_Y")) + .and(JsoniterPrism[NsOuter].field(_.y).getOption(bytes) must beSome("OUTER_Y")) + } + +end NestedSelectorMacroErrorSpec From 2e0b6b977da9c9af6b73ef8951650d3242d7d413 Mon Sep 17 00:00:00 2001 From: Rodolfo Hansen Date: Fri, 18 Sep 2026 10:09:53 +0200 Subject: [PATCH 6/6] perf(avro): allocation-free injectivity check in the nominal rung MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolution is construction-time only, but it is not free, and the first cut paid more than it needed to. Measured with the prototype's own harness (best of 5 x 200_000 builds, same box, same session, `ConstructionCostProbe`): base Set[Int] this identity .field(_.name) 84.9 115.2 98.6 ns/build snake .field(_.clickId) 85.1 316.0 264.0 ns/build nested .field(_.meta).field(…) 116.9 462.2 430.3 ns/build Two changes, both free: - `Set[Int]` -> a linear scan over the filled prefix of the result array. The set was allocating a boxed Integer and a new set node per case field, on a list that is a handful of entries; the scan is O(n^2) on an n that is the case-class arity. - An early precondition: a total, injective map from case fields into schema fields cannot exist when the case class has MORE fields than the record, so that shape skips the scan entirely. Same answer, no work. The remaining gap is inherent to the rule: the total-nominal rung resolves EVERY case field before it trusts any one of them, so a snake_case parent runs a normalised name compare per (case field x schema field) pair where position ran none. That is the price of not re-aiming a working call site at a lucky single match, it is paid once per drilled prism at construction, and it stays strictly off the read/write path. Zero per-operation cost re-verified from bytecode rather than asserted: `javap -p -c` over the compiled avro classes puts every reference to `resolveFieldName` / `fieldNameAt` / `requireField*` in `AvroPrism`'s `widenPath` / `widenPathNamed` / `toFieldsPrism` / `resolveFieldNames` and `AvroTraversal`'s `widenSuffix` / `widenSuffixNamed` / `toFieldsTraversal`. `AvroFocus`, `AvroFocus$Leaf`, `AvroFocus$Fields`, `AvroRecordPrism`, `AvroRecordTraversal` and `AvroBinaryCursor` contain none. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0194EHFR4NamCpTHiqy7B74V --- .../dev/constructive/eo/avro/AvroWalk.scala | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala index b18e54e7..ba9ee609 100644 --- a/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala +++ b/avro/src/main/scala/dev/constructive/eo/avro/AvroWalk.scala @@ -513,19 +513,27 @@ private[avro] object AvroWalk: * An empty `caseNames` (a NamedTuple parent, which has no case fields) abstains too. */ private def totalNominalIndex(record: Schema, caseNames: List[String], declIdx: Int): Int = - if caseNames.isEmpty || declIdx < 0 || declIdx >= caseNames.size then -1 + val arity = caseNames.size + // A total, injective map needs at least as many schema fields as case fields — a free + // precondition that skips the whole scan for the codec that drops a field. + if arity == 0 || declIdx < 0 || declIdx >= arity || arity > record.getFields.size then -1 else - val out = new Array[Int](caseNames.size) - @tailrec def loop(i: Int, rest: List[String], seen: Set[Int]): Boolean = + val out = new Array[Int](arity) + // Injectivity by scanning the filled prefix of `out` rather than a `Set[Int]`: this runs once + // per drilled hop at construction, over a case-field list that is a handful of entries, and a + // Set here costs a boxed Integer and a new Set node per field. + @tailrec def seen(j: Int, idx: Int): Boolean = + j < 0 || (out(j) != idx && seen(j - 1, idx)) + @tailrec def loop(i: Int, rest: List[String]): Boolean = rest match case Nil => true case n :: t => val idx = nominalIndex(record, n) - if idx < 0 || seen.contains(idx) then false + if idx < 0 || !seen(i - 1, idx) then false else out(i) = idx - loop(i + 1, t, seen + idx) - if loop(0, caseNames, Set.empty) then out(declIdx) else -1 + loop(i + 1, t) + if loop(0, caseNames) then out(declIdx) else -1 /** Position of the schema field naming `scalaName` — exactly, else uniquely up to separators and * case. `-1` when no field matches or when more than one does: an ambiguous signal is no signal.