From 8de44d60d7d6ef5da988fc7c6a80babac1804503 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Thu, 17 Aug 2023 15:44:35 -0600 Subject: [PATCH 1/8] add basic mixin handling --- modules/openapi/src/internals/Hint.scala | 2 + .../src/internals/IModelToSmithy.scala | 5 + .../postprocess/AllOfTransformer.scala | 116 ++++++++++++++---- modules/openapi/tests/src/AllOfSpec.scala | 4 +- modules/openapi/tests/src/TestUtils.scala | 6 + 5 files changed, 105 insertions(+), 28 deletions(-) diff --git a/modules/openapi/src/internals/Hint.scala b/modules/openapi/src/internals/Hint.scala index dfa276d..5ce1346 100644 --- a/modules/openapi/src/internals/Hint.scala +++ b/modules/openapi/src/internals/Hint.scala @@ -47,5 +47,7 @@ object Hint { case class JsonName(name: String) extends Hint case class Examples(value: List[Node]) extends Hint case class ExternalDocs(description: Option[String], url: String) extends Hint + case object IsMixin extends Hint + case class HasMixin(id: DefId) extends Hint } // format: on diff --git a/modules/openapi/src/internals/IModelToSmithy.scala b/modules/openapi/src/internals/IModelToSmithy.scala index da60d15..64f7b78 100644 --- a/modules/openapi/src/internals/IModelToSmithy.scala +++ b/modules/openapi/src/internals/IModelToSmithy.scala @@ -67,9 +67,13 @@ final class IModelToSmithy(useEnumTraitSyntax: Boolean) .addHints(hints ++ jsonNameHint) .build() } + val mixinIds = structHints.collect { case Hint.HasMixin(defId) => + StructureShape.builder.id(defId.toSmithy).build() + }.asJavaCollection val builder = StructureShape .builder() .id(id.toSmithy) + .mixins(mixinIds) .addHints(structHints) members.foreach(builder.addMember(_)) builder.build() @@ -306,6 +310,7 @@ final class IModelToSmithy(useEnumTraitSyntax: Boolean) case Hint.UniqueItems => List(new UniqueItemsTrait()) case Hint.Nullable => List(new NullableTrait()) case Hint.JsonName(value) => List(new JsonNameTrait(value)) + case Hint.IsMixin => List(MixinTrait.builder.build) case Hint.ExternalDocs(desc, url) => List( ExternalDocumentationTrait diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index e55f60f..76fd600 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -18,6 +18,7 @@ package postprocess import cats.syntax.all._ import scala.annotation.tailrec +import cats.kernel.Eq object AllOfTransformer extends IModelPostProcessor { @@ -25,22 +26,74 @@ object AllOfTransformer extends IModelPostProcessor { fieldsToAdd: Vector[Field], defsToRemove: Set[DefId], isDocument: Boolean, - hints: List[Hint] + hints: List[Hint], + newOrUpdateDefs: Set[Definition] ) { def addFields(f: Vector[Field]) = copy(fieldsToAdd = fieldsToAdd ++ f) def addRemoveDef(d: DefId) = copy(defsToRemove = defsToRemove + d) def setIsDocument = copy(isDocument = true) def withHints(hints: List[Hint]) = copy(hints = this.hints ++ hints) + def addOrUpdateDef(d: Definition) = + copy(newOrUpdateDefs = newOrUpdateDefs + d) } private object State { def empty: State = - State(Vector.empty, Set.empty, isDocument = false, List.empty) + State(Vector.empty, Set.empty, isDocument = false, List.empty, Set.empty) } private val DocumentPrimitive = DefId(Namespace(List("smithy", "api")), Name.stdLib("Document")) + private implicit val defIdEq: Eq[DefId] = Eq.fromUniversalEquals + + private def isReferencedAsTarget( + shape: Definition, + allShapes: Vector[Definition] + ): Boolean = { + @tailrec + def loop( + remainingShapes: List[Definition], + isReferenced: Boolean = false + ): Boolean = + remainingShapes match { + case _ if isReferenced => isReferenced + case Nil => isReferenced + case (s: Shape) :: tail => + val hasRef = s match { + case s: Structure => + s.localFields.exists(_.tpe === shape.id) + case s: SetDef => + s.member === shape.id + case l: ListDef => + l.member === shape.id + case m: MapDef => + m.key === shape.id || m.value === shape.id + case u: Union => + u.alts.exists(_.tpe === shape.id) + case n: Newtype => + n.target === shape.id + case _: Enumeration => false + case o: OperationDef => + val allRefs = o.input.toVector ++ o.output.toVector ++ o.errors + allRefs.contains_(shape.id) + case _: ServiceDef => false + } + loop(tail, hasRef) + case _ :: tail => loop(tail, isReferenced) + } + + loop(allShapes.toList) + } + + // FOR EACH structure parent, detect if they are used in any contexts outside of this structure + // IF NOT: + // - Make structure parent a mixin + // - Apply as mixin to this structure instead of moving all fields in + // IF YES: + // - Make a new structure, identical to the parent, postfixed with `Mixin` that has all fields + // - Make existing parent structure have 0 fields and instead mixin the structure made in previous step + // - Update so this structure mixes in new structure instead of bringing in all fields @tailrec private def getAllParentFields( newId: DefId, @@ -49,7 +102,12 @@ object AllOfTransformer extends IModelPostProcessor { state: State = State.empty ): State = parents.toList match { case parent :: tail => + val parentHasOtherReferences = isReferencedAsTarget(parent, allShapes) parent match { + case s: Structure if !parentHasOtherReferences => + state + .addOrUpdateDef(s.mapHints(_ :+ Hint.IsMixin)) + .withHints(List(Hint.HasMixin(s.id))) case Structure(id, local, newParents, hints) => val updatedLocal = local.map(f => f.copy(id = f.id.copy(modelId = newId))) @@ -73,31 +131,37 @@ object AllOfTransformer extends IModelPostProcessor { private def transform(in: IModel): Vector[Definition] = { val allShapes = in.definitions - val (remove, result) = allShapes - .map { - case s @ Structure(id, localFields, parents, _) if parents.nonEmpty => - val parentDefs = - allShapes.filter(definition => parents.contains(definition.id)) - val State(fieldsToAdd, defsToRemove, isDocument, hints) = - getAllParentFields(id, allShapes, parentDefs) - val newStruct = s.copy( - localFields = localFields ++ fieldsToAdd, - parents = Vector.empty, - hints = s.hints ++ hints - ) - val finalShape = - if (isDocument) Newtype(id, DocumentPrimitive, hints) - else newStruct - ( - defsToRemove, - finalShape - ) - case other => (Set.empty, other) - } - .unzip - .leftMap(_.flatten) + val (remove, newOrUpdate, result) = allShapes.map { + case s @ Structure(id, localFields, parents, _) if parents.nonEmpty => + val parentDefs = + allShapes.filter(definition => parents.contains(definition.id)) + val State( + fieldsToAdd, + defsToRemove, + isDocument, + hints, + newOrUpdateDefs + ) = + getAllParentFields(id, allShapes, parentDefs) + val newStruct = s.copy( + localFields = localFields ++ fieldsToAdd, + parents = Vector.empty, + hints = s.hints ++ hints + ) + val finalShape = + if (isDocument) Newtype(id, DocumentPrimitive, hints) + else newStruct + ( + defsToRemove, + newOrUpdateDefs, + finalShape + ) + case other => (Set.empty, Set.empty, other) + }.unzip3 - result.filterNot(d => remove.contains(d.id)) + val updateOrAdd = newOrUpdate.flatten + val toRemove = remove.flatten ++ updateOrAdd.map(_.id) + result.filterNot(d => toRemove.contains(d.id)) ++ updateOrAdd.toVector } def apply(in: IModel): IModel = { diff --git a/modules/openapi/tests/src/AllOfSpec.scala b/modules/openapi/tests/src/AllOfSpec.scala index 33fa944..b8d1177 100644 --- a/modules/openapi/tests/src/AllOfSpec.scala +++ b/modules/openapi/tests/src/AllOfSpec.scala @@ -41,11 +41,11 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure Object { - | l: Integer, + |structure Object with [Other] { | s: String |} | + |@mixin |structure Other { | l: Integer, |} diff --git a/modules/openapi/tests/src/TestUtils.scala b/modules/openapi/tests/src/TestUtils.scala index 2fc8afe..f949787 100644 --- a/modules/openapi/tests/src/TestUtils.scala +++ b/modules/openapi/tests/src/TestUtils.scala @@ -93,6 +93,7 @@ object TestUtils { remaining: ConversionTestInput* ): ConversionResult = { val inputs = input0 +: remaining + val result = OpenApiCompiler.parseAndCompile( OpenApiCompiler.Options( @@ -104,10 +105,14 @@ object TestUtils { ), inputs.map(i => i.filePath -> i.openapiSpec): _* ) + + println("HJE " + result) + val resultW = result.map(ModelWrapper(_)) val assembler = Model .assembler() .discoverModels() + inputs.foreach { i => i.smithySpec match { case StringOutput(str) => @@ -141,6 +146,7 @@ object TestUtils { input0, remaining: _* ) + res match { case OpenApiCompiler.Failure(err, errors) => errors.foreach(println) From 8607554bab716ecad7651130afaa09e3b7725484 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Thu, 17 Aug 2023 16:46:16 -0600 Subject: [PATCH 2/8] more allof test cases working --- .../src/internals/IModelToSmithy.scala | 8 +- .../postprocess/AllOfTransformer.scala | 49 +++++++- .../postprocess/SimplifyNameTransformer.scala | 6 +- modules/openapi/tests/src/AllOfSpec.scala | 113 ++++++++++++++++-- modules/openapi/tests/src/TestUtils.scala | 2 - 5 files changed, 160 insertions(+), 18 deletions(-) diff --git a/modules/openapi/src/internals/IModelToSmithy.scala b/modules/openapi/src/internals/IModelToSmithy.scala index 64f7b78..cdb9ff4 100644 --- a/modules/openapi/src/internals/IModelToSmithy.scala +++ b/modules/openapi/src/internals/IModelToSmithy.scala @@ -67,15 +67,17 @@ final class IModelToSmithy(useEnumTraitSyntax: Boolean) .addHints(hints ++ jsonNameHint) .build() } - val mixinIds = structHints.collect { case Hint.HasMixin(defId) => + val mixins = structHints.collect { case Hint.HasMixin(defId) => StructureShape.builder.id(defId.toSmithy).build() - }.asJavaCollection + }.asJava val builder = StructureShape .builder() .id(id.toSmithy) - .mixins(mixinIds) .addHints(structHints) members.foreach(builder.addMember(_)) + mixins.forEach { m => + val _ = builder.addMixin(m) + } builder.build() case MapDef(id, key, value, hints) => MapShape diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index 76fd600..57a83fe 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -19,6 +19,7 @@ package postprocess import cats.syntax.all._ import scala.annotation.tailrec import cats.kernel.Eq +import org.typelevel.ci._ object AllOfTransformer extends IModelPostProcessor { @@ -103,11 +104,50 @@ object AllOfTransformer extends IModelPostProcessor { ): State = parents.toList match { case parent :: tail => val parentHasOtherReferences = isReferencedAsTarget(parent, allShapes) + val parentIsTopLevel = parent.hints.contains(Hint.TopLevel) parent match { - case s: Structure if !parentHasOtherReferences => - state + case s @ Structure(_, _, newParents, _) + if !parentHasOtherReferences && parentIsTopLevel => + val finalState = state .addOrUpdateDef(s.mapHints(_ :+ Hint.IsMixin)) .withHints(List(Hint.HasMixin(s.id))) + getAllParentFields( + newId, + allShapes, + tail.toVector ++ allShapes.filter(s => newParents.contains(s.id)), + finalState + ) + case Structure(id, localFields, newParents, hints) + if parentHasOtherReferences && parentIsTopLevel => + // TODO: Check for collisions with Mixin suffix + + val newMixinParentId = + id.copy(name = id.name :+ Segment.Arbitrary(ci"Mixin")) + val newFields = localFields.map(f => + f.copy(id = f.id.copy(modelId = newMixinParentId)) + ) + val newMixinParent = Structure( + newMixinParentId, + newFields, + newParents, + hints :+ Hint.IsMixin + ) + val currentParent = Structure( + id, + Vector.empty, + Vector.empty, + List(Hint.HasMixin(newMixinParent.id)) + ) + val finalState = state + .addOrUpdateDef(newMixinParent) + .addOrUpdateDef(currentParent) + .withHints(List(Hint.HasMixin(newMixinParent.id))) + getAllParentFields( + newId, + allShapes, + tail.toVector ++ allShapes.filter(s => newParents.contains(s.id)), + finalState + ) case Structure(id, local, newParents, hints) => val updatedLocal = local.map(f => f.copy(id = f.id.copy(modelId = newId))) @@ -126,7 +166,10 @@ object AllOfTransformer extends IModelPostProcessor { getAllParentFields(newId, allShapes, tail.toVector, newState) case _ => getAllParentFields(newId, allShapes, tail.toVector, state) } - case Nil => state + case Nil => + if (state.isDocument) + state.copy(newOrUpdateDefs = Set.empty) + else state } private def transform(in: IModel): Vector[Definition] = { diff --git a/modules/openapi/src/internals/postprocess/SimplifyNameTransformer.scala b/modules/openapi/src/internals/postprocess/SimplifyNameTransformer.scala index 28e27de..9afc19c 100644 --- a/modules/openapi/src/internals/postprocess/SimplifyNameTransformer.scala +++ b/modules/openapi/src/internals/postprocess/SimplifyNameTransformer.scala @@ -78,7 +78,11 @@ object SimplifyNameTransformer extends IModelPostProcessor { tpe = f.tpe.simplify ) } - s.copy(id = s.id.simplify, localFields = newFs) + val newHints = s.hints.map { + case h @ Hint.HasMixin(id) => h.copy(id = id.simplify) + case h => h + } + s.copy(id = s.id.simplify, localFields = newFs, hints = newHints) case s: SetDef => s.copy( id = s.id.simplify, diff --git a/modules/openapi/tests/src/AllOfSpec.scala b/modules/openapi/tests/src/AllOfSpec.scala index b8d1177..b9ed399 100644 --- a/modules/openapi/tests/src/AllOfSpec.scala +++ b/modules/openapi/tests/src/AllOfSpec.scala @@ -47,7 +47,7 @@ final class AllOfSpec extends munit.FunSuite { | |@mixin |structure Other { - | l: Integer, + | l: Integer |} |""".stripMargin @@ -80,15 +80,14 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure Object { - | o: Integer, - | t: Integer - |} + |structure Object with [One, Two] {} | + |@mixin |structure One { | o: Integer, |} | + |@mixin |structure Two { | t: Integer, |} @@ -125,15 +124,14 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | |@documentation("Test") - |structure Object { - | o: Integer, - | t: Integer - |} + |structure Object with [One, Two] {} | + |@mixin |structure One { | o: Integer, |} | + |@mixin |structure Two { | t: Integer, |} @@ -177,4 +175,101 @@ final class AllOfSpec extends munit.FunSuite { TestUtils.runConversionTest(openapiString, expectedString) } + + test("allOf - one ref one embedded with another reference") { + val openapiString = """|openapi: '3.0.' + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | Also: + | type: object + | properties: + | other: + | $ref: "#/components/schemas/Other" + | Other: + | type: object + | properties: + | l: + | type: integer + | Object: + | allOf: + | - $ref: "#/components/schemas/Other" + | - type: object + | properties: + | s: + | type: string + |""".stripMargin + + val expectedString = """|namespace foo + | + |structure Object with [OtherMixin] { + | s: String + |} + | + |@mixin + |structure OtherMixin { + | l: Integer + |} + | + |structure Other with [OtherMixin] {} + | + |structure Also { + | other: Other + |} + |""".stripMargin + + TestUtils.runConversionTest(openapiString, expectedString) + } + + test("allOf - multiple layers".only) { + val openapiString = """|openapi: '3.0.' + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | Three: + | type: object + | properties: + | three: + | type: string + | Two: + | allOf: + | - $ref: "#/components/schemas/Three" + | - type: object + | properties: + | two: + | type: string + | One: + | allOf: + | - $ref: "#/components/schemas/Two" + | - type: object + | properties: + | one: + | type: string + |""".stripMargin + + val expectedString = """|namespace foo + | + |structure One with [Two, Three] { + | one: String + |} + | + |@mixin + |structure Three { + | three: String + |} + | + |@mixin + |structure Two { + | two: String + |} + |""".stripMargin + + TestUtils.runConversionTest(openapiString, expectedString) + } } diff --git a/modules/openapi/tests/src/TestUtils.scala b/modules/openapi/tests/src/TestUtils.scala index f949787..cafb9e1 100644 --- a/modules/openapi/tests/src/TestUtils.scala +++ b/modules/openapi/tests/src/TestUtils.scala @@ -106,8 +106,6 @@ object TestUtils { inputs.map(i => i.filePath -> i.openapiSpec): _* ) - println("HJE " + result) - val resultW = result.map(ModelWrapper(_)) val assembler = Model .assembler() From 4e4c41c6f5857bf9377b4ca46a866011a0ca8f37 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Fri, 18 Aug 2023 13:58:16 -0600 Subject: [PATCH 3/8] all of tests passing --- .../postprocess/AllOfTransformer.scala | 277 ++++++++-------- modules/openapi/tests/src/AllOfSpec.scala | 299 ++++++++++-------- 2 files changed, 312 insertions(+), 264 deletions(-) diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index 57a83fe..41f934e 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -20,34 +20,16 @@ import cats.syntax.all._ import scala.annotation.tailrec import cats.kernel.Eq import org.typelevel.ci._ +import scala.collection.mutable.ListBuffer object AllOfTransformer extends IModelPostProcessor { - private case class State( - fieldsToAdd: Vector[Field], - defsToRemove: Set[DefId], - isDocument: Boolean, - hints: List[Hint], - newOrUpdateDefs: Set[Definition] - ) { - def addFields(f: Vector[Field]) = copy(fieldsToAdd = fieldsToAdd ++ f) - def addRemoveDef(d: DefId) = copy(defsToRemove = defsToRemove + d) - def setIsDocument = copy(isDocument = true) - def withHints(hints: List[Hint]) = copy(hints = this.hints ++ hints) - def addOrUpdateDef(d: Definition) = - copy(newOrUpdateDefs = newOrUpdateDefs + d) - } - - private object State { - def empty: State = - State(Vector.empty, Set.empty, isDocument = false, List.empty, Set.empty) - } - private val DocumentPrimitive = DefId(Namespace(List("smithy", "api")), Name.stdLib("Document")) private implicit val defIdEq: Eq[DefId] = Eq.fromUniversalEquals + // Checks if a given shape is ever referenced outside of being used as a mixin private def isReferencedAsTarget( shape: Definition, allShapes: Vector[Definition] @@ -87,124 +69,155 @@ object AllOfTransformer extends IModelPostProcessor { loop(allShapes.toList) } - // FOR EACH structure parent, detect if they are used in any contexts outside of this structure - // IF NOT: - // - Make structure parent a mixin - // - Apply as mixin to this structure instead of moving all fields in - // IF YES: - // - Make a new structure, identical to the parent, postfixed with `Mixin` that has all fields - // - Make existing parent structure have 0 fields and instead mixin the structure made in previous step - // - Update so this structure mixes in new structure instead of bringing in all fields - @tailrec - private def getAllParentFields( - newId: DefId, - allShapes: Vector[Definition], - parents: Vector[Definition], - state: State = State.empty - ): State = parents.toList match { - case parent :: tail => - val parentHasOtherReferences = isReferencedAsTarget(parent, allShapes) - val parentIsTopLevel = parent.hints.contains(Hint.TopLevel) - parent match { - case s @ Structure(_, _, newParents, _) - if !parentHasOtherReferences && parentIsTopLevel => - val finalState = state - .addOrUpdateDef(s.mapHints(_ :+ Hint.IsMixin)) - .withHints(List(Hint.HasMixin(s.id))) - getAllParentFields( - newId, - allShapes, - tail.toVector ++ allShapes.filter(s => newParents.contains(s.id)), - finalState - ) - case Structure(id, localFields, newParents, hints) - if parentHasOtherReferences && parentIsTopLevel => - // TODO: Check for collisions with Mixin suffix - - val newMixinParentId = - id.copy(name = id.name :+ Segment.Arbitrary(ci"Mixin")) - val newFields = localFields.map(f => - f.copy(id = f.id.copy(modelId = newMixinParentId)) - ) - val newMixinParent = Structure( - newMixinParentId, - newFields, - newParents, - hints :+ Hint.IsMixin - ) - val currentParent = Structure( - id, - Vector.empty, - Vector.empty, - List(Hint.HasMixin(newMixinParent.id)) - ) - val finalState = state - .addOrUpdateDef(newMixinParent) - .addOrUpdateDef(currentParent) - .withHints(List(Hint.HasMixin(newMixinParent.id))) - getAllParentFields( - newId, - allShapes, - tail.toVector ++ allShapes.filter(s => newParents.contains(s.id)), - finalState - ) - case Structure(id, local, newParents, hints) => - val updatedLocal = - local.map(f => f.copy(id = f.id.copy(modelId = newId))) - val newState = state.addFields(updatedLocal).withHints(hints) - val finalState = - if (!hints.contains(Hint.TopLevel)) newState.addRemoveDef(id) - else newState - getAllParentFields( - newId, - allShapes, - tail.toVector ++ allShapes.filter(s => newParents.contains(s.id)), - finalState + private case class ParentHasOtherRefsResult( + newDef: Structure, + newMixin: Structure, + newParent: Structure + ) + + private def parentHasOtherReferencesCase( + parent: Structure, + newDef: Structure + ): ParentHasOtherRefsResult = { + val newMixinParentId = + parent.id.copy(name = parent.id.name :+ Segment.Arbitrary(ci"Mixin")) + + val newFields = + parent.localFields + .map(f => f.copy(id = f.id.copy(modelId = newMixinParentId))) + + // 1. Make a new structure, identical to the parent, postfixed with `Mixin` that has all fields + val newMixin = + parent.copy( + id = newMixinParentId, + localFields = newFields, + hints = parent.hints :+ Hint.IsMixin + ) + + // 2. Make existing parent structure have 0 fields and instead mixin the structure made in previous step + val newParent = + parent.copy( + localFields = Vector.empty, + hints = List(Hint.HasMixin(newMixinParentId)) + ) + + // 3. Update so this structure mixes in new structure instead of bringing in all fields + val nd = + newDef.copy(hints = newDef.hints :+ Hint.HasMixin(newMixinParentId)) + ParentHasOtherRefsResult(nd, newMixin, newParent) + } + + private case class TopLevelParentNoRefsResult( + newDef: Structure, + newParent: Structure + ) + private def topLevelParentNoReferences( + parent: Structure, + newDef: Structure + ): TopLevelParentNoRefsResult = { + // 1. Make structure parent a mixin + val newParent = parent.copy(hints = parent.hints :+ Hint.IsMixin) + // 2. Apply as mixin to this structure instead of moving all fields in + val nd = newDef.copy(hints = newDef.hints :+ Hint.HasMixin(parent.id)) + TopLevelParentNoRefsResult(nd, newParent) + } + + private case class NonTopLevelParentNoRefsResult( + newDef: Structure, + removeParent: Structure + ) + private def nonTopLevelParentNoReferences( + parent: Structure, + newDef: Structure + ): NonTopLevelParentNoRefsResult = { + // Move fields down from parent and remove parent if parent is not a top level + // shape (meaning it is a fabricated AllOf shape created by the OpenApi => IModel transform) + val newFields = + parent.localFields.map(f => f.copy(id = f.id.copy(modelId = newDef.id))) + val remove = parent + val nd = newDef.copy(localFields = newFields) + NonTopLevelParentNoRefsResult(nd, remove) + } + + private def moveParentFieldsAndCreateMixins( + all: Vector[Definition] + ): Vector[Definition] = { + val allShapes: ListBuffer[Definition] = ListBuffer.from(all) + def replace(d: Definition) = { + remove(d) + add(d) + } + def remove(d: Definition) = + allShapes.remove(allShapes.indexWhere(_.id === d.id)) + def add(d: Definition) = + allShapes += d + + def revertMixinHints(newDef: Structure) = { + allShapes.filter(shp => newDef.parents.contains(shp.id)).foreach { + case par: Structure => + replace( + par.copy(hints = par.hints.filterNot(_ == Hint.IsMixin)) ) - case Newtype(_, DocumentPrimitive, hints) => - val newState = state.setIsDocument.withHints(hints) - getAllParentFields(newId, allShapes, tail.toVector, newState) - case _ => getAllParentFields(newId, allShapes, tail.toVector, state) + case _ => () } - case Nil => - if (state.isDocument) - state.copy(newOrUpdateDefs = Set.empty) - else state + } + + all.foreach { + case struct: Structure => + val parents = struct.parents.flatMap(p => allShapes.find(_.id === p)) + var isDocument = false + var newDef: Structure = struct + parents.foreach { + case Newtype(_, DocumentPrimitive, _) => + isDocument = true + case parent: Structure => + // FOR EACH structure parent, detect if they are used in any contexts outside of this structure + val parentHasOtherReferences = + isReferencedAsTarget(parent, all) + val parentIsTopLevel = parent.hints.contains(Hint.TopLevel) + + if (parentHasOtherReferences) { + val result = parentHasOtherReferencesCase(parent, newDef) + newDef = result.newDef + add(result.newMixin) + replace(result.newParent) + } else { + if (parentIsTopLevel) { + val result = topLevelParentNoReferences(parent, newDef) + newDef = result.newDef + replace(result.newParent) + } else { + val result = nonTopLevelParentNoReferences(parent, newDef) + newDef = result.newDef + remove(result.removeParent) + } + } + case other => other + } + if (isDocument) { + // if a document then no need to consider any of the parents a mixin (the mixin would not ever be used) + // so we retroactively remove any `IsMixin` hints we added above + revertMixinHints(newDef) + } + val n: Definition = + if (isDocument) + Newtype( + newDef.id, + DocumentPrimitive, + newDef.hints + ) + else newDef + replace( + n + ) + case other => Vector(other) + } + allShapes.toVector } private def transform(in: IModel): Vector[Definition] = { val allShapes = in.definitions - val (remove, newOrUpdate, result) = allShapes.map { - case s @ Structure(id, localFields, parents, _) if parents.nonEmpty => - val parentDefs = - allShapes.filter(definition => parents.contains(definition.id)) - val State( - fieldsToAdd, - defsToRemove, - isDocument, - hints, - newOrUpdateDefs - ) = - getAllParentFields(id, allShapes, parentDefs) - val newStruct = s.copy( - localFields = localFields ++ fieldsToAdd, - parents = Vector.empty, - hints = s.hints ++ hints - ) - val finalShape = - if (isDocument) Newtype(id, DocumentPrimitive, hints) - else newStruct - ( - defsToRemove, - newOrUpdateDefs, - finalShape - ) - case other => (Set.empty, Set.empty, other) - }.unzip3 - - val updateOrAdd = newOrUpdate.flatten - val toRemove = remove.flatten ++ updateOrAdd.map(_.id) - result.filterNot(d => toRemove.contains(d.id)) ++ updateOrAdd.toVector + moveParentFieldsAndCreateMixins(allShapes) } def apply(in: IModel): IModel = { diff --git a/modules/openapi/tests/src/AllOfSpec.scala b/modules/openapi/tests/src/AllOfSpec.scala index b9ed399..078e378 100644 --- a/modules/openapi/tests/src/AllOfSpec.scala +++ b/modules/openapi/tests/src/AllOfSpec.scala @@ -19,159 +19,180 @@ final class AllOfSpec extends munit.FunSuite { test("allOf - one ref one embedded") { val openapiString = """|openapi: '3.0.' - |info: - | title: test - | version: '1.0' - |paths: {} - |components: - | schemas: - | Other: - | type: object - | properties: - | l: - | type: integer - | Object: - | allOf: - | - $ref: "#/components/schemas/Other" - | - type: object - | properties: - | s: - | type: string - |""".stripMargin + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | Other: + | description: other + | type: object + | properties: + | l: + | type: integer + | Object: + | description: object + | allOf: + | - $ref: "#/components/schemas/Other" + | - type: object + | properties: + | s: + | type: string + |""".stripMargin val expectedString = """|namespace foo - | - |structure Object with [Other] { - | s: String - |} - | - |@mixin - |structure Other { - | l: Integer - |} - |""".stripMargin + | + |/// object + |structure Object with [Other] { + | s: String + |} + | + |/// other + |@mixin + |structure Other { + | l: Integer + |} + |""".stripMargin TestUtils.runConversionTest(openapiString, expectedString) } test("allOf - two refs") { val openapiString = """|openapi: '3.0.' - |info: - | title: test - | version: '1.0' - |paths: {} - |components: - | schemas: - | One: - | type: object - | properties: - | o: - | type: integer - | Two: - | type: object - | properties: - | t: - | type: integer - | Object: - | allOf: - | - $ref: "#/components/schemas/One" - | - $ref: "#/components/schemas/Two" - |""".stripMargin + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | One: + | description: one + | type: object + | properties: + | o: + | type: integer + | Two: + | description: two + | type: object + | properties: + | t: + | type: integer + | Object: + | description: object + | allOf: + | - $ref: "#/components/schemas/One" + | - $ref: "#/components/schemas/Two" + |""".stripMargin val expectedString = """|namespace foo - | - |structure Object with [One, Two] {} - | - |@mixin - |structure One { - | o: Integer, - |} - | - |@mixin - |structure Two { - | t: Integer, - |} - |""".stripMargin + | + |/// object + |structure Object with [One, Two] {} + | + |/// one + |@mixin + |structure One { + | o: Integer, + |} + | + |/// two + |@mixin + |structure Two { + | t: Integer, + |} + |""".stripMargin TestUtils.runConversionTest(openapiString, expectedString) } - test("allOf - two refs - description") { + test("allOf - document ref") { val openapiString = """|openapi: '3.0.' - |info: - | title: test - | version: '1.0' - |paths: {} - |components: - | schemas: - | One: - | type: object - | properties: - | o: - | type: integer - | Two: - | type: object - | properties: - | t: - | type: integer - | Object: - | description: Test - | allOf: - | - $ref: "#/components/schemas/One" - | - $ref: "#/components/schemas/Two" - |""".stripMargin + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | One: + | description: one + | type: object + | properties: + | o: + | type: integer + | Two: + | description: two + | type: object + | properties: + | Object: + | description: object + | allOf: + | - $ref: "#/components/schemas/One" + | - $ref: "#/components/schemas/Two" + |""".stripMargin val expectedString = """|namespace foo - | - |@documentation("Test") - |structure Object with [One, Two] {} - | - |@mixin - |structure One { - | o: Integer, - |} - | - |@mixin - |structure Two { - | t: Integer, - |} - |""".stripMargin + | + |/// object + |document Object + | + |/// one + |structure One { + | o: Integer, + |} + | + |/// two + |document Two + |""".stripMargin TestUtils.runConversionTest(openapiString, expectedString) } - test("allOf - document ref") { + test("allOf - document ref with two layers") { val openapiString = """|openapi: '3.0.' - |info: - | title: test - | version: '1.0' - |paths: {} - |components: - | schemas: - | One: - | type: object - | properties: - | o: - | type: integer - | Two: - | type: object - | properties: - | Object: - | allOf: - | - $ref: "#/components/schemas/One" - | - $ref: "#/components/schemas/Two" - |""".stripMargin + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | Three: + | description: three + | type: object + | properties: + | o: + | type: integer + | One: + | description: one + | allOf: + | - $ref: "#/components/schemas/Two" + | - $ref: "#/components/schemas/Three" + | Two: + | description: two + | type: object + | properties: + | Object: + | description: object + | allOf: + | - $ref: "#/components/schemas/One" + |""".stripMargin val expectedString = """|namespace foo - | - |document Object - | - |structure One { - | o: Integer, - |} - | - |document Two - |""".stripMargin + | + |/// object + |document Object + | + |/// three + |structure Three { + | o: Integer + |} + | + |/// one + |document One + | + |/// two + |document Two + |""".stripMargin TestUtils.runConversionTest(openapiString, expectedString) } @@ -185,16 +206,19 @@ final class AllOfSpec extends munit.FunSuite { |components: | schemas: | Also: + | description: also | type: object | properties: | other: | $ref: "#/components/schemas/Other" | Other: + | description: other | type: object | properties: | l: | type: integer | Object: + | description: object | allOf: | - $ref: "#/components/schemas/Other" | - type: object @@ -205,17 +229,21 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | + |/// object |structure Object with [OtherMixin] { | s: String |} | + |/// other |@mixin |structure OtherMixin { | l: Integer |} | + |/// other |structure Other with [OtherMixin] {} | + |/// also |structure Also { | other: Other |} @@ -224,7 +252,7 @@ final class AllOfSpec extends munit.FunSuite { TestUtils.runConversionTest(openapiString, expectedString) } - test("allOf - multiple layers".only) { + test("allOf - multiple layers") { val openapiString = """|openapi: '3.0.' |info: | title: test @@ -233,11 +261,13 @@ final class AllOfSpec extends munit.FunSuite { |components: | schemas: | Three: + | description: three | type: object | properties: | three: | type: string | Two: + | description: two | allOf: | - $ref: "#/components/schemas/Three" | - type: object @@ -245,6 +275,7 @@ final class AllOfSpec extends munit.FunSuite { | two: | type: string | One: + | description: one | allOf: | - $ref: "#/components/schemas/Two" | - type: object @@ -255,21 +286,25 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure One with [Two, Three] { + |/// one + |structure One with [Two] { | one: String |} | + |/// three |@mixin |structure Three { | three: String |} | + |/// two |@mixin - |structure Two { + |structure Two with [Three] { | two: String |} |""".stripMargin TestUtils.runConversionTest(openapiString, expectedString) } + } From 7aa3eb86ccceeea6e7e455d8e123497b83a73549 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Fri, 18 Aug 2023 16:27:43 -0600 Subject: [PATCH 4/8] json schema anyOf passing --- .../src/internals/JsonSchemaToIModel.scala | 7 +- modules/json-schema/tests/src/AllOfSpec.scala | 12 +- .../postprocess/AllOfTransformer.scala | 105 ++++++++++-------- .../postprocess/TaggedUnionTransformer.scala | 42 ++++--- modules/openapi/tests/src/AllOfSpec.scala | 40 +++++++ modules/openapi/tests/src/UnionSpec.scala | 21 ++-- 6 files changed, 148 insertions(+), 79 deletions(-) diff --git a/modules/json-schema/src/internals/JsonSchemaToIModel.scala b/modules/json-schema/src/internals/JsonSchemaToIModel.scala index bc44ee3..5cb0875 100644 --- a/modules/json-schema/src/internals/JsonSchemaToIModel.scala +++ b/modules/json-schema/src/internals/JsonSchemaToIModel.scala @@ -222,7 +222,12 @@ private class JsonSchemaToIModel[F[_]: Parallel: TellShape: TellError]( schema ) } - F.pure(OpenApiObject(local.context.addHints(hints), fields)) + F.pure( + OpenApiObject( + local.context.addHints(hints, retainTopLevel = true), + fields + ) + ) case Extractors.CaseArray(hints, itemSchema) => F.pure( diff --git a/modules/json-schema/tests/src/AllOfSpec.scala b/modules/json-schema/tests/src/AllOfSpec.scala index 3ae5198..38aeb49 100644 --- a/modules/json-schema/tests/src/AllOfSpec.scala +++ b/modules/json-schema/tests/src/AllOfSpec.scala @@ -122,12 +122,16 @@ final class AllOfSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure Example { + |structure Example with [Two] { | firstName: String, | lastName: String, - | sport: String, - | vehicle: String, - | price: Integer + | sport: String + |} + | + |@mixin + |structure Two { + | vehicle: String, + | price: Integer |} | |structure Test { diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index 41f934e..9928540 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -135,7 +135,7 @@ object AllOfTransformer extends IModelPostProcessor { val newFields = parent.localFields.map(f => f.copy(id = f.id.copy(modelId = newDef.id))) val remove = parent - val nd = newDef.copy(localFields = newFields) + val nd = newDef.copy(localFields = newDef.localFields ++ newFields) NonTopLevelParentNoRefsResult(nd, remove) } @@ -143,15 +143,20 @@ object AllOfTransformer extends IModelPostProcessor { all: Vector[Definition] ): Vector[Definition] = { val allShapes: ListBuffer[Definition] = ListBuffer.from(all) + def getLatest(d: Definition): Definition = + allShapes.find(_.id === d.id).getOrElse(d) def replace(d: Definition) = { remove(d) add(d) } - def remove(d: Definition) = - allShapes.remove(allShapes.indexWhere(_.id === d.id)) + def remove(d: Definition) = { + val index = allShapes.indexWhere(_.id === d.id) + if (index >= 0) { allShapes.remove(index) } + } def add(d: Definition) = allShapes += d + // TODO: Likely problematic if parent of document AND non document case def revertMixinHints(newDef: Structure) = { allShapes.filter(shp => newDef.parents.contains(shp.id)).foreach { case par: Structure => @@ -162,55 +167,59 @@ object AllOfTransformer extends IModelPostProcessor { } } - all.foreach { - case struct: Structure => - val parents = struct.parents.flatMap(p => allShapes.find(_.id === p)) - var isDocument = false - var newDef: Structure = struct - parents.foreach { - case Newtype(_, DocumentPrimitive, _) => - isDocument = true - case parent: Structure => - // FOR EACH structure parent, detect if they are used in any contexts outside of this structure - val parentHasOtherReferences = - isReferencedAsTarget(parent, all) - val parentIsTopLevel = parent.hints.contains(Hint.TopLevel) - - if (parentHasOtherReferences) { - val result = parentHasOtherReferencesCase(parent, newDef) - newDef = result.newDef - add(result.newMixin) - replace(result.newParent) - } else { - if (parentIsTopLevel) { - val result = topLevelParentNoReferences(parent, newDef) + all.foreach { d => + // get latest in case modifications have been made to this definition since the + // iterations started + getLatest(d) match { + case struct: Structure => + val parents = struct.parents.flatMap(p => allShapes.find(_.id === p)) + var isDocument = false + var newDef: Structure = struct + parents.foreach { + case Newtype(_, DocumentPrimitive, _) => + isDocument = true + case parent: Structure => + // FOR EACH structure parent, detect if they are used in any contexts outside of this structure + val parentHasOtherReferences = + isReferencedAsTarget(parent, all) + val parentIsTopLevel = parent.hints.contains(Hint.TopLevel) + + if (parentHasOtherReferences) { + val result = parentHasOtherReferencesCase(parent, newDef) newDef = result.newDef + add(result.newMixin) replace(result.newParent) } else { - val result = nonTopLevelParentNoReferences(parent, newDef) - newDef = result.newDef - remove(result.removeParent) + if (parentIsTopLevel) { + val result = topLevelParentNoReferences(parent, newDef) + newDef = result.newDef + replace(result.newParent) + } else { + val result = nonTopLevelParentNoReferences(parent, newDef) + newDef = result.newDef + remove(result.removeParent) + } } - } - case other => other - } - if (isDocument) { - // if a document then no need to consider any of the parents a mixin (the mixin would not ever be used) - // so we retroactively remove any `IsMixin` hints we added above - revertMixinHints(newDef) - } - val n: Definition = - if (isDocument) - Newtype( - newDef.id, - DocumentPrimitive, - newDef.hints - ) - else newDef - replace( - n - ) - case other => Vector(other) + case other => other + } + if (isDocument) { + // if a document then no need to consider any of the parents a mixin (the mixin would not ever be used) + // so we retroactively remove any `IsMixin` hints we added above + revertMixinHints(newDef) + } + val n: Definition = + if (isDocument) + Newtype( + newDef.id, + DocumentPrimitive, + newDef.hints + ) + else newDef + replace( + n + ) + case other => Vector(other) + } } allShapes.toVector } diff --git a/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala b/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala index c84bb80..f204bb6 100644 --- a/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala +++ b/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala @@ -30,21 +30,37 @@ object TaggedUnionTransformer extends IModelPostProcessor { def empty: TaggedUnionInfo = TaggedUnionInfo(Vector.empty, isTagged = true) } + private def getFieldsFromAllParentsTransitively( + target: Definition, + allDefinitions: Vector[Definition] + ): Vector[Field] = { + target match { + case s: Structure => + val allParents = allDefinitions.filter(d => s.parents.contains(d.id)) + allParents.flatMap { parent => + getFieldsFromAllParentsTransitively( + parent, + allDefinitions + ) + } ++ s.localFields + case _ => Vector.empty + } + } + private def getInfoForTargets( - targets: Vector[Definition] + targets: Vector[Definition], + allDefinitions: Vector[Definition] ): TaggedUnionInfo = { targets.foldLeft(TaggedUnionInfo.empty) { (info, t) => - t match { - case Structure(_, localFields, parents, _) => - val singleLocal = localFields.size == 1 - val allLocalRequired = - localFields.forall(_.hints.contains(Hint.Required)) - val noParents = parents.isEmpty - if (singleLocal && allLocalRequired && noParents) - info.addFields(localFields) - else info.setNotTagged - case _ => info.setNotTagged - } + val allFields = + getFieldsFromAllParentsTransitively(t, allDefinitions) + val singleLocal = allFields.size == 1 + val allLocalRequired = + allFields.forall(_.hints.contains(Hint.Required)) + val noParents = true // parents.isEmpty + if (singleLocal && allLocalRequired && noParents) + info.addFields(allFields) + else info.setNotTagged } } @@ -54,7 +70,7 @@ object TaggedUnionTransformer extends IModelPostProcessor { ): TaggedUnionInfo = { val altTargetDefinitions = allShapes.filter(definition => alts.exists(_.tpe == definition.id)) - getInfoForTargets(altTargetDefinitions) + getInfoForTargets(altTargetDefinitions, allShapes) } private def transform(in: IModel): Vector[Definition] = { diff --git a/modules/openapi/tests/src/AllOfSpec.scala b/modules/openapi/tests/src/AllOfSpec.scala index 078e378..99cea11 100644 --- a/modules/openapi/tests/src/AllOfSpec.scala +++ b/modules/openapi/tests/src/AllOfSpec.scala @@ -307,4 +307,44 @@ final class AllOfSpec extends munit.FunSuite { TestUtils.runConversionTest(openapiString, expectedString) } + test("allOf - multiple parents") { + val openapiString = """|openapi: '3.0.' + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | NumberParentOne: + | allOf: + | - $ref: '#/components/schemas/NumberParentOneParent' + | NumberParentOneParent: + | type: object + | properties: + | num: + | type: integer + | required: + | - num + | Number: + | allOf: + | - $ref: '#/components/schemas/NumberParentOne' + |""".stripMargin + + val expectedString = """|namespace foo + | + |structure Number with [NumberParentOne] {} + | + |@mixin + |structure NumberParentOne with [NumberParentOneParent] {} + | + |@mixin + |structure NumberParentOneParent { + | @required + | num: Integer, + |} + |""".stripMargin + + TestUtils.runConversionTest(openapiString, expectedString) + } + } diff --git a/modules/openapi/tests/src/UnionSpec.scala b/modules/openapi/tests/src/UnionSpec.scala index 4795635..d5741d5 100644 --- a/modules/openapi/tests/src/UnionSpec.scala +++ b/modules/openapi/tests/src/UnionSpec.scala @@ -307,14 +307,12 @@ final class UnionSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure Number { - | @required - | num: Integer, - |} + |structure Number with [NumberParentOne] {} | + |@mixin |structure NumberParentOne { | @required - | num: Integer, + | num: Integer |} | |structure Text { @@ -367,21 +365,18 @@ final class UnionSpec extends munit.FunSuite { val expectedString = """|namespace foo | - |structure Number { - | @required - | num: Integer, - |} + | + |structure Number with [NumberParentOne] {} | |structure Text { | @required | txt: String, |} | - |structure NumberParentOne { - | @required - | num: Integer, - |} + |@mixin + |structure NumberParentOne with [NumberParentOneParent] {} | + |@mixin |structure NumberParentOneParent { | @required | num: Integer, From 91c8101448dea1a6412d642cad24f42a8f916a4c Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Mon, 21 Aug 2023 10:14:13 -0600 Subject: [PATCH 5/8] update to use mutable map --- .../postprocess/AllOfTransformer.scala | 42 +++++++------------ 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index 9928540..34305f3 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -20,7 +20,7 @@ import cats.syntax.all._ import scala.annotation.tailrec import cats.kernel.Eq import org.typelevel.ci._ -import scala.collection.mutable.ListBuffer +import scala.collection.mutable object AllOfTransformer extends IModelPostProcessor { @@ -142,27 +142,15 @@ object AllOfTransformer extends IModelPostProcessor { private def moveParentFieldsAndCreateMixins( all: Vector[Definition] ): Vector[Definition] = { - val allShapes: ListBuffer[Definition] = ListBuffer.from(all) - def getLatest(d: Definition): Definition = - allShapes.find(_.id === d.id).getOrElse(d) - def replace(d: Definition) = { - remove(d) - add(d) - } - def remove(d: Definition) = { - val index = allShapes.indexWhere(_.id === d.id) - if (index >= 0) { allShapes.remove(index) } - } - def add(d: Definition) = - allShapes += d + val allShapes: mutable.Map[DefId, Definition] = + mutable.Map.from(all.map(a => a.id -> a)) // TODO: Likely problematic if parent of document AND non document case def revertMixinHints(newDef: Structure) = { - allShapes.filter(shp => newDef.parents.contains(shp.id)).foreach { + allShapes.values.filter(shp => newDef.parents.contains(shp.id)).foreach { case par: Structure => - replace( - par.copy(hints = par.hints.filterNot(_ == Hint.IsMixin)) - ) + allShapes += (par.id -> + par.copy(hints = par.hints.filterNot(_ == Hint.IsMixin))) case _ => () } } @@ -170,9 +158,9 @@ object AllOfTransformer extends IModelPostProcessor { all.foreach { d => // get latest in case modifications have been made to this definition since the // iterations started - getLatest(d) match { + allShapes.getOrElse(d.id, d) match { case struct: Structure => - val parents = struct.parents.flatMap(p => allShapes.find(_.id === p)) + val parents = struct.parents.flatMap(p => allShapes.get(p)) var isDocument = false var newDef: Structure = struct parents.foreach { @@ -187,17 +175,17 @@ object AllOfTransformer extends IModelPostProcessor { if (parentHasOtherReferences) { val result = parentHasOtherReferencesCase(parent, newDef) newDef = result.newDef - add(result.newMixin) - replace(result.newParent) + allShapes += (result.newMixin.id -> result.newMixin) + allShapes += (result.newParent.id -> result.newParent) } else { if (parentIsTopLevel) { val result = topLevelParentNoReferences(parent, newDef) newDef = result.newDef - replace(result.newParent) + allShapes += (result.newParent.id -> result.newParent) } else { val result = nonTopLevelParentNoReferences(parent, newDef) newDef = result.newDef - remove(result.removeParent) + allShapes.remove(result.removeParent.id) } } case other => other @@ -215,13 +203,11 @@ object AllOfTransformer extends IModelPostProcessor { newDef.hints ) else newDef - replace( - n - ) + allShapes += (n.id -> n) case other => Vector(other) } } - allShapes.toVector + allShapes.values.toVector } private def transform(in: IModel): Vector[Definition] = { From 92f2ed34357f98bdca5626e154b16b4ceccc0992 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Mon, 21 Aug 2023 10:39:10 -0600 Subject: [PATCH 6/8] handle unused mixin hints better --- .../postprocess/AllOfTransformer.scala | 42 ++++++++------- modules/openapi/tests/src/AllOfSpec.scala | 51 +++++++++++++++++++ 2 files changed, 74 insertions(+), 19 deletions(-) diff --git a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala index 34305f3..f2dd159 100644 --- a/modules/openapi/src/internals/postprocess/AllOfTransformer.scala +++ b/modules/openapi/src/internals/postprocess/AllOfTransformer.scala @@ -142,18 +142,8 @@ object AllOfTransformer extends IModelPostProcessor { private def moveParentFieldsAndCreateMixins( all: Vector[Definition] ): Vector[Definition] = { - val allShapes: mutable.Map[DefId, Definition] = - mutable.Map.from(all.map(a => a.id -> a)) - - // TODO: Likely problematic if parent of document AND non document case - def revertMixinHints(newDef: Structure) = { - allShapes.values.filter(shp => newDef.parents.contains(shp.id)).foreach { - case par: Structure => - allShapes += (par.id -> - par.copy(hints = par.hints.filterNot(_ == Hint.IsMixin))) - case _ => () - } - } + val allShapes: mutable.LinkedHashMap[DefId, Definition] = + mutable.LinkedHashMap.from(all.map(a => a.id -> a)) all.foreach { d => // get latest in case modifications have been made to this definition since the @@ -190,24 +180,38 @@ object AllOfTransformer extends IModelPostProcessor { } case other => other } - if (isDocument) { - // if a document then no need to consider any of the parents a mixin (the mixin would not ever be used) - // so we retroactively remove any `IsMixin` hints we added above - revertMixinHints(newDef) - } val n: Definition = if (isDocument) Newtype( newDef.id, DocumentPrimitive, - newDef.hints + newDef.hints.filterNot(_.isInstanceOf[Hint.HasMixin]) ) else newDef allShapes += (n.id -> n) case other => Vector(other) } } - allShapes.values.toVector + removeUnusedMixins(allShapes).values.toVector + } + + private def removeUnusedMixins( + allShapes: mutable.Map[DefId, Definition] + ): mutable.Map[DefId, Definition] = { + val usedAsMixins: Set[DefId] = allShapes.values.flatMap { v => + v.hints.collect { case Hint.HasMixin(id) => id } + }.toSet + + val isAMixin: Set[DefId] = allShapes.values.flatMap { v => + v.hints.collect { case Hint.IsMixin => v.id } + }.toSet + + val unused = isAMixin.diff(usedAsMixins) + + allShapes.map { case (id, shp) => + if (unused(id)) id -> shp.mapHints(_.filterNot(_ == Hint.IsMixin)) + else id -> shp + } } private def transform(in: IModel): Vector[Definition] = { diff --git a/modules/openapi/tests/src/AllOfSpec.scala b/modules/openapi/tests/src/AllOfSpec.scala index 99cea11..b9b5c49 100644 --- a/modules/openapi/tests/src/AllOfSpec.scala +++ b/modules/openapi/tests/src/AllOfSpec.scala @@ -347,4 +347,55 @@ final class AllOfSpec extends munit.FunSuite { TestUtils.runConversionTest(openapiString, expectedString) } + test("allOf - document AND normal parent refs") { + val openapiString = """|openapi: '3.0.' + |info: + | title: test + | version: '1.0' + |paths: {} + |components: + | schemas: + | One: + | description: one + | type: object + | properties: + | o: + | type: integer + | Two: + | description: two + | type: object + | properties: + | Three: + | description: three + | type: object + | allOf: + | - $ref: "#/components/schemas/One" + | Object: + | description: object + | allOf: + | - $ref: "#/components/schemas/One" + | - $ref: "#/components/schemas/Two" + |""".stripMargin + + val expectedString = """|namespace foo + | + |/// object + |document Object + | + |/// one + |@mixin + |structure One { + | o: Integer, + |} + | + |/// three + |structure Three with [One] {} + | + |/// two + |document Two + |""".stripMargin + + TestUtils.runConversionTest(openapiString, expectedString) + } + } From 2a721879617d22a8f73204b713292d197f8dd523 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Mon, 21 Aug 2023 11:01:48 -0600 Subject: [PATCH 7/8] add docs --- README.md | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/README.md b/README.md index 195fb3f..bd5ab0a 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ _Note: this library is published to work on Java 8 and above. However, you will - [Primitives](#primitives) - [Aggregate Shapes](#aggregate-shapes) - [Structure](#structure) + - [Structures with Mixins](#structures-with-mixins) - [Untagged Union](#untagged-union) - [Tagged Union](#tagged-union) - [Discriminated Union](#discriminated-union) @@ -235,6 +236,139 @@ structure Testing { } ``` +##### Structures with Mixins + +Smithy Translate will convert allOfs from OpenAPI into structures with mixins in smithy where possible. AllOfs in OpenAPI have references to other types which compose the current type. We refer to these as "parents" or "parent types" below. There are three possibilities when converting allOfs to smithy shapes: + +1. The parent structures are only ever used as mixins + +OpenAPI: +```yaml +openapi: '3.0.' +info: + title: doc + version: 1.0.0 +paths: {} +components: + schemas: + One: + type: object + properties: + one: + type: string + Two: + type: object + properties: + two: + type: string + Three: + type: object + allOf: + - $ref: "#/components/schemas/One" + - $ref: "#/components/schemas/Two" +``` + +Smithy: +```smithy +@mixin +structure One { + one: String +} + +@mixin +structure Two { + two: String +} + +structure Three with [One, Two] {} +``` + +Here we can see that both parents, `One` and `Two` are converted into mixins and used as such on `Three`. + +2. The parents structures are used as mixins and referenced as member targets + +OpenAPI: +```yaml +openapi: '3.0.' +info: + title: doc + version: 1.0.0 +paths: {} +components: + schemas: + One: + type: object + properties: + one: + type: string + Two: + type: object + allOf: + - $ref: "#/components/schemas/One" + Three: + type: object + properties: + one: + $ref: "#/components/schemas/One" +``` + +Smithy: +```smithy +@mixin +structure OneMixin { + one: String +} + +structure One with [OneMixin] {} + +structure Two with [OneMixin] {} + +structure Three { + one: One +} +``` + +Here `One` is used as a target of the `Three$one` member and is used as a mixin in the `Two` structure. Since smithy does not allow mixins to be used as targets, we have to create a separate mixin shape, `OneMixin` which is used as a mixin for `One` which is ultimately what we use for the target in `Three`. + +3. One of the parents is a document rather than a structure + +OpenAPI: +```yaml +openapi: '3.0.' +info: + title: doc + version: 1.0.0 +paths: {} +components: + schemas: + One: + type: object + properties: {} + Two: + type: object + properties: + two: + type: string + Three: + type: object + allOf: + - $ref: "#/components/schemas/One" + - $ref: "#/components/schemas/Two" +``` + +Smithy: +```smithy +document One + +structure Two { + two: String +} + +document Three +``` + +In this case, no mixins are created since none are ultimately used. Since `One` is translated to a document, `Three` must also be a document since it has `One` as a parent shape. As such, `Two` is never used as a mixin. + ##### Untagged Union The majority of `oneOf` schemas in OpenAPI represent untagged unions. From fa5ad05e48635b284fbffb3a4d2472a81145b9c2 Mon Sep 17 00:00:00 2001 From: Jeff Lewis Date: Mon, 21 Aug 2023 12:19:58 -0600 Subject: [PATCH 8/8] remove noParents field that isn't used --- .../src/internals/postprocess/TaggedUnionTransformer.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala b/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala index f204bb6..83f1834 100644 --- a/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala +++ b/modules/openapi/src/internals/postprocess/TaggedUnionTransformer.scala @@ -57,8 +57,7 @@ object TaggedUnionTransformer extends IModelPostProcessor { val singleLocal = allFields.size == 1 val allLocalRequired = allFields.forall(_.hints.contains(Hint.Required)) - val noParents = true // parents.isEmpty - if (singleLocal && allLocalRequired && noParents) + if (singleLocal && allLocalRequired) info.addFields(allFields) else info.setNotTagged }