bugfix: can't add multiple query-params

[mekpavit]

The generated client has never sent more than 1 query-parameter to LINE api. This is because the generated client keeps overriding the query variable for every query-parameters an endpoint has.

I raised this MR to fix this issue by using url.Values.Add(..., ...) method instead.

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (247a707) 55.19% compared to head (841142c) 55.15%.
Report is 11 commits behind head on master.

❗ Current head 841142c differs from pull request most recent head 1ff2861. Consider uploading reports for the commit 1ff2861 to get more accurate results

Files	Patch %	Lines
linebot/webhook/model_follow_event.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   55.19%   55.15%   -0.05%     
==========================================
  Files          87       87              
  Lines        5479     5483       +4     
==========================================
  Hits         3024     3024              
- Misses       2233     2237       +4     
  Partials      222      222              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Yang-33]

Thank you, @mekpavit !

[Yang-33]

(Not related to this change) Hmm... start is one of optional parameters, but it requires string value like "". Perhaps calling GetFollowers would fail in current implementation when we pass empty string... This seems bug in line-bot-sdk-go...

[mekpavit]

@Yang-33 Agree with you. I can help check what will happen if we put empty string as a query-param value. Maybe net/http might just ignore and omit it.

But if it doesn't, might need to change the generator to properly handle optional query-params (maybe if-else to check if it's optional before doing url.Values.Add(...).

[mekpavit]

@Yang-33 Just tested with the following, it seems net/http doesn't omit the query-parameter when it's empty string...

func TestGetFollowers_ItShouldNotPassStartWhenItIsEmptyString(t *testing.T) {
	server := httptest.NewServer(
		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			if r.URL.Query().Has("start") {
				w.Header().Set("TEST-ERROR", "incorrect start being sent from client. does not expect to have `start` in query-parameter")
				w.WriteHeader(http.StatusInternalServerError)
				return
			}
			json.NewEncoder(w).Encode(messaging_api.GetFollowersResponse{UserIds: []string{}, Next: "abcdef"})
		}),
	)
	client, err := messaging_api.NewMessagingApiAPI(
		"channelToken",
		messaging_api.WithEndpoint(server.URL),
	)
	if err != nil {
		t.Fatalf("Failed to create client: %v", err)
	}
	resp, _, _ := client.GetFollowersWithHttpInfo("", 1000)
	if resp.StatusCode != http.StatusOK {
		t.Errorf("Not getting 200 response back: %s", resp.Header.Get("TEST-ERROR"))
	}
}

[mekpavit]

@Yang-33 Please don't merge this PR yet then. I will figure it out how to deal with optional query-parameter correctly when I have time.

[Yang-33]

Thank you for checking this 🙏 I see, let's keep this PR.

[mekpavit]

@Yang-33 What's your opinion on using a pointer for optional parameter? Like GetFollowers(start *string, limit *int32)?

[Yang-33]

There are at least two options.
(1) Use a pointer. However, since line-bot-sdk-go generates code automatically, all functions will use a pointer. (Or, only optional parameters use a pointer.)
(2) As before, if it is "", do not specify this to request.

Personally, I like (1), but I don't know whether (1) is preferable as Go language.
I'd like to hear @kkdai 's opinion.

[kkdai]

Hi @Yang-33 and @mekpavit
I agree with @Yang-33 -san, the option (1) is much better.
Utilizing optional parameters can lead to confusion, particularly when there are only two and both could potentially be nil.

Consider the following scenarios:

process(&value1, nil) // Only value1 is provided
process(nil, &value2) // Only value2 is provided
process(nil, nil)     // Neither is provided
process(&value1, &value2) // Both are provided

In this context, option (1) provides clearer and more explicit handling of the parameters.

[mekpavit]

@Yang-33 @kkdai I will try to make only optional parameters to be pointer then. This might requires some substantial change to the api.pebble. Many places are going to require a param.required check and have an if param.Name != nil { ... } else { ... }. But I believe that is a better approach as you guys mentioned.

[kkdai]

LGTM