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

GraphQL Interceptor Execution Logic Improvement #4199

Closed
ThisaruGuruge opened this issue Mar 14, 2023 · 5 comments
Closed

GraphQL Interceptor Execution Logic Improvement #4199

ThisaruGuruge opened this issue Mar 14, 2023 · 5 comments
Assignees
Labels
module/graphql Issues related to Ballerina GraphQL module Team/PCM Protocol connector packages related issues Type/Improvement

Comments

@ThisaruGuruge
Copy link
Member

Description:

Problem

Currently, the Ballerina GraphQL package has Service-Level Interceptor support.

In the current implementation, when a service-level interceptor is added to a service, it executes per each field of the GraphQL schema. Consider the following as an example:

import ballerina/graphql;
import ballerina/log;

readonly service class LogInterceptor {
    *graphql:Interceptor;
    isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
        log:printInfo(string `Field "${fieldName}" execution started!`);
        var data = context.resolve('field);
        log:printInfo(string `Field "${fieldName}" execution completed!`);
        return data;
    }
}

@graphql:ServiceConfig {
    interceptors: [new LogInterceptor()]
}
service on new graphql:Listener(9090) {
    resource function get profile() returns Profile {
        return new ("John Doe", 30);
    }
}

service class Profile {
    private final string name;
    private final int age;

    function init(string name, int age) {
        self.name = name;
        self.age = age;
    }

    resource function get name() returns string {
        return self.name;
    }

    resource function get age() returns int {
        return self.age;
    }
}

In the above example, the interceptor is applied to all the fields, such as Query.profile, Profile.name, and Profile.age. Consider the following GraphQL document:

query {
    profile {
        name
        age
    }
}

For the above query, the server will log something like this (ignore the log format, just the functionality):

Field "profile" execution started!
Field "name" execution started!
Field "name" execution completed!
Field "age" execution started!
Field "age" execution completed!
Field "profile" execution completed!

This was an okay approach back then since there was no way to intercept a field execution otherwise. But one drawback of this approach was that if a developer needed to intercept a field, they have to filter the field name inside the interceptor logic using string manipulation and execute the interceptor. This is not an optimal solution. We had seen this before and that's why we planned for the resource-level (or in GraphQL terms, field-level) interceptors.

Since now we are working on implementing the resource-level interceptors, applying the service-level interceptors to all the fields will become obsolete and it also does not mean anything anymore.

So the plan is to apply the service-level interceptors to the service level, which means any of the entry points of a GraphQL schema. This can be the Query type, Mutation type, or Subscription type. The interceptor will be applied to the root operation only. If an interceptor is needed to be applied for specific fields, it can be done using the resource-level interceptors.

This change can be a breaking change for projects where interceptors are used to intercept fields.

Solution

Going forward, we can do the following:

  1. Introduce the resource-level interceptors in a future release. (Probably SwanLake Update 6)
    With this release, we can:
  • Introduce a compiler warning when an interceptor is used, and guide them to use resource-level interceptors where possible.
  • Migrate the documentation (examples, BBEs, guides, etc.) to showcase the difference between the two interceptors.
  1. Removing the execution of service-level interceptors for each field in the next release (SwanLake Update 7) or maybe the one after the next release (SwanLake Update 8).

@shafreenAnfar @DimuthuMadushan @MohamedSabthar WDYT?

@ThisaruGuruge ThisaruGuruge added Type/Improvement module/graphql Issues related to Ballerina GraphQL module Team/PCM Protocol connector packages related issues labels Mar 14, 2023
@shafreenAnfar
Copy link
Contributor

Interesting, I see it a bit differently. I wonder if we really need to change the behaviour on the basis of having resource level interceptor. In my head, it was just an extension to interceptor array.

For instance, if you want to log the execution time for reach resource function we can easily do it with current architecture, and if we have any other common thing, we can still do it. But with the proposed model we have to add it each and every resource if I am not mistaken.

@ThisaruGuruge
Copy link
Member Author

ThisaruGuruge commented Mar 14, 2023

Conclusion

Had an offline discussion with @shafreenAnfar and @DimuthuMadushan

The following are points discussed:

  • Applying the same interceptor logic to each and every field can be useful on some occasions, such as when a developer needs to add a log message in each field execution (for debugging purposes, etc.).
  • Applying the interceptor logic once is also useful on some occasions, such as Authorization handling. (In authorizing, the request needs to be authorized at the entry point and then it is no longer needed; authentication is different).

Semantically, there are two different types of interceptors.

  • Type Interceptors
  • Field Interceptors

Type Interceptors (Service-Level Interceptors)

