-
Notifications
You must be signed in to change notification settings - Fork 93
Fix bug around Lazy caching and update Lazy model #1857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: series/0.18
Are you sure you want to change the base?
Changes from all commits
cab833b
919b48d
a91c12a
8a9b3d9
e8d2216
20c7b0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package smithy4s.example | ||
|
|
||
| import smithy4s.Hints | ||
| import smithy4s.Schema | ||
| import smithy4s.ShapeId | ||
| import smithy4s.ShapeTag | ||
| import smithy4s.schema.Schema.int | ||
| import smithy4s.schema.Schema.struct | ||
|
|
||
| final case class LeafNode(value: Int) | ||
|
|
||
| object LeafNode extends ShapeTag.Companion[LeafNode] { | ||
| val id: ShapeId = ShapeId("smithy4s.example", "LeafNode") | ||
|
|
||
| val hints: Hints = Hints.empty | ||
|
|
||
| // constructor using the original order from the spec | ||
| private def make(value: Int): LeafNode = LeafNode(value) | ||
|
|
||
| implicit val schema: Schema[LeafNode] = struct( | ||
| int.required[LeafNode]("value", _.value), | ||
| )(make).withId(id).addHints(hints) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package smithy4s.example | ||
|
|
||
| import smithy4s.Hints | ||
| import smithy4s.Schema | ||
| import smithy4s.ShapeId | ||
| import smithy4s.ShapeTag | ||
| import smithy4s.schema.Schema.bijection | ||
| import smithy4s.schema.Schema.recursive | ||
| import smithy4s.schema.Schema.union | ||
|
|
||
| sealed trait Tree extends scala.Product with scala.Serializable { self => | ||
| @inline final def widen: Tree = this | ||
| def $ordinal: Int | ||
|
|
||
| object project { | ||
| def tree: Option[TreeNode] = Tree.TreeCase.alt.project.lift(self).map(_.tree) | ||
| def leaf: Option[LeafNode] = Tree.LeafCase.alt.project.lift(self).map(_.leaf) | ||
| } | ||
|
|
||
| def accept[A](visitor: Tree.Visitor[A]): A = this match { | ||
| case value: Tree.TreeCase => visitor.tree(value.tree) | ||
| case value: Tree.LeafCase => visitor.leaf(value.leaf) | ||
| } | ||
| } | ||
| object Tree extends ShapeTag.Companion[Tree] { | ||
|
|
||
| def tree(tree: TreeNode): Tree = TreeCase(tree) | ||
| def leaf(leaf: LeafNode): Tree = LeafCase(leaf) | ||
|
|
||
| val id: ShapeId = ShapeId("smithy4s.example", "Tree") | ||
|
|
||
| val hints: Hints = Hints.empty | ||
|
|
||
| final case class TreeCase(tree: TreeNode) extends Tree { final def $ordinal: Int = 0 } | ||
| final case class LeafCase(leaf: LeafNode) extends Tree { final def $ordinal: Int = 1 } | ||
|
|
||
| object TreeCase { | ||
| val hints: Hints = Hints.empty | ||
| val schema: Schema[Tree.TreeCase] = bijection(TreeNode.schema.addHints(hints), Tree.TreeCase(_), _.tree) | ||
| val alt = schema.oneOf[Tree]("tree") | ||
| } | ||
| object LeafCase { | ||
| val hints: Hints = Hints.empty | ||
| val schema: Schema[Tree.LeafCase] = bijection(LeafNode.schema.addHints(hints), Tree.LeafCase(_), _.leaf) | ||
| val alt = schema.oneOf[Tree]("leaf") | ||
| } | ||
|
|
||
| trait Visitor[A] { | ||
| def tree(value: TreeNode): A | ||
| def leaf(value: LeafNode): A | ||
| } | ||
|
|
||
| object Visitor { | ||
| trait Default[A] extends Visitor[A] { | ||
| def default: A | ||
| def tree(value: TreeNode): A = default | ||
| def leaf(value: LeafNode): A = default | ||
| } | ||
| } | ||
|
|
||
| implicit val schema: Schema[Tree] = recursive(union( | ||
| Tree.TreeCase.alt, | ||
| Tree.LeafCase.alt, | ||
| ){ | ||
| _.$ordinal | ||
| }.withId(id).addHints(hints)) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package smithy4s.example | ||
|
|
||
| import smithy4s.Hints | ||
| import smithy4s.Schema | ||
| import smithy4s.ShapeId | ||
| import smithy4s.ShapeTag | ||
| import smithy4s.schema.Schema.recursive | ||
| import smithy4s.schema.Schema.struct | ||
|
|
||
| final case class TreeNode(left: Tree, right: Tree) | ||
|
|
||
| object TreeNode extends ShapeTag.Companion[TreeNode] { | ||
| val id: ShapeId = ShapeId("smithy4s.example", "TreeNode") | ||
|
|
||
| val hints: Hints = Hints.empty | ||
|
|
||
| // constructor using the original order from the spec | ||
| private def make(left: Tree, right: Tree): TreeNode = TreeNode(left, right) | ||
|
|
||
| implicit val schema: Schema[TreeNode] = recursive(struct( | ||
| Tree.schema.required[TreeNode]("left", _.left), | ||
| Tree.schema.required[TreeNode]("right", _.right), | ||
| )(make).withId(id).addHints(hints)) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| package smithy4s | ||
|
|
||
| import munit.FunSuite | ||
|
|
||
| import smithy4s.example.{Tree, TreeNode, LeafNode, Foo} | ||
| import cats.Hash | ||
| import smithy4s.schema._ | ||
| import scala.annotation.tailrec | ||
| import smithy4s.internals.maps.MMap | ||
| import smithy4s.interopcats.SchemaVisitorHash | ||
| import smithy4s.schema.Schema.recursive | ||
| import smithy4s.schema.Schema._ | ||
|
|
||
| class RecursiveSpec extends FunSuite { | ||
|
|
||
| case class Recurse(n: Option[Recurse]) | ||
|
|
||
| object Recurse { | ||
| implicit val schema: Schema[Recurse] = recursive { | ||
| struct( | ||
| schema.optional[Recurse]("n", _.n) | ||
| )(Recurse.apply) | ||
| } | ||
| } | ||
|
|
||
| def buildTree(size: Int): Tree = { | ||
| val seed = Math.round(Math.random() * 1000).toInt | ||
| val nodes = (1 to size).map(i => Tree.leaf(LeafNode(i * seed))).toList | ||
|
|
||
| @tailrec() | ||
| def recursiveFold(els: List[Tree]): Tree = | ||
| els match { | ||
| case head :: Nil => head | ||
| case x => { | ||
| val joined: List[Tree] = x | ||
| .sliding(2, 2) | ||
| .flatMap { | ||
| case left :: right :: Nil => | ||
| List(Tree.tree(TreeNode(left, right))) | ||
| case x => x | ||
| } | ||
| .toList | ||
| recursiveFold(joined) | ||
| } | ||
| } | ||
|
|
||
| recursiveFold(nodes) | ||
| } | ||
|
|
||
| def buildRecursive(size: Int): Recurse = | ||
| (1 to size).foldLeft(Recurse(None))((tail, i) => Recurse(Some(tail))) | ||
|
|
||
| def useLazyTestCache[F[_]](store: MMap[Any, Any]) = new CompilationCache[F] { | ||
| override def getOrElseUpdate[A]( | ||
| schema: Schema[A], | ||
| fetch: Schema[A] => F[A] | ||
| ): F[A] = { | ||
| store.getOrElseUpdate(schema, fetch(schema)).asInstanceOf[F[A]] | ||
| } | ||
| } | ||
|
|
||
| def ignoreLazyTestCache[F[_]](store: MMap[Any, Any]) = | ||
| new CompilationCache[F] { | ||
| override def getOrElseUpdate[A]( | ||
| schema: Schema[A], | ||
| fetch: Schema[A] => F[A] | ||
| ): F[A] = { | ||
| if (schema.isInstanceOf[Schema.LazySchema[_]]) { fetch(schema) } | ||
| else store.getOrElseUpdate(schema, fetch(schema)).asInstanceOf[F[A]] | ||
| } | ||
| } | ||
|
|
||
| def runTest[A]( | ||
| caseString: String, | ||
| value: Int => A, | ||
| buildCache: MMap[Any, Any] => CompilationCache[Hash], | ||
| transformSchema: Schema[A] => Schema[A] = (x: Schema[A]) => x | ||
| )(implicit schema: Schema[A]) = | ||
| test(s"$caseString") { | ||
| val store: MMap[Any, Any] = MMap.empty | ||
|
|
||
| val updatedSchema = transformSchema(schema) | ||
|
|
||
| val hashVisitor: Hash[A] = | ||
| SchemaVisitorHash.fromSchema(updatedSchema, buildCache(store)) | ||
|
|
||
| // Invoke hash with a size that will have some recursion so that the build cache an be materialized | ||
| hashVisitor.hash(value(2)) | ||
| val sizeAfterInitializing = store.size | ||
|
|
||
| val sizes = List(10, 100, 256) | ||
| sizes.foreach(i => hashVisitor.hash(value(i))) | ||
| val sizeAfterHashing = store.size | ||
|
|
||
| assertEquals( | ||
| sizeAfterHashing, | ||
| sizeAfterInitializing, | ||
| "cache store size has grown after initialization" | ||
| ) | ||
| } | ||
|
|
||
| def addHints[A](schema: Schema[A]): Schema[A] = { | ||
| schema.transformHintsTransitively( | ||
| _.add( | ||
| smithy.api.Documentation("Adding some hints") | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| def runTestCases[A: Schema](caseString: String, value: Int => A) = { | ||
| runTest( | ||
| s"$caseString: current cache, hints unchanged", | ||
| value, | ||
| buildCache = ignoreLazyTestCache | ||
| ) | ||
| runTest( | ||
| s"$caseString: current cache, hints transformed", | ||
| value, | ||
| buildCache = ignoreLazyTestCache, | ||
| transformSchema = addHints[A] | ||
| ) | ||
| runTest( | ||
| s"$caseString: updated cache, hints unchanged", | ||
| value, | ||
| buildCache = useLazyTestCache | ||
| ) | ||
| runTest( | ||
| s"$caseString: updated cache, hints transformed", | ||
| value, | ||
| buildCache = useLazyTestCache, | ||
| transformSchema = addHints[A] | ||
| ) | ||
| } | ||
|
|
||
| runTestCases("Foo", Foo.int) | ||
| runTestCases("Tree", buildTree) | ||
| runTestCases("Recurse", buildRecursive) | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| package smithy4s | ||
| package schema | ||
|
|
||
| import smithy4s.internals.maps.MMap | ||
| import Schema._ | ||
| import scala.reflect.ClassTag | ||
|
|
||
|
|
@@ -214,6 +215,7 @@ object Schema { | |
| private final class TransitiveCompiler( | ||
| underlying: Schema ~> Schema | ||
| ) extends (Schema ~> Schema) { | ||
| val lazyCompileCache: MMap[Any, Any] = MMap.empty | ||
|
|
||
| def apply[A]( | ||
| fa: Schema[A] | ||
|
|
@@ -224,8 +226,14 @@ object Schema { | |
| underlying(u.copy(alternatives = u.alternatives.map(handleAlt(_)))) | ||
| case BijectionSchema(s, bijection) => | ||
| underlying(BijectionSchema(this(s), bijection)) | ||
| case LazySchema(suspend) => | ||
| underlying(LazySchema(suspend.map(this.apply))) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the problematic line here and it's actually 2 bugs in the same line. both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my PR, this fix can be simplified to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying your simplification causes some tests to fail. Need to investigate why. |
||
| case l @ LazySchema(suspend) => { | ||
| lazyCompileCache | ||
| .getOrElseUpdate( | ||
| l, | ||
| LazySchema(suspend.map(this.apply)) | ||
| ) | ||
| .asInstanceOf[Schema[A]] | ||
| } | ||
| case RefinementSchema(s, refinement) => | ||
| underlying(RefinementSchema(this(s), refinement)) | ||
| case c: CollectionSchema[c, a] => | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not necessarily required for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does making it a
sealed abstract classsolve the MiMa issue ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it does not.