add hint mask construct #70

lewisjkl · 2022-01-21T22:00:44Z

Introduces the concept of HintMask that is derived from smithy protocolDefinitions

lewisjkl · 2022-01-21T23:34:09Z

modules/http4s/src/smithy4s/http4s/SimpleRestJsonBuilder.scala

+      smithy4s.http.json.codecs(
+        HintMask()
+      ) // TODO smithy4s.api.SimpleRestJson.hintMask??


It would be great it we could apply the hint mask across the codecs from the SimpleProtocolBuilder rather than needing to do it everywhere. The issue I had with doing this was that the SimpleProtocolBuilder does not have direct access to the schematic (it works with the CodecAPI).

My initial thought here was that we could codegen the hintMask for protocols as part of the rendering. Not sure if there is a better way. The current protocols only have a list of TraitShapeId like: smithy.api.ProtocolDefinition(Some(List(smithy.api.TraitShapeId("smithy.api#cors"), .... I tried looking at the renderer, but couldn't confidently figure out how to begin approaching this 😅

It would be great it we could apply the hint mask across the codecs from the SimpleProtocolBuilder

We could add a method to the CodecAPI interface tbh.

I tried looking at the renderer, but couldn't confidently figure out how to begin approaching this

that's cool, we'll have a brainstorm next week, but the progress at this point in time is already very satisfying.

modules/json/src/smithy4s/http/json/codecs.scala

lewisjkl · 2022-01-21T23:35:44Z

modules/protocol/resources/META-INF/smithy/smithy4s.smithy

+@protocolDefinition(traits: [
+    "smithy.api#cors",
+    "smithy.api#endpoint",
+    "smithy.api#hostLabel",
+    "smithy.api#http",
+    "smithy.api#httpError",
+    "smithy.api#httpHeader",
+    "smithy.api#httpLabel",
+    "smithy.api#httpPayload",
+    "smithy.api#httpPrefixHeaders",
+    "smithy.api#httpQuery",
+    "smithy.api#httpQueryParams",
+    "smithy.api#httpResponseCode",
+    "smithy.api#jsonName",
+    "smithy.api#timestampFormat"
+])


These are all just copied from the aws restJson1 protocol. Some of them may not apply here and we have a few others that need to be added (e.g. discriminated)

modules/core/src/smithy4s/HintMask.scala

kubukoz · 2022-01-21T23:32:24Z

modules/core/src/smithy4s/syntax/package.scala

@@ -113,4 +113,8 @@ package object syntax
      OneOf(oneOf.label, oneOf.schema.withHints(hints: _*), oneOf.inject)
  }

+  implicit class withMaskSyntax[F[_]](val s: Schematic[F]) extends AnyVal {


s should be private, otherwise you can do schematic.s.s.s.s.s I think

Mmm not sure it should leave there tbh, nor that it should be a extension, but 🤷

Fair enough, I think it has proved less useful than I thought it would initially anyway

kubukoz · 2022-01-21T23:34:50Z

modules/protocol/resources/META-INF/smithy/smithy4s.smithy

@@ -14,7 +14,22 @@ namespace smithy4s.api
 @uuidFormat
 string UUID

-@protocolDefinition
+@protocolDefinition(traits: [
+    "smithy.api#cors",


Does @discriminated belong here?

Also @ error, if it's a thing (can't remember, on phone, just spitballing)

didn't see the previous comments when I was writing this :D

No worries :) appreciate the comments!

kubukoz · 2022-01-21T23:35:15Z

modules/protocol/resources/META-INF/smithy/smithy4s.smithy

+    "smithy.api#httpHeader",
+    "smithy.api#httpLabel",
+    "smithy.api#httpPayload",
+    "smithy.api#httpPrefixHeaders",


I gotta learn what this is :D

modules/json/src/smithy4s/http/json/codecs.scala

Baccata · 2022-01-22T09:56:07Z

modules/core/src/smithy4s/HintMask.scala

+abstract class HintMask {
+  protected def toSet: Set[Hints.Key[_]]
+  def ++(other: HintMask): HintMask
+  def apply(hints: Hints): Hints


Baccata · 2022-01-22T09:56:22Z

modules/core/src/smithy4s/HintMask.scala

+  private[this] final class MaskSchematic[F[_]](
+      schematic: Schematic[F],
+      mask: HintMask
+  ) extends PassthroughSchematic[F](schematic) {


good idea !

Baccata · 2022-01-22T09:56:29Z

modules/core/src/smithy4s/HintMask.scala

+      mask: HintMask
+  ) extends PassthroughSchematic[F](schematic) {
+    override def withHints[A](fa: F[A], hints: Hints): F[A] =
+      schematic.withHints(fa, mask(hints))


Baccata · 2022-01-22T10:32:31Z

modules/core/src/smithy4s/PassthroughSchematic.scala

+
+class PassthroughSchematic[F[_]](schematic: Schematic[F])
+    extends Schematic[F]
+    with GenericAritySchematic[F] {


So unfortunately, this Passthrough construct should not extend this. We'll have to use the build-time Boilerplate.

Is that because the schematics passed in may not be using the generic arity trait so they would rely on the other functions directly?

We could just delete the non-generic arity methods in the schematic interface. I think experience shows that we always use the generic one anyway.

We would however keep the arity-based methods in the syntax package, to create schemas. They would just all delegate to the generic-arity one.

Yeah, I am fine with that. I just implemented them all in case we decide to go that route.. I agree though that the generic route seems to be sufficient.

modules/core/src/smithy4s/HintMask.scala

Baccata · 2022-01-24T21:52:52Z

modules/core/src/smithy4s/syntax/package.scala

+    def mask(hintMask: HintMask): Schematic[F] = HintMask.mask(s, hintMask)
+  }
+
+  implicit def newTypeToHintKey[A](a: Newtype[A]): Hints.Key[_] = a.key


What happens if you return Hints.Key[A] here ? Does the generated code still compile ? If it does, we should do that, and maybe move this implicit def into the companion object of Hints.Key

Cool, I will give that a try

Doesn't compile like that:

type mismatch; found : smithy4s.Hints.Key[a.Type] required: smithy4s.Hints.Key[A]

Moved to the companion object though as I think that is better

modules/http4s/src/smithy4s/http4s/SimpleProtocolBuilder.scala

Baccata · 2022-01-24T21:59:14Z

modules/codegen/src/smithy4s/codegen/Renderer.scala

-      if (
-        hints.contains(Hint.ClientError) || hints.contains(Hint.ServerError)
-      ) {
+      if (hints.contains(Hint.Error)) {


Baccata · 2022-01-25T16:02:16Z

modules/core/src/smithy4s/http/CodecAPI.scala

@@ -40,7 +40,7 @@ trait CodecAPI {
    * @param schema the value's schema
    * @return the codec associated to the A value.
    */
-  def compileCodec[A](schema: Schema[A]): Codec[A]
+  def compileCodec[A](schema: Schema[A], hintMask: HintMask): Codec[A]


I don't really like this, the application of the hintMask should be an implementation detail of the CodecAPI instances I think

kubukoz · 2022-01-26T19:56:28Z

modules/http4s/src/smithy4s/http4s/SimpleRestJsonBuilder.scala

 object SimpleRestJsonBuilder
    extends SimpleProtocolBuilder[smithy4s.api.SimpleRestJson](
-      smithy4s.http.json.codecs
+      smithy4s.http.json.codecs(
+        smithy4s.api.SimpleRestJson.protocol.hintMask ++ HintMask(InputOutput)


nitpick (if-minor): Kind of makes me think the standard hints (smithy.api) should always be forwarded no matter what.

Nah : imagine you want to have a json RPC API and also serve it over http/restJson. You need a mechanism to ignore the http binding traits when using the RPC protocol (or re-implement a bespoke schematic)

seems like smithy.api is too generic a package then 😅

Co-authored-by: Olivier Mélois <[email protected]>

add hint mask construct

0f140d5

lewisjkl force-pushed the hint-masks branch from 52d2d42 to 0f140d5 Compare January 21, 2022 22:06

lewisjkl added 3 commits January 21, 2022 15:09

add mask syntax

36d6370

add second remove method to Hints src-3

1f247d2

work on applying hint mask to schematics from protocol builders

e0da2d6

lewisjkl commented Jan 21, 2022

View reviewed changes

modules/json/src/smithy4s/http/json/codecs.scala Outdated Show resolved Hide resolved

lewisjkl commented Jan 21, 2022

View reviewed changes

kubukoz reviewed Jan 21, 2022

View reviewed changes

Baccata reviewed Jan 22, 2022

View reviewed changes

lewisjkl added 2 commits January 24, 2022 13:53

update renderer to render hint masks

46e8159

create missing headers

0fd0120

Baccata reviewed Jan 24, 2022

View reviewed changes

modules/core/src/smithy4s/HintMask.scala Outdated Show resolved Hide resolved

lewisjkl added 3 commits January 24, 2022 14:44

remove Http specific hint traits from codegen

1898930

unify error hint in codegen

42d92a2

get rid of mask syntax

790786b

Baccata reviewed Jan 24, 2022

View reviewed changes

modules/http4s/src/smithy4s/http4s/SimpleProtocolBuilder.scala Outdated Show resolved Hide resolved

Baccata reviewed Jan 24, 2022

View reviewed changes

lewisjkl added 5 commits January 24, 2022 15:03

fun with structs

b2bfd03

relocate newTypeToHintKey conversion

97d9d3c

add smithy4s.api traits to simpleRestJson protocol

464129c

implement hints without having remove method

43bf07b

update AWS module with hintMask

8ff34da

Baccata added the ‼️ binary breaking ‼️ label Jan 25, 2022

Baccata added 2 commits January 25, 2022 14:57

Merge remote-tracking branch 'origin/main' into hint-masks

7e92aec

Only 1 struct method to implement now

95b5534

Baccata added 3 commits January 25, 2022 15:23

collectFirst + foldMap

e9753bd

Add HintMask.allAllowed, revert Aws changes

ec2facf

Fix benchmarks

bad5119

Baccata reviewed Jan 25, 2022

View reviewed changes

lewisjkl added 5 commits January 25, 2022 11:34

remove hintMask as parameter from CodecAPI

33d28d9

fix aws module compilation

fc6c1cd

generate headers

d99d85b

fix json module compilation

80e5d96

fix json benchmark

189e5d3

lewisjkl marked this pull request as ready for review January 25, 2022 20:55

lewisjkl added 2 commits January 25, 2022 13:59

remove hintmask from AwsProtocol

f11cda8

clean up simpleRestJson protocolDefinition traits

bd33588

Baccata approved these changes Jan 26, 2022

View reviewed changes

kubukoz approved these changes Jan 26, 2022

View reviewed changes

kubukoz merged commit 03a60cb into main Jan 26, 2022

kubukoz deleted the hint-masks branch January 26, 2022 19:58

kubukoz mentioned this pull request Jan 26, 2022

Add a "Hint Mask" construct #52

Closed

Baccata added a commit that referenced this pull request May 10, 2022

add hint mask construct (#70)

29ade3a

Co-authored-by: Olivier Mélois <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add hint mask construct #70

add hint mask construct #70

lewisjkl commented Jan 21, 2022 •

edited

Loading

lewisjkl Jan 21, 2022

Baccata Jan 22, 2022

lewisjkl Jan 21, 2022

kubukoz Jan 21, 2022

Baccata Jan 22, 2022

lewisjkl Jan 22, 2022

kubukoz Jan 21, 2022

kubukoz Jan 21, 2022

kubukoz Jan 22, 2022

lewisjkl Jan 22, 2022

kubukoz Jan 21, 2022

Baccata Jan 22, 2022

Baccata Jan 22, 2022

Baccata Jan 22, 2022

Baccata Jan 22, 2022

lewisjkl Jan 22, 2022

Baccata Jan 22, 2022

Baccata Jan 24, 2022

lewisjkl Jan 24, 2022

Baccata Jan 24, 2022

lewisjkl Jan 24, 2022

lewisjkl Jan 24, 2022

lewisjkl Jan 24, 2022

Baccata Jan 24, 2022

Baccata Jan 25, 2022

kubukoz Jan 26, 2022

Baccata Jan 26, 2022

kubukoz Jan 26, 2022

add hint mask construct #70

add hint mask construct #70

Conversation

lewisjkl commented Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewisjkl commented Jan 21, 2022 •

edited

Loading