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

EncodeURL will ignore some fields if request message contains repeated custom message #3141

Closed
CaoYan1 opened this issue Dec 28, 2023 · 11 comments · May be fixed by #3311 or #3184
Closed

EncodeURL will ignore some fields if request message contains repeated custom message #3141

CaoYan1 opened this issue Dec 28, 2023 · 11 comments · May be fixed by #3311 or #3184
Labels
bug Something isn't working

Comments

@CaoYan1
Copy link

CaoYan1 commented Dec 28, 2023

What happened:

I defined an RPC method:

message MsgInline {
}

message Request {
  string id = 1;
  repeated MsgInline msg = 2;
}

message Response {
}

service MyService {
  rpc Call(Request) returns (Response) {
    option (google.api.http) = {
      post: "/my-api/{id}",
      body: "*",
    };
  }
}

And I called it via http client, the id field in url path was always empty.

Anything else we need to know?:

image

I find that form.EncodeValues(msg) will return error[unsupported message type:] if the request message contains repeated custom message, and ignore the rest fields, then my id field is ignored. I think an unsupported field shouldn't make other fields don't work.

Environment:

  • Kratos version: v2.7.2
  • Go version (use go version): 1.21.1
  • OS (e.g: cat /etc/os-release): macos
@CaoYan1 CaoYan1 added the bug Something isn't working label Dec 28, 2023
Copy link

dosubot bot commented Dec 28, 2023

🤖

Hey there @CaoYan1! 🖐️

I'm Dosu, a friendly bot here to help you out while waiting for a human maintainer. I'm your go-to for solving bugs, answering questions, and helping you become the best contributor you can be! 🚀

Just wanted to say hi and let you know I'm here for you. Hang tight, we'll get through your issue together! 💪😀

Based on the information you've provided, it seems like you're encountering an issue where the form.EncodeValues(msg) function is returning an error when it encounters a repeated custom message in your request, causing the rest of the fields to be ignored.

This behavior is due to how the form.EncodeValues(msg) function handles unsupported field types. When it encounters an unsupported field type, it returns an error and stops processing the rest of the fields. This is why your id field is being ignored. You can see this in the encodeMessage function in the encoding/form/proto_encode.go file:

func encodeMessage(msgDescriptor protoreflect.MessageDescriptor, value protoreflect.Value) (string, error) {
	// ...
	default:
		return "", fmt.Errorf("unsupported message type: %q", string(msgDescriptor.FullName()))
	}
}

However, I found a similar issue in the Kratos repository that was resolved: 当message中有自定义list消息且url中有参数时,使用生成的http client无法生成正确的url. This issue was addressed in the following pull request: 修复地址栏参数.

Based on this, it seems like the Kratos team has made some changes to handle this scenario. I recommend checking out these resources to see if they can help resolve your issue. If you're still encountering problems, could you please provide more details about your use case and the exact error message you're seeing? This will help me better understand the issue and provide a more accurate solution.

Sources


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@CaoYan1
Copy link
Author

CaoYan1 commented Dec 28, 2023

My Kratos version is latest(2.7.2) and doesn't work still

@kvii
Copy link
Contributor

kvii commented Jan 3, 2024

Can you provide your http client code? I try to reproduce your case, but there was no issue found.

Here is my reproduce repo.

# send http request
curl http://localhost:8000/my-api/1 -X POST -H 'Content-Type: application/json' -d '{"msg":[{}]}'

# console
id:"1"  msg:{}

@CaoYan1
Copy link
Author

CaoYan1 commented Jan 10, 2024

Based on your repo, you can try this code. I can reproduce this issue. @kvii

func TestHttpClient(t *testing.T) {
	client, _ := http.NewClient(context.Background(), http.WithEndpoint("localhost:8000"))
	c := NewMyServiceHTTPClient(client)
	resp, err := c.Call(context.Background(), &Request{
		Id: "id",
		Msg: []*MsgInline{
			{},
		},
	})
        // will return 404 error
	fmt.Println(resp, err)
}

@kvii
Copy link
Contributor

kvii commented Jan 10, 2024

Oh, this is an issue of kratos http client, not server. I pushed a commit to my repo, add tests for this issue at here.

Source codes:

  • Generated http client side code in my reproduce repo:

https://github.com/kvii/issue3141/blob/dd94e68294372d6194fc440f41021ae4ea7aa98b/api/helloworld/v1/greeter_http.pb.go#L72-L73

  • Codes in binding.EncodeURL:

func EncodeURL(pathTemplate string, msg interface{}, needQuery bool) string {
if msg == nil || (reflect.ValueOf(msg).Kind() == reflect.Ptr && reflect.ValueOf(msg).IsNil()) {
return pathTemplate
}
queryParams, _ := form.EncodeValues(msg)

@CaoYan1
Copy link
Author

CaoYan1 commented Jan 16, 2024

Cool! Do you have a plan to fix it?

@kvii
Copy link
Contributor

kvii commented Jan 16, 2024

😭 I'm busy now.

@CaoYan1
Copy link
Author

CaoYan1 commented Jan 16, 2024

I'd like to help you.

@kvii
Copy link
Contributor

kvii commented Jan 31, 2024

I've created a PR for this issue. But the code style is terrible.

@web-xiaxia
Copy link

web-xiaxia commented Feb 28, 2024

这个问题 #2805 应该是解决了的

@kratos-ci-bot
Copy link
Collaborator

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


This issue https://github.com/go-kratos/kratos/pull/2805 should be solved

@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Jun 1, 2024
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 8, 2024
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants