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

feat(be): Online debug function adds support for transfer files #1465

Merged
merged 10 commits into from
Feb 25, 2021

Conversation

Jaycean
Copy link
Member

@Jaycean Jaycean commented Feb 9, 2021

Please answer these questions before submitting a pull request


New feature or improvement

  • Online debug function adds support for transfer files

@Jaycean Jaycean marked this pull request as draft February 9, 2021 12:48
@codecov-io
Copy link

codecov-io commented Feb 9, 2021

Codecov Report

Merging #1465 (6f324b9) into master (bd5ed03) will decrease coverage by 38.70%.
The diff coverage is 74.07%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1465       +/-   ##
===========================================
- Coverage   68.95%   30.25%   -38.71%     
===========================================
  Files          48       48               
  Lines        3038     3051       +13     
===========================================
- Hits         2095      923     -1172     
- Misses        705     1925     +1220     
+ Partials      238      203       -35     
Impacted Files Coverage Δ
...l/handler/route_online_debug/route_online_debug.go 74.50% <74.07%> (+0.70%) ⬆️
api/internal/core/store/query.go 0.00% <0.00%> (-88.10%) ⬇️
api/internal/core/store/selector.go 0.00% <0.00%> (-75.93%) ⬇️
api/internal/handler/label/label.go 8.91% <0.00%> (-72.28%) ⬇️
api/internal/handler/service/service.go 21.27% <0.00%> (-70.22%) ⬇️
api/internal/handler/plugin/plugin.go 16.66% <0.00%> (-70.00%) ⬇️
api/internal/handler/upstream/upstream.go 20.56% <0.00%> (-68.23%) ⬇️
api/internal/handler/data_loader/route_export.go 3.38% <0.00%> (-65.79%) ⬇️
api/internal/utils/runtime/runtime.go 0.00% <0.00%> (-64.29%) ⬇️
api/internal/handler/server_info/server_info.go 33.33% <0.00%> (-57.15%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd5ed03...6f324b9. Read the comment docs.

@juzhiyuan
Copy link
Member

Hi, please sync your codes with the master branch to fix the CI failure.

@Jaycean
Copy link
Member Author

Jaycean commented Feb 18, 2021

Hi, please sync your codes with the master branch to fix the CI failure.

Thank you. I haven't added the BE E2E test for the corresponding code. I will add it later.

@nic-chen
Copy link
Member

@Jaycean
Copy link
Member Author

Jaycean commented Feb 19, 2021

hi @Jaycean

we could add e2e test cases in https://github.com/apache/apisix-dashboard/tree/master/api/test/e2enew

OK, Thks, all E2E tests are added to the new folder? Or need to add both?

@nic-chen
Copy link
Member

hi @Jaycean
we could add e2e test cases in https://github.com/apache/apisix-dashboard/tree/master/api/test/e2enew

OK, Thks, all E2E tests are added to the new folder? Or need to add both?

added to the new folder is ok, thanks.

@Jaycean Jaycean marked this pull request as ready for review February 22, 2021 05:53
@juzhiyuan juzhiyuan requested a review from starsz February 22, 2021 22:23
api/test/e2enew/base/http.go Outdated Show resolved Hide resolved
api/test/e2enew/base/http.go Outdated Show resolved Hide resolved
api/test/e2enew/base/http.go Outdated Show resolved Hide resolved
}
for k, v := range reqParams {
if err := writer.WriteField(k, v); err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

api/test/e2enew/base/http.go Outdated Show resolved Hide resolved
api/test/e2enew/base/http.go Outdated Show resolved Hide resolved

reqBody := urlValues.Encode()

return strings.NewReader(reqBody), contentType, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the param to the body? This confused me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http.go In the getReader function, It from the old E2E/ http.go.

My idea is that E2E will be rewritten with the new ginkgo, so I migrate it directly here without modifying the code.

My understanding here is to deal with a variety of data formats, such as form-data, JSON, files.

The incoming params may be in a variety of situations. Here is the third one url.Values set reqParams map[string]string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nic-chen Do you know about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that there is a problem with this. The content of the file should not be read in the test case, but the content of the uploaded file should be read in the main program.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuxiran

Because the front-end has cross-domain problems, it needs to conduct a transparent transmission through the manage API.

Here, the front-end places the file bytes in the body and transfers them to the back-end, and then directly transmits them through the back-end. Therefore, the E2E test needs to read the file here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my question is that why we should put the reqParams to reqBody?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request params include url, prototal, method, header params and body params, they used to form the new request rewarding by golang.

Before this pr, we put all request params to the request body, and we use json to transfor data
due to this new feature, the origin request support not only json, form-data is accepted. Inorder not to disturb the origin formdata, we put others params including: url, prototal, method and header params to the request header, and the reqest body keep the origin body, so that the forwarding request can use the body directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuxiran Thanks for the explanation. 😄

@juzhiyuan juzhiyuan requested a review from starsz February 25, 2021 05:23
@juzhiyuan juzhiyuan merged commit e51323f into apache:master Feb 25, 2021
@Jaycean Jaycean deleted the debug_online_with_file branch March 2, 2021 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants