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

Make @http:Payload annotation optional #3276

Closed
shafreenAnfar opened this issue Aug 23, 2022 · 13 comments · Fixed by ballerina-platform/module-ballerina-http#1428
Closed

Make @http:Payload annotation optional #3276

shafreenAnfar opened this issue Aug 23, 2022 · 13 comments · Fixed by ballerina-platform/module-ballerina-http#1428
Assignees
Labels
module/http Points/5 Team/PCM Protocol connector packages related issues Type/Improvement
Milestone

Comments

@shafreenAnfar
Copy link
Contributor

shafreenAnfar commented Aug 23, 2022

At the moment when writing a post resource function, it is required to do the below.

resource function post student(@http:Payload Student student) returns json {
    string name = student.Name;
    return {Name: name};
}

This pattern is common (80% case) for the accessors post, put and patch as they all require an entity body. Therefore, what if we say the first parameter of the resource definitions for these accessors is defaulted to @http:Payload. This means above can be written as follows,

resource function post student(Student student) returns json {
    string name = student.Name;
    return {Name: name};
}

If the user wants both entity body and query params, it would look like the below,

resource function post student(Student student, string version) returns json {
    string name = student.Name;
    return {Name: name};
}

Then for the uncommon case (20% case) where the user is not interested in the entity body but the query params, @http:QueryParam annotation can be introduced. For example,

resource function post student(@http:QueryParam string version) returns json {
    string name = student.Name;
    return {Name: name};
}

Any thoughts?

@chamil321
Copy link
Contributor

The existing resource signature behaviour is quite intuitive and understandable via few rules.

  • Define query params without annotation.
  • Use annotation for Payload and Header params.

So this will be another information for the user to keep in mind also it brings back some ordering again to the signature which was drop during the redesign.

Few followup questions

  1. How is the behaviour when the accessor is default with payload param?

Should the payload param be the first param with @http:Payload annotation or can it be anywhere with the annotation?

  1. What if user want multiple query params to define in the 20% case?

Use @http:QueryParam annotation for every query param or just for the first one?

@shafreenAnfar
Copy link
Contributor Author

The existing resource signature behaviour is quite intuitive and understandable via few rules.

Yes, current rules are simple and straightforward but I am not sure how intuitive they are in this particular context though. Because, for example, in the case of post the first thing that comes to your mind is reading the payload. Therefore, it is natural to make the first parameter represent the payload. So it is sort of counter-intuitive to ask users to annotate it with @http:Payload, IMO.

How is the behaviour when the accessor is default with payload param?

As mentioned in the issue this only applies to post, put, and patch as they all have a payload.

What if user want multiple query params to define in the 20% case?

Then the user has to annotate for each query param.

@shafreenAnfar shafreenAnfar added Team/PCM Protocol connector packages related issues Area/http2 module/http and removed Area/http2 labels Aug 23, 2022
@sanjiva
Copy link

sanjiva commented Aug 25, 2022

I love this feature if we can make the rules simple for the 80% case. What about more constraints:

  • Payload param must be last (does that conflict with a rest param to capture arbitrary query params?)
  • Payload param must be structured basic type not a simple basic type

So, these will work correctly then:

resource function get foo(string name, Person p) {
}

resource function post bar(Person p) {
}

resource function post baz(int a, string bar, Person p) {
}

@sanjiva
Copy link

sanjiva commented Aug 25, 2022

I noticed your comment that will only apply for post/put/patch. But do we need to put more restrictions than HTTP? If the protocol allows an entity body then we allow it. IIRC that's then all except HEAD.

The only place this will be annoying is if I want to bring text/* payload content into a string:

resource function post foo(string qP1, int qP2, @http:Payload string payload) {
}

@shafreenAnfar
Copy link
Contributor Author

shafreenAnfar commented Aug 31, 2022

After discussing @jclark, @sanjiva, and others, it was decided not to make @http:Payload annotation default based on the order of parameters but the type. Therefore, the following are the new constraints to make @http:Payload default.

  • Parameters must include only one structured type of map/record
  • If there is more than one structured type, the ambiguity must be resolved using @http:Payload
  • If there are no structured types all are considered as query params
  • Above rules are only applicable to POST, PUT, PATCH, DELETE and DEFAULT accessors

The above rules are introduced thinking about the 80% case. Therefore, in the case of GET requests, those rules are excluded as it is not the 80% case to send a payload with the request.

HEAD and OPTIONS are special types of requests designed to get meta information about a resource. The dispatcher handles them in a special way. Therefore, those are also excluded from the above rules as well.

@shafreenAnfar
Copy link
Contributor Author

As in the original issue description, we still need the http:QueryParam annotation. Because we support structured type of map/record when it comes to query params. Consider the below scenario,

resource function post student(json student) returns json {
    string name = student.Name;
    return {Name: name};
}

In this case, student parameter is considered as the payload as per the above rules. However, for some strange reason if the user meant here is a query param, then in order to do that, the user must need a way to override the default behaviour. One way of doing that is using http:QueryParam annotation as follows.

resource function post student(@http:QueryParam json student) returns json {
    string name = student.Name;
    return {Name: name};
}

Again, this is a very rare scenario, IMO. But for the sake of completeness we need it.

@chamil321
Copy link
Contributor

HEAD and OPTIONS are special types of requests designed to get meta information about a resource. The dispatcher handles them in a special way. Therefore, those are also excluded from the above rules as well.

In addition to that, if user has defined the accessor as HEAD or OPTIONS in resource functions, the request will be catered similar to GET request. Specially the resource which has the HEAD accessor will not send the response payload. Hence we can consider GET, HEAD and OPTIONS as non-entity body accessors

@chamil321
Copy link
Contributor

chamil321 commented Aug 31, 2022

@lnash94 @xlight05 Please note the change of rules for defining Payload parameter.

@jclark
Copy link

jclark commented Aug 31, 2022

I would also suggest that an included record param (*T) is always treated as specifying query parameters.

@sanjiva
Copy link

sanjiva commented Aug 31, 2022

@shafreenAnfar we should not restrict it to just map typed parameters. Even lists are fine as its very normal to get an array of JSON objects.

So how about:

  • If there's one parameter of a record type or a list type then that's the payload

@sanjiva
Copy link

sanjiva commented Aug 31, 2022

For example

resource function post(Student[] p) {
}

@shafreenAnfar
Copy link
Contributor Author

shafreenAnfar commented Sep 2, 2022

I and @chamil321 had a quick chat on this. Yeah, above suggestions are valid suggestions. We will update the rules with those suggestions.

When it comes to the list type it is common to have list of primitives (i.e. int[]) as query params than payload. Therefore wouldn't it be better to have a more constrained rule as follows.

  • If there's one parameter of a record type or a list of record type (i.e. Student[]) then that's the payload
  • byte[] is an exception and also follows the above rules

@chamil321
Copy link
Contributor

chamil321 commented Jan 31, 2023

resource function post student(@http:Query map<json> student) returns json {
    string name = student.Name;
    return {Name: name};
}

I and @shafreenAnfar had a discussion and decided to go ahead with @http:Query annotation to be consistent with @http:Payload and @http:Header annotations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/http Points/5 Team/PCM Protocol connector packages related issues Type/Improvement
Projects
Archived in project
Status: Standard Lib - PCM
Development

Successfully merging a pull request may close this issue.

5 participants