Add source location mapping #168

paulpdaniels · 2020-01-18T08:13:12Z

This PR adds source location mapping to fields and fragments. It is primarily used for outputting when errors occur.

ghostdogpr

Looks awesome! Just a couple questions.

ghostdogpr · 2020-01-18T08:41:53Z

core/src/main/scala/caliban/CalibanError.scala

@@ -29,6 +31,7 @@ object CalibanError {
  case class ExecutionError(
    msg: String,
    path: List[Either[String, Int]] = Nil,
+    locationInfo: Option[LocationInfo] = None,


How about adding it to ParsingError and ValidationError?

ghostdogpr · 2020-01-18T08:47:09Z

core/src/main/scala/caliban/schema/Schema.scala

@@ -418,9 +420,13 @@ trait DerivationSchema[R] {

 object GenericSchema {

-  def effectfulExecutionError(path: List[Either[String, Int]], e: Throwable): ExecutionError =
+  def effectfulExecutionError(


Not related to your code, but this one seems only used in Executor, how about moving it there?

ghostdogpr · 2020-01-18T08:52:36Z

core/src/main/scala/caliban/GraphQLResponse.scala

+      case ExecutionError(_, path, location, _) if path.nonEmpty =>
+        val locationJson =
+          location.fold(Json.obj())(
+            l => Json.obj("location" -> Json.obj("column" -> l.column.asJson, "line" -> l.line.asJson))


Ah, one more thing. As mentioned in #156, the spec suggests a format which is slightly different from what you did: "locations": [ { "line": 6, "column": 7 } ].

note that location is evaluated only when path is set. This may not be what you want.

Ah you’re right, though ive never actually seen multiple locations in the wild. I guess I should change it to be a List instead of an Option too

I mean path and location are presumably independent of each other therefore the guard if path.nonEmpty => looks to be dangerous as it impacts evaluation of location. One could consider location being non-empty and path being empty even though it's more of an intellectual exercise than a "reality".

phderome · 2020-01-18T19:51:41Z

core/src/main/scala/caliban/execution/Executor.scala

            case EnumValue(v) if mergeFields(currentField, v).collectFirst {
-                  case Field("__typename", _, _, _, _, _, _) => true
+                  case Field("__typename", _, _, _, _, _, _, _) => true
                }.nonEmpty =>


I personally don't like many underscores, though I understand that was the case prior to this PR...

How about this notation?
case f: Field if f.name == "__typename" => true

I agree it gets verbose. One concern I have about this change is that it removes exhaustive checks on the case match. I think it’s less of a concern where there are if constraints because I think that already disables the check as well.

phderome · 2020-01-18T19:56:12Z

core/src/main/scala/caliban/parsing/SourceMapper.scala

+  /**
+   * Implementation taken from https://github.com/lihaoyi/fastparse/blob/master/fastparse/src/fastparse/ParserInput.scala
+   */


you may need to reproduce his MIT license somewhere if you have not taken care of that.

Good point. Is that usually at the file level or do we just need to have it once at the top level?

I know of firms who do it at product level. You'd have to ask ZIO principals or Pierre I guess.

I think in the file would be fine. Found an example in ZIO here https://github.com/zio/zio/blob/master/stacktracer/jvm/src/main/scala/zio/internal/stacktracer/impl/AkkaLineNumbers.scala

phderome · 2020-01-18T19:58:05Z

core/src/main/scala/caliban/validation/Validator.scala

@@ -132,7 +133,7 @@ object Validator {
  private def collectSelectionSets(selectionSet: List[Selection]): List[Selection] =
    selectionSet ++ selectionSet.flatMap {
      case _: FragmentSpread                  => Nil
-      case Field(_, _, _, _, selectionSet)    => collectSelectionSets(selectionSet)
+      case Field(_, _, _, _, selectionSet, _) => collectSelectionSets(selectionSet)


I'd be inclined to use a more concise notation that does not use underscores much, once again here. Again, my comment relates to pre-PR code, but since we modify this line, we may as well consider it.
case f: Field => collectSelectionSets(f.selectionSet)

Here should be fine since exhaustive checking would already be disabled from the line above

phderome · 2020-01-18T19:59:36Z

core/src/main/scala/caliban/validation/Validator.scala

@@ -161,7 +162,7 @@ object Validator {
    IO.foreach(selectionSet) {
        case FragmentSpread(_, directives) =>
          checkDirectivesUniqueness(directives).as(directives.map((_, __DirectiveLocation.FRAGMENT_SPREAD)))
-        case Field(_, _, _, directives, selectionSet) =>
+        case Field(_, _, _, directives, selectionSet, _) =>


since we don't match on special values, again just case f: Field => ... access f members where needed

phderome · 2020-01-18T23:23:49Z

core/src/main/scala/caliban/execution/Executor.scala

            case EnumValue(v) if mergeFields(currentField, v).collectFirst {
-                  case Field("__typename", _, _, _, _, _, _) => true
+                  case Field("__typename", _, _, _, _, _, _, _) => true
                }.nonEmpty =>
              // special case of an hybrid union containing case objects, those should return an object instead of a string
              val obj = mergeFields(currentField, v).collectFirst {
-                case Field(name @ "__typename", _, _, alias, _, _, _) =>
+                case Field(name @ "__typename", _, _, alias, _, _, _, _) =>
                  ObjectValue(List(alias.getOrElse(name) -> StringValue(v)))
              }
              obj.fold(s)(PureStep(_))


In fact I am proposing this simplification for current lines 60-68, which pass the tests.

case s @ PureStep(value) => value match { case EnumValue(v) => // special case of an hybrid union containing case objects, those should return an object instead of a string val obj = mergeFields(currentField, v).collectFirst { case f: Field if f.name == "__typename" => ObjectValue(List(f.alias.getOrElse(f.name) -> StringValue(v))) } obj.fold(s)(PureStep(_)) case _ => s }

I don't see any need to do mergeFields...collectFirst twice here, once in match guard and once in evaluation optional obj.

Note that I am considering s to be the default value for value match in the complex case and the second match all case just like existing code.

ghostdogpr · 2020-01-28T11:43:43Z

Looks good to me! It needs a rebase to solve the merge conflict with master and I think we’re good.

ghostdogpr

Thanks, it's awesome!

paulpdaniels requested a review from ghostdogpr January 18, 2020 08:19

ghostdogpr reviewed Jan 18, 2020

View reviewed changes

phderome reviewed Jan 18, 2020

View reviewed changes

paulpdaniels force-pushed the feature/source-location-mapping branch from 81adfd8 to 88b4cba Compare January 28, 2020 10:08

paulpdaniels added 6 commits January 28, 2020 19:46

feat(sourcelocation): Add SourceMapper

83ec5b4

feat(sourcemapping): Add source mapping. Fix tests

921d52f

Fix formatting

b3709e9

Fix formatting

5af1f1d

Address PR reviews

1ae504f

Fix format and rebase

88ffb52

paulpdaniels force-pushed the feature/source-location-mapping branch from 88b4cba to 88ffb52 Compare January 28, 2020 11:46

ghostdogpr approved these changes Jan 28, 2020

View reviewed changes

ghostdogpr merged commit 9cd1643 into ghostdogpr:master Jan 28, 2020

ghostdogpr mentioned this pull request Jan 28, 2020

Errors: add locations #156

Closed

paulpdaniels deleted the feature/source-location-mapping branch January 28, 2020 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source location mapping #168

Add source location mapping #168

paulpdaniels commented Jan 18, 2020

ghostdogpr left a comment

ghostdogpr Jan 18, 2020

ghostdogpr Jan 18, 2020

ghostdogpr Jan 18, 2020

phderome Jan 19, 2020

paulpdaniels Jan 19, 2020

phderome Jan 19, 2020

phderome Jan 18, 2020

paulpdaniels Jan 19, 2020

phderome Jan 18, 2020

paulpdaniels Jan 19, 2020

phderome Jan 19, 2020

ghostdogpr Jan 19, 2020

phderome Jan 18, 2020 •

edited

Loading

paulpdaniels Jan 19, 2020

phderome Jan 18, 2020

phderome Jan 18, 2020

phderome Jan 18, 2020

ghostdogpr commented Jan 28, 2020

ghostdogpr left a comment

Add source location mapping #168

Add source location mapping #168

Conversation

paulpdaniels commented Jan 18, 2020

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

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

phderome Jan 18, 2020 • 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

ghostdogpr commented Jan 28, 2020

ghostdogpr left a comment

Choose a reason for hiding this comment

phderome Jan 18, 2020 •

edited

Loading