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.

Sign up for GitHub

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

Jump to bottom

add option to allow access methods from default url #1214

Merged
merged 3 commits into from
Sep 2, 2020
Merged

add option to allow access methods from default url #1214

merged 3 commits into from
Sep 2, 2020

Conversation

wasphin
Copy link
Member

@wasphin wasphin commented Aug 22, 2020

No description provided.

@gydong
Copy link
Contributor

gydong commented Aug 31, 2020

这个没必要修改代码，在mapping中增加一行自己到自己的映射关系即可：
"SERVICE/METHOD => SERVICE/METHOD"

@wasphin
Copy link
Member Author

wasphin commented Aug 31, 2020

不支持添加 service 同名的映射吧?

我这里会提示:

Impossible: _fullname_service and _service_map are inconsistent before inserting

添加时在这里是不是限制了?
https://github.com/apache/incubator-brpc/blob/8801a8444f35e93f37b5d13019577edbdedfd6f6/src/brpc/server.cpp#L1316

@gydong
Copy link
Contributor

gydong commented Sep 1, 2020

我是这样写的，没问题
image

@wasphin
Copy link
Member Author

wasphin commented Sep 1, 2020

@gydong 这样是可以通过 /Echo 访问 SERVICE 的 Echo 方法, 但是不能通过原路径 /SERVICE/Echo 进行访问, 映射后是全路径, 这样如果想要在有映射的情况下, 保留通过 /SERVICE/Echo 访问的方式, 那么需要怎么配呢?

我这边的场景是服务端希望可以支持 gRPC 及 HTTP 进行访问:

  1. gRPC 部分直接使用 proto 定义的方法进行访问, 即: /SERVICE_FULL_NAME/Echo;
  2. HTTP 则通过映射后的路径进行访问, 即: /v1/xxx/Echo.

同时还希望保留 default_method 的行为.

@gydong
Copy link
Contributor

gydong commented Sep 1, 2020

嗯，你说的对，我去了解一下相关背景，看看如何满足你的需求更合适

jamesge
jamesge reviewed Sep 1, 2020
View reviewed changes
src/brpc/server.h Outdated
@@ -334,6 +339,7 @@ class Server {
struct MethodProperty {
bool is_builtin_service;
bool own_method_status;
bool restful_mapped_only;
Copy link
Contributor

Choose a reason for hiding this comment

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

这个要移到下面的params中。

Copy link
Member Author

Choose a reason for hiding this comment

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

已改

src/brpc/server.h Outdated
// Work with restful_mappings, if this flag is true, reject methods accessed
// from default urls (SERVICE/METHOD).
// Default: true
bool restful_mapped_only;
Copy link
Contributor

Choose a reason for hiding this comment

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

叫allow_default_url吧，更明确一点。注意改名后，值是相反的。

Copy link
Member Author

Choose a reason for hiding this comment

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

已改

@wasphin
Copy link
Member Author

wasphin commented Sep 2, 2020

travis 在编译 unittest 时失败, 但应该与此次改动无关.

@jamesge jamesge merged commit 7bda11b into apache:master Sep 2, 2020
@jamesge
Copy link
Contributor

jamesge commented Sep 2, 2020

@wasphin thanks for the commit and UT

@wasphin wasphin deleted the feature/restful-mapped-only branch September 2, 2020 06:29
@wasphin wasphin mentioned this pull request Dec 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesge jamesge jamesge left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wasphin @gydong @jamesge