Expose current executor's location

[rushmorem]

Executor has a push_error method which is very useful for reporting
back errors. However, this method needs to know the location of the
error (SourcePosition). This commit adds a location method to the
executor.

[mhallin]

This looks good, but I would like to know your use case for needing to call push_error manually. Is returning errors from fields not enough, or is it related to #84?

[rushmorem]

It's not related to #84. It's loosely related to #40, which #80 seeks to resolve. To me, these are all related in that they are trying to improve the ergonomics when processing multiple inputs.

For my specific use case, I'm building a domain registration API. A user might want to check availability of multiple domain names at the same time. For example:-

{
  domains {
    check(domains: ["example.com", "example.local"]) {
      domain
      available
    }
  }
}

Like in the example above, some of the domains submitted may not be valid (example.local in this case). I want my servers to be fault tolerant. That is, instead of simply returning an error message saying example.local is not a valid domain name, I want it to return the correct result for example.com and an error for example.local like so:-

{
  "data": {
    "domains": {
      "check": [
        {
          "available": false,
          "domain": "example.com"
        }
      ]
    }
  },
  "errors": [
    {
      "message": "example.local is not a valid domain name",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "path": [
        "domains",
        "check"
      ]
    }
  ]
}

I couldn't figure out any other way of achieving this besides using push_error.

EDIT:-

Is returning errors from fields not enough

No, it's not enough. The problem with FieldResult in this case is that it's all or nothing. Either you get all results back or you have a single error and no data. That is, you lose "fault tolerance".

[srijs]

It could be that I'm missing something here, but wouldn't it be more idiomatic if you used the built-in mechanisms that GraphQL provides to do batch queries (perhaps combined with fragments to keep it DRY)?

fragment checkField on Check {
  domain,
  available
}

query {
  domains {
    check(domain: 'example.com') {
      ...checkFields
    }
    check(domain: 'example.local') {
      ...checkFields
    }
  }
}

This would get you a FieldResult per domain check that you can expose the error through.

[rushmorem]

TLDR: Using a List as input for a use case like this one is more user friendly and more efficient.

---

I may be wrong about this but IMHO batching like that is ideal for queries that actually do different things instead of doing the same thing on different inputs.

There are a number of drawbacks for batching a query like this versus simply accepting a List like I'm doing:-

It complicates client queries unnecessarily. Compare this:-

{
  domains {
    check(domains: ["example.com", "example.local"]) {
      domain
      available
    }
  }
}

to this:-

fragment checkField on Check {
  domain,
  available
}

query {
  domains {
    alias1: check(domain: "example.com") {
      ...checkFields
    }
    alias2: check(domain: "example.local") {
      ...checkFields
    }
  }
}

• The user will have to write code for generating unique aliases for each domain.
• The user will also have to handle those aliases in the returned response.
• The first query is easier to understand at a glance.

Hence, I think batching in this sense is overkill for such a simple use case.

There are also practical reasons why I wouldn't use it for a use case like this:-

• In the first query, since I'm simply accepting a Vec<String>, I can easily strip duplicates from the query before processing. This is impossible using the second approach. I will have to trust the user to send unique domains, otherwise I will waste system resources processing duplicate domains for the same request.
• Because I'm using the first approach, I process the domains concurrently. I wouldn't be able to do this otherwise. This wouldn't be much of a big deal if juniper processed fields asynchronously.

[mhallin]

I see, this seems reasonable :)

May I suggest a small API change though: since you probably always will write executor.push_error(..., executor.location()), we could change push_error to always use the current location, and then have another method like push_error_at_location that takes an explicit location?

[rushmorem]

@mhallin That's a good idea. Let me do that.

[rushmorem]

@mhallin Instead of push_error_at_location how about just push_error_at much like how std library methods (split_at et al) are named?

[rushmorem]

@mhallin Done. I used push_error_at. Let me know if you still prefer push_error_at_location and want me to change to that.

[mhallin]

This looks great, thanks a lot!