Typesafe directive - for wrapping case classes on fields (incl. input types) and arguments #2091

develeon · 2024-01-21T15:53:58Z

By using @typesafe directive, the code generator will wrap your typesafe fields in case classes without
imposing further changes to GraphQl schema. Hence allowing type safety on important identifiers and fields.
Tested with Option and List as well.

Schema example:

directive @typesafe(name : String) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION

type Query {
  getUser(id: ID! @typesafe(name: "UserId")): User
}

type User {
  id: ID! @typesafe(name: "UserId")
  ...
}

with ID as scalar mapping to Int you would get something like this:

case class UserId(value: Int) extends AnyVal
object UserId {
  implicit val schema: Schema[Any, UserId] = summon[Schema[Any, Int]].contramap(_.value)
  implicit val argBuilder: ArgBuilder[UserId] = summon[ArgBuilder[Int]].map(UserId(_))
}

final case class QueryGetUserArgs(id: UserId)

final case class User(
  @GQLDirective(Directive("typesafe", Map("name" -> StringValue("UserId"))))
  id: UserId,
  ...
}

See SchemaWriterSpec.scala for more extensive type mapping.

oyvindberg · 2024-01-23T10:50:33Z

To give some background in case it's not obvious: We're working on improving the story for schema-first development. One thing we identified which we really wanted was to have statically typed IDs on the backend.

So given that preference, we identified a few paths forward:

add a scalar for each ID type to our schema. As far as I understand, this pushes some complexity over to the clients of the API in that they need to provide a mapping for them when generating code and so on. We want the API to be easily consumable, so that's not optimal.
add object types for each ID type in the schema. In this case clients will need to handle the wrapping themselves for all queries, and using the API will feel different and less convenient than a normal graphql API.
This solution where we "tag" ID's as being of different flavours with directives. This way, clients will just see ID like in any other graphql API. They are free to read the annotated ID types in the schema as documentation, or customize their own codegen in the same manner as we suggest here. I think this is a very powerful and flexible technique to solve this use case.

Any thoughts on this @ghostdogpr?

ghostdogpr · 2024-01-23T11:40:51Z

This looks good! I just need a little bit of time to review because it's code I'm not super familiar with (schema codegen was mostly contributed externally). Worst case scenario I'll take a look during the weekend 👍

@kyri-petrou if you have time feel free to review as well

oyvindberg · 2024-01-23T11:44:17Z

Nice!

One very obvious todo is that it needs to be documented, likely alongside the @lazy changes from earlier @develeon .

Can do that after we agree on the exact path forward here 👍

develeon · 2024-01-23T12:25:59Z

Nice!

One very obvious todo is that it needs to be documented, likely alongside the @lazy changes from earlier @develeon .

Can do that after we agree on the exact path forward here 👍

I'm all up for that. Also I think the @lazy directive seems very powerful, so it would be a pity if it where not to be used due to limited documentation.

oyvindberg · 2024-01-23T12:36:26Z

Oh, it is! check https://ghostdogpr.github.io/caliban/docs/server-codegen.html#lazy-evaluation . I meant we should document this on the same page

kyri-petrou · 2024-01-23T12:40:32Z

At a glance it looks good, but I wanna take some more time and understand the code better since I'm not too familiar with it either.

One question that comes to mind at a very high level, why @typesafe? Is this a directive already used by other schema-first libraries?

PS: I'm don't mind it either way, I'm just curious on whether there's any specific reasoning behind it :)

develeon · 2024-01-23T12:47:52Z

There is no particular reason for the directive name, so open for suggestions.
Had some other working names, but decided to go back to using @typesafe as feel that more descriptive to what we want to achieve.

ghostdogpr

About the name @typesafe sounds a little bit too general for what it does here? How about something like @typeAlias?

ghostdogpr · 2024-01-24T00:38:01Z

core/src/main/scala/caliban/parsing/adt/Directive.scala

+
+  val LazyDirective      = "lazy"
+  val TypesafeDirective  = "typesafe"
+  val DeprecatedDiretive = "deprecated"


typo: Diretive => Directive

ghostdogpr · 2024-01-24T00:38:49Z

core/src/main/scala/caliban/parsing/adt/Directive.scala

+  def isTypesafe(directives: List[Directive]): Boolean          =
+    directives.exists(_.name == TypesafeDirective)
+  def typesafeName(directives: List[Directive]): Option[String] =
+    Directives


You can call findDirective directly

Well spotted!

ghostdogpr · 2024-01-24T00:42:01Z

tools/src/main/scala/caliban/tools/SchemaWriter.scala

