HTTP annotations allow mutable objects

[MaryamZi]

Description:
The spec requires the type in annotations to be a subtype of true, map<value:Cloneable>, or map<value:Cloneable>[]. But the jBallerina implementation allowed mutable object since stdlib annotations used mutable objects. This was tracked by #74 and since that seemed to have been fixed we tried updating this validation in the compiler, but it seems like there are new mutable objects now - http:Inteceptor.

Affected Versions:
2.6.0

Related Issues (optional):
#74

[shafreenAnfar]

I see. We can change it to readonly and solve the problem. However, this would limit the usage of interceptors. For instance, it would no longer be possible to cache any values such as token validated results, etc. Therefore, before we do this, I wonder if there is any possible alternative we could consider.

[TharmiganK]

Had a chat with @jclark @shafreenAnfar @MaryamZi @hasithaa and came up with the below solution for configuring interceptors rather than having it in the annotation.

import ballerina/http;

// This service type will be a part of `http` module.
public type InterceptableService distinct service object {
    *http:Service;

    function intercetors() returns http:Interceptor[];
};

service class MyRequestInterceptor {
    *http:RequestInterceptor;

    resource function 'default [string... path](http:RequestContext ctx)
            returns http:NextService|error? {
        return ctx.next();
    }
}

service class MyResponseInterceptor {
    *http:ResponseInterceptor;

    remote function interceptResponse(http:RequestContext ctx)
            returns http:NextService|error? {
        return ctx.next();
    }
}

// Making the service as a part of `InterceptorableService` will validate the 
// the availability of the `interceptors` function with the matching signature
service InterceptableService / on new http:Listener(9090) {

    function intercetors() returns [MyRequestInterceptor, MyResponseInterceptor] {
        return [new MyRequestInterceptor(), new MyResponseInterceptor()];
    }

    resource function get hello() returns string {
        return "Hello, World!";
    }
}

[TharmiganK]

@shafreenAnfar Having second thoughts on having a function because of the following concerns:

1. We are having this function only to set the interceptor pipeline and there is no usages other than initialising the interceptor pipeline using this function(OpenAPI spec generation including interceptor return types - see point 3). So IMO this should be done more like in a init function.

   public type InterceptableService distinct service object {
       *http:Service;
       http:Interceptor[] interceptors;
   };
   
   service InterceptableService / on new http:Listener(9090) {
   
       function init() {
            self.interceptors = [new MyRequestInterceptor(), new MyResponseInterceptor()];
       }
   
       resource function get hello() returns string {
           return "Hello, World!";
       }
   }

2. Even though we have this function after the initialisation of the service, there won't be no effect on interceptor pipeline. Since we have it as function, some may misunderstand that we get the interceptor pipeline by calling this function every time we get a request which is not true.

   public type InterceptableService distinct service object {
       *http:Service;
   
       function interceptors() returns http:Interceptor[];
   };
   
   http:Interceptor[] interceptors = [new MyRequestInterceptor(), new MyResponseInterceptor()];
   
   service InterceptableService / on new http:Listener(9090) {
   
       function interceptors() returns http:Interceptor[] {
           return interceptors;
       }
   
       resource function get hello() returns string {
           // No effect even the pipeline is changed here
           interceptors.push(new MyResponseErrorInterceptor());
           return "Hello, World!";
       }
   }

3. Even the function allows us to retrieve the interceptor types in the pipeline for OpenAPI generation, we still have the issue on getting the listener level interceptors. So this will be useful only if we have service level interceptors.

   public type InterceptableService distinct service object {
       *http:Service;
   
       function interceptors() returns http:Interceptor[];
   };
   
   listener http:Listener serverEP = new http:Listener(9090, 
              interceptors = new MyDefaultResponseInterceptor());
   
   service InterceptableService / on  serverEP {
   
       function interceptors() returns [MyRequestInterceptor,  MyResponseInterceptor] {
           return [new MyRequestInterceptor(), new MyResponseInterceptor()];
       }
   
       resource function get hello() returns string {
           return "Hello, World!";
       }
   }

[TharmiganK]

@shafreenAnfar @dilanSachi and myself had a discussion on the above comments and decided on the following:

1. We have decided to have it as a function so that the interceptor pipeline can be reflected in the OpenAPI spec
2. The function name was wrong and decided to have it like initInterceptors() for now
3. We have discussed about the listener level interceptors which cannot be reflected in the OpenAPI spec. Need to check a way to define the typedescs of the listener level interceptors somewhere in the listener init.

So in order to add this service level interceptor change we have to do the following tasks:

• Add InterceptableService to http module and get the service level interceptors using this initInterceptors() function.
• Give a warning/compiler error when interceptors are added from both this function and the annotation
• Deprecate the usage of having the interceptors in the annotation (might be provided by the compiler plugin)
• Update the website and other samples

@dilanSachi can you check the above tasks and create a separate issue to track the progress of this change?

[TharmiganK]

For the listener we could do something like this as defining a function type:

import ballerina/http;

service class MyRequestInterceptor {
    *http:RequestInterceptor;

	resource function 'default [string... path](http:RequestContext ctx) returns http:NextService|error? {
		return ctx.next();
	}
}

service class MyResponseInterceptor {
    *http:ResponseInterceptor;

	remote function interceptResponse(http:RequestContext ctx) returns http:NextService|error? {
		return ctx.next();
	}
}

// Ideally, this will be a part of `http` module
public type CreateInterceptorsFunction function () returns http:Interceptor[];

// The function implementation. Note the tuple return type.
function createInterceptors() returns [MyResponseInterceptor, MyRequestInterceptor] {
    return [new MyResponseInterceptor(), new MyRequestInterceptor()];
}

// This class is just to mock the `http:Listener`
public class Listener {
	http:Interceptor[] interceptors;
	int port;

	function init(int port, CreateInterceptorsFunction interceptors) {
		self.port = port;
		self.interceptors = interceptors();
	}
}

Listener serverEP = new(9090, interceptors = createInterceptors);

[dilanSachi]

We have currently deprecated the usage of http:Interceptor in the http:ServiceConfig and also http:ListenerConfiguration. We can close this issue once we remove those completely.

[TharmiganK]

This issue has been fixed now