The current interceptor is somewhat similar to a type interceptor. This means the interceptor can be applied to a type. Although the current implementation only supports adding an interceptor to the root __Schema type, this can be extended to support other types as well. Consider the following scenario:

import ballerina/graphql;
import ballerina/log;

readonly service class LogInterceptor {
    *graphql:Interceptor;
    isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
        log:printInfo(string `Field "${fieldName}" execution started!`);
        var data = context.resolve('field);
        log:printInfo(string `Field "${fieldName}" execution completed!`);
        return data;
    }
}

service on new graphql:Listener(9090) {
    resource function get profile() returns Profile {
        return new ("John Doe", 30);
    }
}

@graphql:ServiceConfig {
    interceptors: [new LogInterceptor()]
}
service class Profile {
    private final string name;
    private final int age;

    function init(string name, int age) {
        self.name = name;
        self.age = age;
    }

    resource function get name() returns string {
        return self.name;
    }

    resource function get age() returns int {
        return self.age;
    }
}

In the above code, the interceptor is applied to the Profile type instead of the root __Schema type. (This is not currently supported). In this case, the interceptor logic should be applied to all the fields in this type, and their subfields. (Similar to how the interceptor is applied to all the fields in the current state where we add the interceptor to the root __Schema type. In any case, the interceptors should be applied to each and every field and subfield of the type.

Field Interceptors (Resource-Level Interceptors)

Field interceptors are applied to a certain field of an object in the GraphQL schema (with the limitation of the field must be represented using a resource/remote method). Then the interceptor logic should only be applied to that particular field. It should not be applied to the field type or the fields/subfields of the field type. This can be introduced with the current interceptor proposal.

Consider the following code:

import ballerina/graphql;
import ballerina/log;

readonly service class LogInterceptor {
    *graphql:Interceptor;
    isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
        log:printInfo(string `Field "${fieldName}" execution started!`);
        var data = context.resolve('field);
        log:printInfo(string `Field "${fieldName}" execution completed!`);
        return data;
    }
}

service on new graphql:Listener(9090) {

    @graphql:ResourceConfig {
        interceptors: [new LogInterceptor()]
    }
    resource function get greeting() returns string {
        return "Hello, World!";
    }

    resource function get profile() returns Profile {
        return new ("John Doe", 30);
    }
}

service class Profile {
    private final string name;
    private final int age;

    function init(string name, int age) {
        self.name = name;
        self.age = age;
    }

    @graphql:ResourceConfig {
        interceptors: [new LogInterceptor()]
    }
    resource function get name() returns string {
        return self.name;
    }

    resource function get age() returns int {
        return self.age;
    }
}

In the above example, the interceptor is applied to the Query.greeting field and the Profile.name fields.

This answers the first part of the question where the type interceptors (service-level interceptors) are applied globally (meaning the interceptor is applied to all the fields and the subfields of the type), while the field interceptors (resource-level interceptors) are applied locally (meaning the interceptor is applied only for the given field). But if a developer needs to apply an interceptor to the entry point, there is no way of doing this using the above-mentioned methods.

Defining Entry Point Interceptors

There are 2 approaches to doing this:

Using Different Types of Interceptors

In this approach, we can introduce three types of interceptors:

Service Interceptors

This works as same as the current interceptor implementation. Can be applied to the service and it will be applied to all the fields.

Resource Interceptors

This is as same as the one proposed in the resource-level interceptors. Can be applied to one or more fields in the GraphQL schema.

Listener Interceptor

This can be applied at the listener level, meaning the interceptor will be applied to each request at the interceptor level. This is similar to the HTTP listener interceptors. Implementation of this might be tricky given that this should handle all the requests coming through the listener, and also adhere to the onion principle followed by the GraphQL interceptors.

We might be able to avoid the breaking change by introducing different names for resource-level interceptors and listener-level interceptors while keeping the current graphql:Interceptor for service-level interceptors. But that naming would be somewhat counter-intuitive.

Using an Annotation for the Interceptor

In this approach, the interceptor will also have an annotation or a flag to state whether the interceptor is applied globally or locally.

By default, an interceptor applied to a field will be local. But when applied to a type, it can be global (meaning the interceptor will be applied to each and every field and subfield), or local (meaning the interceptor will be applied to that particular type). Following is an example:

@graphql:InterceptorConfig {
    global: true // This can be changed based on the requirement. 
}
readonly service class LogInterceptor {
    *graphql:Interceptor;
    isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
        log:printInfo(string `Field "${fieldName}" execution started!`);
        var data = context.resolve('field);
        log:printInfo(string `Field "${fieldName}" execution completed!`);
        return data;
    }
}

With this approach, it is intuitive to think that the resource-level interceptors too would be applied globally (meaning the interceptor should be applied to the field, and the type of the field - similar to a type interceptor). In that case, we might have to implement that as well.

This annotation may be replaced with a flag when creating the interceptor. We can avoid breaking the functionality of this by making the global: true to be the default value.

Going forward, I think we should finalize on an approach and implement that before releasing the resource-level interceptors because partial implementation would result in future breaking changes.

@shafreenAnfar @DimuthuMadushan @MohamedSabthar Please add your thoughts and anything I missed.

@DimuthuMadushan
Copy link

DimuthuMadushan commented Mar 20, 2023

Meeting notes:
Participants: Shafreen, Thisaru, Dimuthu

The following decisions were taken:

  1. Followings are the interceptors support in the GraphQL module:

    • Listener Interceptor
    • Service Interceptor
    • Field Interceptor
  2. Any of the above interceptors can be defined the same as the current interceptors defining strategy(using read-only service object) by inferring the Interceptor type already defined in the GrahQL module. Hence no breaking changes.

    type Interceptor distinct service object {
        isolated remote function execute() returns anydata|error;
    }
  3. Introduce an annotation for Interceptors as follows:

    @graphql:InterceptorConfig {
        global: true // This can be changed based on the requirement. 
    }

    This can be applied only to the service interceptors. The functionality of the global flag is as same as the annotation described in the Using an Annotation for the Interceptor section of the above comment. GraphQL Interceptor Execution Logic Improvement #4199 (comment).

  4. Listener interceptors can be applied at the listener level, meaning the interceptor will be applied to each request at the interceptor level. This is similar to the HTTP listener interceptors which also adhere to the onion principle.

    readonly service class ListenerInterceptor1 {
        *graphql:Interceptor;
        isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
            log:printInfo(string `Listener interceptor`);
            var data = context.resolve('field);
            return data;
        }
    }
    
    listener graphqlListener = new graphql:Listener(9090, {
       interceptors: [new ListenerInterceptor1()]
    });
  5. Service interceptors can be used as same as described in the spec. Additionally, the execution can be changed using the annotation newly introduced.

    @graphql:InterceptorConfig {
        global: true 
    }
    readonly service class ServiceInterceptor1 {
        *graphql:Interceptor;
        isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
            log:printInfo(string `Field "${fieldName}" execution started!`);
            var data = context.resolve('field);
            log:printInfo(string `Field "${fieldName}" execution completed!`);
            return data;
       }
    }
    
    @graphql:ServiceConfig {
        interceptors: [new ServiceInterceptor1()]
    }
    service on graphqlListener {
        ...
    }
  6. Field interceptors are applied to a certain field of an object in the GraphQL schema (with the limitation of the field must be represented using a resource/remote method). This can be introduced with Proposal: GraphQL Field Interceptors #3528

    @graphql:InterceptorConfig {
        global: true 
    }
    readonly service class FieldInterceptor1 {
        *graphql:Interceptor;
        isolated remote function execute(graphql:Context context, graphql:Field 'field) returns anydata|error {
            log:printInfo(string `Field "${fieldName}" execution started!`);
            var data = context.resolve('field);
            log:printInfo(string `Field "${fieldName}" execution completed!`);
            return data;
        }
    }
    
    @graphql:ResourceConfig {
        interceptors: [new FieldInterceptor1()]
    }
    resource function get greeting() returns string {
        ...
    }

@DimuthuMadushan
Copy link

@shafreenAnfar @ThisaruGuruge There is an issue with this approach. We cannot provide the Field object for listener interceptors since it should execute before the validation phase of the fields. We may need to introduce a new interceptor type for the Listener interceptors.

@ThisaruGuruge
Copy link
Member Author

I think we can park the listener interceptor for now. Our initial problem was to find a solution where a developer needs to apply an interceptor to an entry point of the GraphQL schema. This can be achieved using the service-level interceptors with the graphql:InterceptorConfig annotation.

Therefore, I think the listener-level interceptor does not add more value, at least for now. The only usage I can think of is to handle each and every request that comes to the listener, but that's more of an HTTP thing than a GraphQL thing, and we shouldn't couple those functionalities with GraphQL. If someone needs to handle such cases, they can either use an HTTP listener to listen to all the requests and handle them, or they can use an HTTP gateway before the GraphQL service.

We can think about introducing them later if a requirement arises. But in most cases, we can solve this with service and resource interceptors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/graphql Issues related to Ballerina GraphQL module Team/PCM Protocol connector packages related issues Type/Improvement
Projects
Archived in project
Development

No branches or pull requests

3 participants