-    def writeInputValue(value: InputValueDefinition): String =
-      s"""${writeDescription(value.description)}${safeName(value.name)} : ${writeType(value.ofType)}"""
+    def writeGQLTypesafeDirective(field: FieldDefinition) =
+      if (field.directives.exists(_.name == TypesafeDirective)) {


nitpick: rather than exists + filter + head, you could use find and then fold on the Option. Same below.

Thanks I even found some further optimization cause of this :)

ghostdogpr · 2024-01-24T00:43:07Z

tools/src/main/scala/caliban/tools/SchemaWriter.scala

+      val fnName       = directive.arguments("name").toInputString.replace("\"", "")
+      val typesafeType = writeTypesafeType(fieldType)
+
+      s"""case class $fnName(value : $typesafeType) extends AnyVal


Do we need to sanitize the name?

kyri-petrou · 2024-01-24T02:05:23Z

How about something like @typeAlias

I was thinking the same. Another option would be @newtype but not sure how well known is that term

develeon · 2024-01-25T23:19:27Z

How about something like @typeAlias

I was thinking the same. Another option would be @newtype but not sure how well known is that term

I refactor to use @newtype, think at least from world of Haskell it is established and fit well with our application. But if you prefer @typeAlias I will update it.

Also added documentation.

kyri-petrou

Looks great overall! My main comment / concern is that unless I'm missing something, the summon keyword wouldn't work when generating Scala 2 code

kyri-petrou · 2024-01-28T07:28:31Z

core/src/main/scala/caliban/parsing/adt/Directive.scala

+  val LazyDirective       = "lazy"
+  val NewtypeDirective    = "newtype"
+  val DeprecatedDirective = "deprecated"


Very minor, but perhaps make these final val instead

kyri-petrou · 2024-01-28T07:34:55Z

tools/src/main/scala/caliban/tools/SchemaWriter.scala

+
+      s"""case class $fnName(value : $newtype) extends AnyVal
+         |object $fnName {
+         |    implicit val schema: Schema[Any, $fnName] = summon[Schema[Any, $newtype]].contramap(_.value)


I'm a bit confused, does summon work for Scala 2?

Also, perhaps just use Schema[$newtype].contramap(_.value) instead, the Schema.apply method "summons" the implicit Schema

Could not get that to work, but used implicitly (Scala 2 version of summon).
Tested it on my Scala 3 setup and that worked, but have to double check my Scala options.

To catch these kind of issues it would be nice to add the new annotation to the sbt plug-in "scripted" test suite because it runs both on scala 2 and scala 3

kyri-petrou · 2024-01-28T07:35:19Z

tools/src/main/scala/caliban/tools/SchemaWriter.scala

+      s"""case class $fnName(value : $newtype) extends AnyVal
+         |object $fnName {
+         |    implicit val schema: Schema[Any, $fnName] = summon[Schema[Any, $newtype]].contramap(_.value)
+         |    implicit val argBuilder: ArgBuilder[$fnName] = summon[ArgBuilder[$newtype]].map($fnName(_))


Same as above, I believe you can use [ArgBuilder[$newtype].map($fnName(_))

kyri-petrou

I've never worked on this part of the codebase, but AFAICT it looks great :) The only thing that "saddens" me a bit is that we don't get to utilize Scala 3's opaque types, but I think it's likely for the best to have a unified Scala 2/3 implementation.

Thank you very much for the contribution, really highly appreciated!

develeon · 2024-01-29T13:59:33Z

I've never worked on this part of the codebase, but AFAICT it looks great :) The only thing that "saddens" me a bit is that we don't get to utilize Scala 3's opaque types, but I think it's likely for the best to have a unified Scala 2/3 implementation.

Thank you very much for the contribution, really highly appreciated!

If we are able to support opaque types in the future I think we can just add a type argument to the directive.
ex. directive @newtype(name : String, type : String)

But I did try bit with opaque types initially and ran into a issue, if I recall it was:
Failed to synthesize an instance of type deriving.Mirror.Of

Hence similar to the issue for union types: #1926

kyri-petrou · 2024-01-30T00:35:59Z

But I did try bit with opaque types initially and ran into a issue, if I recall it was: Failed to synthesize an instance of type deriving.Mirror.Of

With opaque types, you have to generate the Schema in the companion object of the opaque type for it to work, something along these lines:

import scala.compiletime.summonInline

opaque type Foo = Int
object Foo {
  inline given Schema[Any, Foo] = summonInline[Schema[Any, Int]]
}

Either way, I think let's steer off opaque types for now and we can revisit this if there is interest in having that option

vuepress/docs/docs/server-codegen.md

Added scripted test for newtype directive

develeon · 2024-01-30T08:40:18Z

Removed documentation from this PR
and created new PR with it here : #2101

ghostdogpr · 2024-01-30T08:41:05Z

Thanks!

develeon force-pushed the series/2.x_typesafe_directive branch 2 times, most recently from d64bcb9 to 9657ad0 Compare January 21, 2024 22:15

ghostdogpr reviewed Jan 24, 2024

View reviewed changes

develeon force-pushed the series/2.x_typesafe_directive branch from 678ee91 to 977b8fa Compare January 25, 2024 23:07

develeon force-pushed the series/2.x_typesafe_directive branch from d950194 to 7389539 Compare January 27, 2024 19:09

kyri-petrou requested changes Jan 28, 2024

View reviewed changes

develeon force-pushed the series/2.x_typesafe_directive branch from 0666805 to 8e99d73 Compare January 28, 2024 13:34

develeon requested review from kyri-petrou and ghostdogpr January 29, 2024 11:33

kyri-petrou approved these changes Jan 29, 2024

View reviewed changes

ghostdogpr approved these changes Jan 30, 2024

View reviewed changes

ghostdogpr reviewed Jan 30, 2024

View reviewed changes

vuepress/docs/docs/server-codegen.md Outdated Show resolved Hide resolved

develeon added 4 commits January 30, 2024 09:24

Added Typesafe directive

e810d3a

avoided using distinctBy due to cross compatibility issue.

084ddbb

Renamed directive to Newtype and simplified statements with fold.

aeb6bc4

Updated to use implicitly instead of summon for scala 2 support.

e155487

Added scripted test for newtype directive

develeon force-pushed the series/2.x_typesafe_directive branch from 0950347 to e155487 Compare January 30, 2024 08:25

develeon mentioned this pull request Jan 30, 2024

Series/2.x Documentation for newtype directive #2101

Merged

ghostdogpr merged commit 9c3d9c9 into ghostdogpr:series/2.x Jan 30, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typesafe directive - for wrapping case classes on fields (incl. input types) and arguments #2091

Typesafe directive - for wrapping case classes on fields (incl. input types) and arguments #2091

develeon commented Jan 21, 2024 •

edited

oyvindberg commented Jan 23, 2024

ghostdogpr commented Jan 23, 2024

oyvindberg commented Jan 23, 2024

develeon commented Jan 23, 2024

oyvindberg commented Jan 23, 2024

kyri-petrou commented Jan 23, 2024

develeon commented Jan 23, 2024 •

edited

ghostdogpr left a comment

ghostdogpr Jan 24, 2024

ghostdogpr Jan 24, 2024

develeon Jan 25, 2024

ghostdogpr Jan 24, 2024

develeon Jan 25, 2024

ghostdogpr Jan 24, 2024

kyri-petrou commented Jan 24, 2024

develeon commented Jan 25, 2024 •

edited

kyri-petrou left a comment

kyri-petrou Jan 28, 2024

kyri-petrou Jan 28, 2024

develeon Jan 28, 2024

ghostdogpr Jan 28, 2024

kyri-petrou Jan 28, 2024

kyri-petrou left a comment

develeon commented Jan 29, 2024

kyri-petrou commented Jan 30, 2024

develeon commented Jan 30, 2024

ghostdogpr commented Jan 30, 2024

Typesafe directive - for wrapping case classes on fields (incl. input types) and arguments #2091

Typesafe directive - for wrapping case classes on fields (incl. input types) and arguments #2091

Conversation

develeon commented Jan 21, 2024 • edited

oyvindberg commented Jan 23, 2024

ghostdogpr commented Jan 23, 2024

oyvindberg commented Jan 23, 2024

develeon commented Jan 23, 2024

oyvindberg commented Jan 23, 2024

kyri-petrou commented Jan 23, 2024

develeon commented Jan 23, 2024 • edited

ghostdogpr left a 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

kyri-petrou commented Jan 24, 2024

develeon commented Jan 25, 2024 • edited

kyri-petrou left a 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

kyri-petrou left a comment

Choose a reason for hiding this comment

develeon commented Jan 29, 2024

kyri-petrou commented Jan 30, 2024

develeon commented Jan 30, 2024

ghostdogpr commented Jan 30, 2024

develeon commented Jan 21, 2024 •

edited

develeon commented Jan 23, 2024 •

edited

develeon commented Jan 25, 2024 •

edited