diff --git a/CHANGELOG.md b/CHANGELOG.md index a61143a1..f0e2ace3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,61 @@ 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. +- **`.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)`. +- **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 + +- **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 d66ad3d1..8a3c6550 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): * @@ -75,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. @@ -241,16 +266,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", + ) ) ) @@ -261,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]]. */ @@ -289,9 +323,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 +336,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 +344,7 @@ final class AvroPrism[A] private[avro] ( path, scalaNames(i), declIdxs(i), + caseNames, "AvroPrism.fields", ) ) @@ -339,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/AvroPrismMacro.scala b/avro/src/main/scala/dev/constructive/eo/avro/AvroPrismMacro.scala index 1113f12a..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,13 +25,18 @@ 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](${ 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 +50,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, @@ -199,12 +210,19 @@ 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](${ 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 +253,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 +273,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 +293,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 +356,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..cc30ae56 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", + ) ) ) @@ -171,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 @@ -219,10 +227,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, @@ -242,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 5d501781..ba9ee609 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, @@ -379,10 +380,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 +395,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 +407,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 +454,125 @@ 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 + /** 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. + * + * 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 = + 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](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(i - 1, idx) then false + else + out(i) = idx + 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. + */ + 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/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/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/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/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..d3aabfbd --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/AvroNominalResolutionSpec.scala @@ -0,0 +1,194 @@ +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") + } + + // ---- 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/DivergentCodecs.scala b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/DivergentCodecs.scala new file mode 100644 index 00000000..91e0a7a6 --- /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..7e031444 --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionFalsePositiveSpec.scala @@ -0,0 +1,365 @@ +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 + } + + "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 === + 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 new file mode 100644 index 00000000..220f890e --- /dev/null +++ b/avro/src/test/scala/dev/constructive/eo/avro/vulcan/ResolutionResidualSpec.scala @@ -0,0 +1,195 @@ +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 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 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