Skip to content
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.

Sign up for GitHub

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

Jump to bottom

allow passing in the MaxArity for the JsonSchemaVisitor #569

Merged
merged 19 commits into from
Nov 16, 2022
Merged

allow passing in the MaxArity for the JsonSchemaVisitor #569

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
848cad8
allow passing in the MaxArity for the JsonSchemaVisitor
yisraelU Nov 3, 2022
1ee1876
revert changes to restJsonBuilder, rather provide capabilities in Cli…
yisraelU Nov 3, 2022
296711d
remove scala 3 wildcards
yisraelU Nov 3, 2022
ae94c4d
adds abstract SimpleRestJsonBuilder layer to type the codec as
yisraelU Nov 4, 2022
11ad06b
remove withJsonCodec method
yisraelU Nov 4, 2022
fe7dd01
remove codecApi variable
yisraelU Nov 4, 2022
046a4f9
formatting
yisraelU Nov 4, 2022
90af9d8
remove withMaxArity method as unnecessary
yisraelU Nov 8, 2022
30c8e1c
add withMaxArity method and remove case class
yisraelU Nov 8, 2022
8b2085c
Merge branch 'series/0.17' into maxArity
yisraelU Nov 9, 2022
2d9ba19
moved maxArity method to class
yisraelU Nov 10, 2022
31a737e
Merge remote-tracking branch 'origin/maxArity' into maxArity
yisraelU Nov 10, 2022
d28ed75
documentation for maxArity metod
yisraelU Nov 11, 2022
5485d41
add example for using max arity
yisraelU Nov 11, 2022
be2c463
Merge remote-tracking branch 'origin/series/0.17' into maxArity
Baccata Nov 12, 2022
0c112da
add deprecation warning to SimpleRestJsonBuilder extension method
yisraelU Nov 14, 2022
bdcdcbc
Merge remote-tracking branch 'origin/maxArity' into maxArity
yisraelU Nov 14, 2022
7237dd4
fmt
yisraelU Nov 14, 2022
f58aacf
remove use of deprecated extension method
yisraelU Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/example/src/smithy4s/example/Launcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ object Launcher extends IOApp {
val serverRes = for {
impl <- Resource.eval(ObjectServiceImpl.makeIO)
docs = smithy4s.http4s.swagger.docs[IO](ObjectService)
service <- SimpleRestJsonBuilder.routes(impl).resource
service <- SimpleRestJsonBuilder.build.routes(impl).resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure I like the idea of building the builder like this. Seems like the ideal UX would be something like:

SimpleRestJsonBuilder.routes(impl).withMaxJsonArity(2048).resource

where the call to withMaxJsonArity is not necessary if you want to use the default.

We could also take the more flexible approach of:

SimpleRestJsonBuilder.routes(impl).withJsonCodec(myJsonCodecApi).resource

What do you think?

Copy link
Contributor Author

@yisraelU yisraelU Nov 3, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree,
The problem is that SimpleProtocolBuilder doesnt have access to the JsonCodecApi as its typed to CodecApi, so we cant access the existing codec to make a copy .
We can create a new instance of JsonCodecApi inside the builder , or allow a user to pass one in , which i think is more safe and deterministic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it even need to be typed to JsonCodecApi? Couldn't it be just CodecApi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubukoz does it make sense to allow any codec on a RestJsonBuilder ?

Copy link
Member

@kubukoz kubukoz Nov 3, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm now that you said it out loud...

I'd say not in general. But requiring a JSON codec on SimpleProtocolBuilder is worse.

Maybe let's add the method inside SRJB instead of the generic builder, then? And allow SimpleProtocolBuilder users to override the CodecAPI as they see fit (at the very least, with a protected method so that they're forced to subclass the protocol builder)

app = (docs <+> service).orNotFound
server <- EmberServerBuilder.default[IO].withHost(host).withPort(port).withHttpApp(app).build
} yield ()
Expand Down
2 changes: 1 addition & 1 deletion modules/guides/src/smithy4s/guides/Cors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ object Routes {
.getOrElse(noMiddleware)

private val helloRoutes: Resource[IO, HttpRoutes[IO]] =
SimpleRestJsonBuilder.routes(HelloWorldImpl).resource
SimpleRestJsonBuilder.build.routes(HelloWorldImpl).resource

val all: Resource[IO, HttpRoutes[IO]] =
helloRoutes.map(r => corsMiddleWare(r))
Expand Down
6 changes: 5 additions & 1 deletion modules/http4s/src/smithy4s/http4s/SimpleRestJsonBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package http4s

import smithy4s.internals.InputOutput

object SimpleRestJsonBuilder
case class SimpleRestJsonBuilder(jsonMaxArity:Int)
extends SimpleProtocolBuilder[alloy.SimpleRestJson](
smithy4s.http.json.codecs(
alloy.SimpleRestJson.protocol.hintMask ++ HintMask(
Expand All @@ -28,3 +28,7 @@ object SimpleRestJsonBuilder
)
)
)

object SimpleRestJsonBuilder {
def build: SimpleRestJsonBuilder = SimpleRestJsonBuilder(1024)
}
4 changes: 2 additions & 2 deletions modules/http4s/src/smithy4s/http4s/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ package object http4s extends Compat.Package {
private[this] val serviceProvider: smithy4s.Service.Provider[Alg, Op]
) {

def simpleRestJson: SimpleRestJsonBuilder.ServiceBuilder[Alg, Op] =
SimpleRestJsonBuilder(serviceProvider.service)
def simpleRestJson: SimpleRestJsonBuilder#ServiceBuilder[Alg, Op] =
SimpleRestJsonBuilder.build(serviceProvider.service)

}

Expand Down
20 changes: 14 additions & 6 deletions modules/json/src/smithy4s/http/json/codecs.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ import smithy4s.schema.SchemaVisitor
import smithy4s.schema.CompilationCache

final case class codecs(
hintMask: HintMask = codecs.defaultHintMask
) extends JsonCodecAPI(codecs.schemaVisitorJCodec(_), Some(hintMask))
hintMask: HintMask = codecs.defaultHintMask,
maxArity: Int = codecs.defaultMaxArity
) extends JsonCodecAPI(codecs.schemaVisitorJCodec(_, maxArity), Some(hintMask)){
def withMaxArity(maxArity: Int): codecs = copy(maxArity = maxArity)
}

object codecs {

Expand All @@ -43,13 +46,18 @@ object codecs {
// TODO: add tests for `codecs` understanding int enums. Maybe pizza spec
IntEnum
)
val defaultMaxArity: Int = 1024

private[smithy4s] def schemaVisitorJCodec(
cache: CompilationCache[JCodec]
): SchemaVisitor[JCodec] =
new SchemaVisitorJCodec(maxArity = 1024, cache)
cache: CompilationCache[JCodec],
maxArity: Int = defaultMaxArity
): SchemaVisitor[JCodec] =
new SchemaVisitorJCodec(maxArity, cache)

private[smithy4s] val schemaVisitorJCodec: SchemaVisitor[JCodec] =
new SchemaVisitorJCodec(maxArity = 1024, CompilationCache.nop[JCodec])
new SchemaVisitorJCodec(
maxArity = defaultMaxArity,
CompilationCache.nop[JCodec]
)
Comment on lines +56 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this version currently used for? From a quick look it seems like only in tests. Maybe we can remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure what you mean by version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some documentation showing how to increase the maxArity and what that means

good point

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure what you mean by version?

Yeah sorry that is super confusing! I meant the "Val" vs the "def" above. The val I'm not sure is used anywhere. Thought we could remove it if that is in fact the case.


}