-
Notifications
You must be signed in to change notification settings - Fork 454
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: add rust demo plugin request block #1091
Conversation
Apologies for the late reply, this pull request involves quite a few changes, and it might take me more time to review. |
if _body_size > 0{ | ||
match self.get_http_request_body(0, _body_size) { | ||
Some(body) => self.req_body.extend(body), | ||
None => {}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You needn't use match, you could use if let to simplify the code, e.g
if let Some(body) = self.get_http_request_body(0, _body_size) { self.req_body.extend(body) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plugins/wasm-rust/src/lib.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use cargo fmt before commit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add cargo fmt and cargo clippy check for the rust plugin wrapper
#1140
PluginConfig: Default + DeserializeOwned + 'static + Clone, | ||
{ | ||
|
||
// fn create_http_context(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this unused function definition.
|
||
// fn create_http_context(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { | ||
fn create_http_context_use_wrapper(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { | ||
// trait 继承没法重写 RootContext 的 create_http_context,先写个函数让上层调下吧 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need to look at this design and see if there is a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just started learning Rust. I don't know how to override the inherited trait function. The expected result is:
fn create_http_context(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> {
match self.create_http_context_wrapper(_context_id) {
Some(http_context) => Some(Box::new(PluginHttpWrapper::new(
self.rule_matcher(),
http_context,
))),
None => None,
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust has generic specialization, but it is still in an unstable state.
It is also possible to not rely on RootContext but rather treat RootContext as a field.
Since there aren't many Rust plugins available right now, we can merge your modifications first.
res_body: Bytes::new(), | ||
config: None, | ||
rule_matcher: rule_matcher.clone(), | ||
http_content: http_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the fields of a structure match the values of the passed parameters, they can be directly omitted. For example, http_content: http_content
is same as just http_content,
e.g
PluginHttpWrapper{
http_content,
// .....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
// fn create_http_context(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { | ||
fn create_http_context_use_wrapper(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { | ||
// trait 继承没法重写 RootContext 的 create_http_context,先写个函数让上层调下吧 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust has generic specialization, but it is still in an unstable state.
It is also possible to not rely on RootContext but rather treat RootContext as a field.
Since there aren't many Rust plugins available right now, we can merge your modifications first.
@@ -10,3 +10,4 @@ proxy-wasm = "0.2.1" | |||
serde = "1.0" | |||
serde_json = "1.0" | |||
uuid = { version = "1.3.3", features = ["v4"] } | |||
multimap = "0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a wrapper for the plugin, it is still necessary to consider the version number of dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency version I want to express is 0.*
Ⅰ. Describe what this PR did
增加rust demo request-block, 增加rust plugin wrapper,增加rust插件编译与单元测试
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews