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

Support of JSON scalar type. #167

Closed
JurajBurian opened this issue Jan 17, 2020 · 9 comments · Fixed by #176
Closed

Support of JSON scalar type. #167

JurajBurian opened this issue Jan 17, 2020 · 9 comments · Fixed by #176
Labels
enhancement New feature or request server Issue related to caliban server

Comments

@JurajBurian
Copy link

In Several implementations of graphql there exists possibility to extend scalar types.
Blog: blog also mentions JSON scalar extension for an implementation of graphql.
I can’t see any easy way how to do it with current implementation of caliban.
If you folks agree that it should be implemented, I can try to make some PR in near future.

@ghostdogpr
Copy link
Owner

Using Schema.scalarSchema you can easily create custom scalars already no? Like what I did for Unit.

For which type would you add a schema that returns a Json scalar? Something like Circe Json? There’s an Encoder[ResponseValue] , maybe we should add a Decoder too?

@JurajBurian
Copy link
Author

Hello,
thanks for comment, yes Circe Json is ok. Please, look to mine fork.
I tried to solve problem introducing :

 trait GenericScalar[T] extends Value {
   def toJson: Json
 }

Also an implementation of input value encoder: Encoder[InputValue] is necessary.

Then in mine project I am able to define :

final case class JsonScalar(json:Json) extends Value.GenericScalar[Json] {
   override def toString(): String = json.noSpaces
   override def toJson: Json = json
 }
implicit val jsonScalarSchema = schema.scalarSchema[Json]("JSON", Some("Json extension of scalar value."), JsonScalar(_))

 implicit val jsonArgBuilder: ArgBuilder[Json] = {
   case v:InputValue.ObjectValue  =>
     Right(InputValue.circeEncoder(v))
   case other          =>
     Left(ExecutionError(s"Can't build a Json from input: $other"))
 }

I am not very happy with solution, but is working.
If we agree to continue in this approach, I also recommend creating mini artifact named "caliban-json-extension" where we can insert the things defined in the “project”.

@ghostdogpr
Copy link
Owner

ghostdogpr commented Jan 21, 2020

Why do we need GenericScalar? Can't you just do:

implicit val jsonScalarSchema = schema.scalarSchema[Json]("JSON", Some("Json extension of scalar value."), ???)

With ??? being a function that transforms Json to ResponseValue (very similar to existing jsonToValue). Json can already be expressed using ResponseValue so we don't need an extra type. Or am I missing something?

By the way FYI, to include JSON encoders and decoders for Value, GraphQLRequest and GraphQLResponse inside Caliban core without needing a separate dependency but also without forcing the circe dependency on all users, we used the technique described in this blog post: https://blog.7mind.io/no-more-orphans.html relying on an Optional dependency.

@JurajBurian
Copy link
Author

Ok. I see point. Question is, do you want extend project with extensions ? I can create caliban-circe- json-extension, containing all necessary stuff:
(package : caliban.extension.scalar)

object json {

  private def jsonToValue(json: Json): ResponseValue = ...
  private val inputValueEncoder: Encoder[InputValue] =  ...
  implicit val jsonScalarSchema =  new Schema[Any, Json] {
    override def toType(isInput: Boolean): __Type = Types.makeScalar("JSON", Some("Scalar of json type."))
    override def resolve(value: Json): Step[Any]     = PureStep(jsonToValue(value))
  }

  implicit val jsonArgBuilder: ArgBuilder[Json] = {
    case v:InputValue.ObjectValue =>  Right(inputValueEncoder(v))
    case other                    =>  Left(ExecutionError(s"Can't build a Json from input: $other"))
  }
}

I believe that that lot of people can use cicrce json directly ...

@ghostdogpr
Copy link
Owner

I think we can use the same technique I mentioned earlier (as shown in https://github.com/ghostdogpr/caliban/blob/master/core/src/main/scala/caliban/interop/circe/circe.scala), so we don’t need a separate module. What do you think?

@JurajBurian
Copy link
Author

I recommended separate module (artifact) because scalar Json support is an extension (not part of specification of graphql).
Also extension can be understand as "black box solution" .... implicits : Schema and ArgBuilder easily accessible via. import.
For now I am not able to see "black box solution" for different implementations of Json, so this is second reason why we should have separate artifact (having explicit dependency on circe json).
If you agree, I will prepare PR with artefact: caliban-circe- json- scalar-extension.
(later we can create more extensions if we will see something reasonable)

@ghostdogpr
Copy link
Owner

Actually I already have some custom scalars such as Unit, BigInt and BigDecimal in the core library. Having a separate dependency for each of them feels a bit cumbersome to use and to maintain. Each library is cross-published for 2.12, 2.13 + ScalaJS, later dotty.

The optional dependency is not a problem because if someone needs those Schema/ArgBuilder instances for Json, it means they will already have Circe on their classpath. I find it inconsistent that we already have built-in support for Circe but only for that part, people need to add an extra dependency. Either everything should be in caliban-circe, or everything built-in. Since it was decided in #102 to go for the optional dependency, I favor that direction.

@ghostdogpr
Copy link
Owner

I made a PR #176 for this. It only requires import caliban.interop.circe.json._ to work.

@JurajBurian
Copy link
Author

Thanks a lot. Looks great.
I had plan do it, but I was inoperative for few days ...

@ghostdogpr ghostdogpr added server Issue related to caliban server core labels Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server Issue related to caliban server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants