-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
Schema transformations #1903
Schema transformations #1903
Conversation
Love these transformers! I can take a look at the code over the weekend when I have a bit more free time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still trying to wrap my head around how a few parts of this work, but looks great overall. The only thing I'm really wondering is whether it might be possible to apply the transformations prior to execution
@@ -52,7 +57,7 @@ object Executor { | |||
arguments: Map[String, InputValue], | |||
path: List[Either[String, Int]] | |||
): ReducedStep[R] = | |||
step match { | |||
transformer.transformStep.lift(step).getOrElse(step) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, we will be running the transformers on the field of each object in an ObjectStep
prior to determining whether it'll be used, is that correct? I'm kind of worried about the runtime performance this might have on large schemas.
I'm still trying to wrap my head around how all this fits together, but I don't suppose it would be possible to apply the transformations during caliban.execution.Field.apply
? Might be missing something here, but the benefit I see by doing that (assuming it's possible) is that is that we would only be transforming fields that are used as part of the query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transforming Field.apply
would only modify the client request, but we need to change the step itself. A very simple example is if the client request __typename
and you renamed that type. The name will come from the ObjectStep
.
But your point about performance is very valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could transform all the steps tree in advance and save it but the problem is in the Extend
transformer I made, I also need access to Field
when I transform the step 🤔 Need to think about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah even if I do it in advance it will run every time as soon as it's inside a QueryStep
or FunctionStep
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transforming Field.apply would only modify the client request, but we need to change the step itself. A very simple example is if the client request __typename and you renamed that type. The name will come from the ObjectStep.
Agh fair point, didn't think of that issue.
Just thinking out loud: I know you're going to work on a refactoring of how we build the graph, but in the meantime perhaps one way that we could avoid the performance overhead would be by having the transformers have some internal caching on whether they are known to transform / not transform a Step. This would require us however to have a way to assign a consistent hash to a step that doesn't need to be recalculated each time we're executing a request.
Can you think of a way that we could achieve that or would that be too difficult / not worth it given you're planning to refactor the graphql resolution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it doesn't need to be unique per se. It can be the hash code of the schema that created the Step perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still not work when it's depending on the field. Actually as you can see in the group chat I'm thinking of a completely different approach for handling the gateway feature because it is starting to feel wrong to reuse the current executor. If I do I guess transformers would move there too.
Will adopt another approach |
Closes #130
This is the first step towards a much bigger change that will introduce remote sources to Caliban and the ability to combine them into a single GraphQL API (giving Caliban the ability to act as a gateway).
Adds the ability to transform types of an existing schema:
I used
PartialFunction
rather thanString
to give the ability to handle multiple items at once and to write custom logic such as capitalization, prefixing, etc.Example usage:
TODO: