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

HTTP compiler plugin rewrite #4152

Closed
10 tasks done
TharmiganK opened this issue Mar 3, 2023 · 6 comments
Closed
10 tasks done

HTTP compiler plugin rewrite #4152

TharmiganK opened this issue Mar 3, 2023 · 6 comments
Assignees
Labels
Area/CompilerPlugin module/http Team/PCM Protocol connector packages related issues Type/Task
Milestone

Comments

@TharmiganK
Copy link
Contributor

TharmiganK commented Mar 3, 2023

Description:

Currently the HTTP compiler plugin validations are depends on the TypeDescriptor of the TypeSymbol. This logic creates some unexpected bugs due to some missing cases.

Some of the bugs are :

There is a compiler plugin types API which can be used to check whether a TypeSymbol is a subtype of given TypeSymbol. This can be used to simplify our validation logics. For example, the payload validation can be simplified by just checking whether the payload type is a subtype of anydata type symbol. This anydata type symbol can be retrieved from the AnalysisContext.

Describe your task(s)

  • Refactor the following validation logics :
    • Return type validation
    • Payload validation
    • Header parameter validation
    • Query parameter validation
  • Check for any compiler breaking changes after refactoring
  • Check runtime support for any newly allowed types
  • Add ballerina test cases for the newly allowed types
  • Check the impact of the rewrite wrt the compilation time
  • Update any doc changes
@TharmiganK
Copy link
Contributor Author

Change 1: Ballerina tuple types will be allowed as a payload and/or a return type iff the tuple members are subtype of any data. (This is merely a compiler bug fix, runtime already supports these types)

type Address [string, string, string];

type User record {|
    string name;
    Address address;
|};

type UserWithId [int, User];

class Organization {};

type UserNew record {|
    *User;
    Organization org;
|};

service on new http:Listener(9090) {

    resource function get address() returns [string, string, string] {
        return ["221B", "Baker Street", "London"];
    }

    resource function post address(@http:Payload Address address) {}

    resource function get User() returns User {
        return {name: "Sherlock Holmes", address: ["221B", "Baker Street", "London"]};
    }

    resource function post User(@http:Payload User user) {}

    resource function get UserWithId() returns UserWithId {
        return [1, {name: "Sherlock Holmes", address: ["221B", "Baker Street", "London"]}];
    }

    // Error : HTTP_107 - invalid payload parameter type
    resource function post UserNew(@http:Payload UserNew user) {}
}

@TharmiganK
Copy link
Contributor Author

Change 2: Error code HTTP_145-incompatible record field type will be removed from payload validation, since the new validation is done to the entire type rather than the individual members. This new validation just checks whether the TypeSymbol of the payload parameter is a subtype of anydata or not. (Just like checking if type is anydata in ballerina)

class Family {}

type Person record {
    string name;
    int age;
    Family family?;
};

service on new http:Listener(9090) {
    // Previously: incompatible record field type: Family in payload parameter: person(HTTP_145)
    // Now: invalid payload parameter type: Person(HTTP_107)
    resource function post person(@http:Payload Person person) {}
}

@TharmiganK
Copy link
Contributor Author

Change 3: Nested records are not allowed incase of header parameters. (This is a bug fix. Seems like this bug is somehow introduced while fixing some other bugs)

type RateLimitHeaders record {|
    string x\-rate\-limit\-id;
    int? x\-rate\-limit\-remaining;
    string[]? x\-rate\-limit\-types;
|};

type NestedRecord record {|
    RateLimitHeaders xRate;
|};

service on new http:Listener(9090) {

    // Previously: not a compiler error, but runtime will fail with header not found for `xRate`
    // Now: invalid type of header param: xRate(HTTP_109)
    resource function get hello(@http:Header NestedRecord headers) {}
}

@TharmiganK
Copy link
Contributor Author

Change 4: The error for nilable unsupported singleton types will be reported as invalid type of header param and other union types will be reported as invalid union type of header param. This mean some of the error codes will be changed. (This is more of a consistency fix with query param validation)

type RateLimitHeaders record {|
    string x\-rate\-limit\-id;
|};

service on new http:Listener(9090) {

    // Prevoiusly: HTTP_110 - invalid union type of header param headers
    // Now: HTTP_109 - invalid type of header param headers
    resource function get greeting(@http:Header RateLimitHeaders[]? headers, xml? query) returns string {
        return "Hello, World!";
    }

    // HTTP_109 - invalid type of header param headers
    resource function get hello(@http:Header RateLimitHeaders|string headers, string|int query) returns string {
        return "Hello!";
    }
}

@TharmiganK
Copy link
Contributor Author

TharmiganK commented Mar 3, 2023

Change 5:

  1. Other than the basic types, only a map of json type can be allowed as query parameter. This means records are allowed if it is a subtype of map<json> otherwise an error is returned.
// This closed record will be always a subtype of `map<json>`
type User record {|
    string name;
    int age;
|};

type UserNew record {|
    *User;
    xml data;
|};

service on new http:Listener(9090) {

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

    // Error: invalid type of query param: user(HTTP_112)
    resource function get hello(UserNew user) returns string {
        return "Hello!";
    }
}
  1. When it comes to map query parameter type, we allow all the subtypes of json. This means it can be record types which are closed and has only json members, union types with other json types.
// This closed record will be always a subtype of `map<json>`
type User record {|
    string name;
    int age;
|};

type UserOpen record {
    *User;
};

service on new http:Listener(9090) {

    resource function get greeting(map<User> user) returns string {
        return "Hello, World!";
    }

    resource function get bye(map<string|int?> user) returns string {
        return "Bye, World!";
    }

    // Error - invalid query param type: user(HTTP_112)
    resource function get greetingUser(map<UserOpen> user) returns string {
        return "Hello, User!";
    }
}

@TharmiganK
Copy link
Contributor Author

Change 5:
....
2. When it comes to map query parameter type, we allow all the subtypes of json. This means it can be record types which are closed and has only json members, union types with other json types.
....

TODO: Need to check the runtime support for this!

Checked the runtime for this support, currently this is not supported by runtime since we only cast the value to MAP_TYPE. Casting the type with the paramType should fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/CompilerPlugin module/http Team/PCM Protocol connector packages related issues Type/Task
Projects
Archived in project
Development

No branches or pull requests

1 participant