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

Can we prepend onto the path to allow for how the server is set up? #252

Closed
oliverfoggin opened this issue Mar 27, 2024 · 6 comments · Fixed by #253
Closed

Can we prepend onto the path to allow for how the server is set up? #252

oliverfoggin opened this issue Mar 27, 2024 · 6 comments · Fixed by #253
Assignees

Comments

@oliverfoggin
Copy link

oliverfoggin commented Mar 27, 2024

I have a service like example.v1.ExampleService/SayHello.

Which gives me a full url like... https://our.server.com/example.v1.ExampleService/SayHello.

But the actual path in the server has been configured to be like... https://our.server.com/some/api/path/example.v1.ExampleService/SayHello.

I'm not sure how to configure this as it doesn't seem to be something I have access to?

If I add the path onto the host when creating the config it is truncated by the time the request is made.

Is this something that we're able to do?

Thanks

@oliverfoggin
Copy link
Author

Looking further into this the url for the url request is created using...

URL(string: path, relativeTo: URL(string: host))

But the generated code has a path with an initial /. This works by replacing the path on the relativeTo url with the path provided.

If you remove the initial / from the path then this works by appending the path onto the existing path of the relativeTo url.

So, because the generated code looks like this...

@available(iOS 13, *)
internal func `getUser`(request: User_V1_GetUserRequest, headers: Connect.Headers = [:]) async -> ResponseMessage<User_V1_GetUserResponse> {
  return await self.client.unary(path: "/user.v1.UserService/GetUser", idempotencyLevel: .unknown, request: request, headers: headers)
}

It will take whatever is provided in the host like... https://google.com/a/b/c/ and create a URL like https://google.com/user.v1.UserService/GetUser.

If the generated code drops the initial / then we'd get a url like https://google.com/a/b/c/user.v1.UserService/GetUser which is what I'm hoping to get to with the project I'm working on.

Is this something that is configurable in the current project? Or would this require some change to make this work?

Thanks

@oliverfoggin
Copy link
Author

Alternatively... is there a way to add "path" details to the proto file so that it will create a path in the generated code like /a/b/c/user.v1.UserService/GetUser as that would also solve the issue we're having.

Thanks

@rebello95
Copy link
Collaborator

Hey @oliverfoggin - thank you for flagging this issue and for providing examples in #161. I'll investigate this and come up with a resolution. I think the right fix will likely be in how URLs are constructed in the ProtocolClient

@oliverfoggin
Copy link
Author

@rebello95 I agree it definitely needs to be re-looked at.

The quickest/smallest fix would be to revert #161 but that would leave the issue mentioned in there.

Possibly changing the config to take a host and path and then concatenating them together with the proto service path.

Or providing a baseURL and concatenating them.

Either way it should not remove any path elements entered by the developer.

Thanks

@rebello95
Copy link
Collaborator

I think the changes in #161 should stick around since it has parity with what connect-go generates, but we can fix this in the URL concatenation logic, yes.

I have a fix which I will write tests for and open a PR.

@rebello95
Copy link
Collaborator

#253

rebello95 added a commit that referenced this issue Mar 28, 2024
This adds support for specifying hosts with additional path extensions,
such as `https://connectrpc.com/a/b/c/`. Previously, `/a/b/c` would be
dropped.

Resolves #252.

Also related to #161.
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 a pull request may close this issue.

2 participants