fullstorydev / grpcui

An interactive web UI for gRPC, along the lines of postman
MIT License
5.24k stars 388 forks source link

Correctly initialise gRPCurl text area #340

Open gerektoolhy opened 1 month ago

gerektoolhy commented 1 month ago

Description

https://github.com/fullstorydev/grpcui/pull/241 introduced feature where a grpcurl command would be rendered. The grpcurl could then be copied and run in terminal repeatedly.

Issue

After some use, I have noticed a small bug where on initial page load, the grpcurl would contain an incorrectly serialized payload [Object object]. Expectation is that the payload should contain a valid JSON data. At a minimum this has to be an empty JSON object {}, or anything that has been entered either from the first tab, or manually in the Request box.

Screenshot 2024-10-05 at 21 31 03

Root cause

The issue is introduced by line https://github.com/fullstorydev/grpcui/blob/master/internal/resources/webform/webform.js#L158 which calls, which calls updateCurlCommand(requestObj);. The problem with this line is that requestObj is not a string parameter. This object ends up being concatenated to a string, and gets translated to Object object.

To trace this issue down I have instrumented updateCurlCommand() function and logged out how many times it was called and the stack trace along with it. It turned out that on page load this function was called twice. Surprisingly, the first call contained the correct JSON value, but the second not.

The first call was made from updateJsonRequest, which converted request object to string and passed that to updateCurlCommand.

Fix

It turns out, that the second call to updateCurlCommand is not needed, and thus can be removed.

After fix the grpcurl textbox correctly contains a valid JSON as data parameter:

grpcurl -plaintext -d '{
  "diffIds": []
}' localhost:9011 <host_redacted>
gerektoolhy commented 1 month ago

FYI @dragonsinth

gerektoolhy commented 4 weeks ago

@TilGP anything else needs for this to be merged?

TilGP commented 4 weeks ago

@TilGP anything else needs for this to be merged?

Apparently a review from someone with write access, which I am not. (I'm not affiliated with fullstorydev either). I just reviewed it with the hope, that it will bring some attention from the maintainers to this PR.

gerektoolhy commented 4 weeks ago

@TilGP got it, I wrongly assumed that you were. Thanks for confirming the fix though.

FYI @dragonsinth @jhump