diff --git a/avro/src/main/scala/magnolify/avro/AvroType.scala b/avro/src/main/scala/magnolify/avro/AvroType.scala index e6002e8a6..90a3e77b3 100644 --- a/avro/src/main/scala/magnolify/avro/AvroType.scala +++ b/avro/src/main/scala/magnolify/avro/AvroType.scala @@ -169,11 +169,8 @@ object AvroField { } } - private def getDoc(annotations: Seq[Any], name: String): String = { - val docs = annotations.collect { case d: doc => d.toString } - require(docs.size <= 1, s"More than one @doc annotation: $name") - docs.headOption.orNull - } + private def getDoc(annotations: Seq[Any], name: String): String = + magnolify.shared.doc.extract(annotations, name).orNull @implicitNotFound("Cannot derive AvroField for sealed trait") private sealed trait Dispatchable[T] diff --git a/avro/src/test/scala/magnolify/avro/WrongDocAnnotationSuite.scala b/avro/src/test/scala/magnolify/avro/WrongDocAnnotationSuite.scala new file mode 100644 index 000000000..7d9ac06bb --- /dev/null +++ b/avro/src/test/scala/magnolify/avro/WrongDocAnnotationSuite.scala @@ -0,0 +1,88 @@ +/* + * Copyright 2026 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package magnolify.avro + +import magnolify.test.MagnolifySuite + +/** + * Tests for detecting wrong @doc annotations from unexpected packages. + * + * When users accidentally use @doc from a different package (e.g., com.spotify.scio.avro.types.doc) + * instead of magnolify.shared.doc, the doc strings are silently ignored. This test verifies + * that Magnolify now detects and reports this issue. + */ +class WrongDocAnnotationSuite extends MagnolifySuite { + + test("WrongDocOnlyOnField - should fail when field has wrong @doc without correct @doc") { + val ex = intercept[IllegalArgumentException] { + AvroType[WrongDocOnlyOnField] + } + assert(ex.getMessage.contains("unexpected package")) + assert(ex.getMessage.contains("WrongDocOnlyOnField#userId")) + } + + test("WrongDocOnlyOnRecord - should fail when record has wrong @doc without correct @doc") { + val ex = intercept[IllegalArgumentException] { + AvroType[WrongDocOnlyOnRecord] + } + assert(ex.getMessage.contains("unexpected package")) + assert(ex.getMessage.contains("WrongDocOnlyOnRecord")) + } + + test("BothDocsOnField - should succeed when field has both wrong and correct @doc") { + // Having both annotations means the user knows what they're doing + val at = AvroType[BothDocsOnField] + // The correct @doc should be used + val field = at.schema.getField("userId") + assertEquals(field.doc(), "correct doc from magnolify") + } + + test("BothDocsOnRecord - should succeed when record has both wrong and correct @doc") { + val at = AvroType[BothDocsOnRecord] + assertEquals(at.schema.getDoc, "correct record doc from magnolify") + } + + test("CorrectDocOnly - should succeed with only magnolify @doc") { + val at = AvroType[CorrectDocOnly] + val field = at.schema.getField("userId") + assertEquals(field.doc(), "correct doc") + } +} + +// Test case: field has only the wrong @doc +case class WrongDocOnlyOnField( + @magnolify.avro.wrongpkg.doc("wrong doc") userId: String +) + +// Test case: record has only the wrong @doc +@magnolify.avro.wrongpkg.doc("wrong record doc") +case class WrongDocOnlyOnRecord(userId: String) + +// Test case: field has both wrong and correct @doc - should succeed +case class BothDocsOnField( + @magnolify.avro.wrongpkg.doc("wrong doc") @doc("correct doc from magnolify") userId: String +) + +// Test case: record has both wrong and correct @doc - should succeed +@magnolify.avro.wrongpkg.doc("wrong record doc") +@doc("correct record doc from magnolify") +case class BothDocsOnRecord(userId: String) + +// Test case: only correct @doc - should succeed +case class CorrectDocOnly( + @doc("correct doc") userId: String +) diff --git a/avro/src/test/scala/magnolify/avro/wrongpkg/doc.scala b/avro/src/test/scala/magnolify/avro/wrongpkg/doc.scala new file mode 100644 index 000000000..42dfd73b2 --- /dev/null +++ b/avro/src/test/scala/magnolify/avro/wrongpkg/doc.scala @@ -0,0 +1,26 @@ +/* + * Copyright 2026 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package magnolify.avro.wrongpkg + +import scala.annotation.StaticAnnotation + +/** + * A fake @doc annotation in a different package, simulating what happens + * when users accidentally use @doc from com.spotify.scio.avro.types instead + * of magnolify.shared.doc. + */ +class doc(value: String) extends StaticAnnotation diff --git a/parquet/src/main/scala/magnolify/parquet/ParquetField.scala b/parquet/src/main/scala/magnolify/parquet/ParquetField.scala index 85277f60c..c64b2a5b2 100644 --- a/parquet/src/main/scala/magnolify/parquet/ParquetField.scala +++ b/parquet/src/main/scala/magnolify/parquet/ParquetField.scala @@ -195,11 +195,8 @@ object ParquetField { implicit def apply[T]: ParquetField[T] = macro Magnolia.gen[T] - private def getDoc(annotations: Seq[Any], name: String): Option[String] = { - val docs = annotations.collect { case d: magnolify.shared.doc => d.toString } - require(docs.size <= 1, s"More than one @doc annotation: $name") - docs.headOption - } + private def getDoc(annotations: Seq[Any], name: String): Option[String] = + magnolify.shared.doc.extract(annotations, name) // //////////////////////////////////////////////// diff --git a/shared/src/main/scala/magnolify/shared/doc.scala b/shared/src/main/scala/magnolify/shared/doc.scala index 95e539ce2..e362845d7 100644 --- a/shared/src/main/scala/magnolify/shared/doc.scala +++ b/shared/src/main/scala/magnolify/shared/doc.scala @@ -21,3 +21,44 @@ import scala.annotation.StaticAnnotation class doc(doc: String) extends StaticAnnotation with Serializable { override def toString: String = doc } + +object doc { + + /** + * Extract doc string from annotations, validating that only the correct @doc type is used. + * + * This helper detects when users accidentally use @doc from a different package + * (e.g., com.spotify.scio.avro.types.doc) instead of magnolify.shared.doc. Such annotations are + * silently ignored by Magnolify's type derivation, leading to missing documentation in generated + * schemas. + * + * @param annotations + * The annotations to inspect (from Magnolia's CaseClass or Param) + * @param name + * A descriptive name for the annotated element (for error messages) + * @return + * Some(docString) if present, otherwise None + * @throws IllegalArgumentException + * if a wrong @doc annotation is found without a correct one + */ + def extract(annotations: Seq[Any], name: String): Option[String] = { + val correctDocs = annotations.collect { case d: doc => d.toString } + require(correctDocs.size <= 1, s"More than one @doc annotation: $name") + + // Check for @doc annotations from unexpected packages (e.g., com.spotify.scio.avro.types.doc) + // Only fail if there's no correct @doc - if both are present, assume the user knows what they're doing. + if (correctDocs.isEmpty) { + val wrongDocs = annotations.filter { a => + a.getClass.getSimpleName == classOf[doc].getSimpleName && !a.isInstanceOf[doc] + } + require( + wrongDocs.isEmpty, + s"Found @doc annotation(s) from unexpected package(s) on $name: " + + s"${wrongDocs.map(_.getClass.getName).distinct.mkString(", ")}. " + + s"Use ${classOf[doc].getName} instead." + ) + } + + correctDocs.headOption + } +} diff --git a/tensorflow/src/main/scala/magnolify/tensorflow/ExampleType.scala b/tensorflow/src/main/scala/magnolify/tensorflow/ExampleType.scala index a99133fd0..a53d4ea90 100644 --- a/tensorflow/src/main/scala/magnolify/tensorflow/ExampleType.scala +++ b/tensorflow/src/main/scala/magnolify/tensorflow/ExampleType.scala @@ -160,11 +160,10 @@ object ExampleField { } } - private def getDoc(annotations: Seq[Any], name: String): Option[Annotation] = { - val docs = annotations.collect { case d: doc => d.toString } - require(docs.size <= 1, s"More than one @doc annotation: $name") - docs.headOption.map(doc => Annotation.newBuilder().addTag(doc).build()) - } + private def getDoc(annotations: Seq[Any], name: String): Option[Annotation] = + magnolify.shared.doc + .extract(annotations, name) + .map(d => Annotation.newBuilder().addTag(d).build()) @implicitNotFound("Cannot derive ExampleField for sealed trait") private sealed trait Dispatchable[T]