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.

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

Permit scalarMappings to be used with enumerations #930

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

blast-hardcheese
Copy link
Contributor

There's a problem in the latest release of caliban, wherein if you attempt to use scalarMapping to remap an enum, it just interpolates the fully qualified type in the name position, which is syntacticly invalid.

Given: --scalarMappings ExampleStreamType:com.example.myserver.constants.ExampleStreamType, I get:

sealed trait com.example.myserver.constants.ExampleStreamType extends scala.Product with scala.Serializable
object com.example.myserver.constants.ExampleStreamType {
  case object FOO extends com.example.myserver.constants.ExampleStreamType
  case object BAR extends com.example.myserver.constants.ExampleStreamType
  ...

  implicit val decoder: ScalarDecoder[com.example.myserver.constants.ExampleStreamType] = {
    case __StringValue("FOO") => Right(com.example.myserver.constants.ExampleStreamType.FOO)
    case __StringValue("BAR") => Right(com.example.myserver.constants.ExampleStreamType.BAR)
    ...
    case other => Left(DecodingError(s"Can't build ExampleStreamType from input $other"))
  }
  implicit val encoder: ArgEncoder[ExampleStreamType] = new ArgEncoder[ExampleStreamType] {
    override def encode(value: com.example.myserver.constants.ExampleStreamType): __Value = value match {
      case ExampleStreamType.FOO => __EnumValue("FOO")
      case ExampleStreamType.BAR => __EnumValue("BAR")
      ...
    }
    override def typeName: String = "com.example.myserver.constants.ExampleStreamType"
  }
}

This PR addresses this issue by suppressing emitting the enumeration type if the enumeration name is in the scalarMappings map.

@blast-hardcheese
Copy link
Contributor Author

(I say "in the latest release of caliban", it has actually been the case since I started using it a few weeks ago. I've got a workaround by mapping caliban types to my domain types by hand during field selection, but I just got around to taking a crack at fixing this properly.)

@ghostdogpr
Copy link
Owner

Yeah I think the feature was always intended to work with scalars only.

Don't you miss the ArgEncoder for the mapped enum then? You have to add it on your own (which is pretty easy). Anyway, no problem adding this since it's not affecting anyone who don't make use of it.

@blast-hardcheese
Copy link
Contributor Author

blast-hardcheese commented Jun 19, 2021

Yeah, I've got ArgEncoder and ScalarDecoder being brought in via --imports

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@ghostdogpr ghostdogpr merged commit 7c92ec7 into ghostdogpr:master Jun 19, 2021
@blast-hardcheese blast-hardcheese deleted the scalarMappings-enum branch June 19, 2021 09:31
@blast-hardcheese
Copy link
Contributor Author

Perhaps a final thought: I think of an enumeration as being a scalar, which is why I thought of doing this in the first place -- I guess you could abuse types by mapping individual "scalar" string fields to full JSON objects, if you so chose, but I'm certainly not attempting that here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants