[PM-18042] Build request response structure #275
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
coroiu
force-pushed
the
PM-18042-build-request-response-structure
branch
from
762113a to
43882f0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
+ Coverage 71.18% 71.38% +0.19%
==========================================
Files 230 237 +7
Lines 18634 18938 +304
==========================================
+ Hits 13265 13519 +254
- Misses 5369 5419 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| impl TryFrom<Vec<u8>> for RpcRequestMessage { | ||
| type Error = serde_json::Error; | ||
|
|
||
| fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> { | ||
| serde_json::from_slice(&value) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<RpcRequestMessage> for Vec<u8> { | ||
| type Error = serde_json::Error; | ||
|
|
||
| fn try_from(value: RpcRequestMessage) -> Result<Self, Self::Error> { | ||
| serde_json::to_vec(&value) | ||
| } | ||
| } |
There was a problem hiding this comment.
thought: this is another thing I'm a bit divided on; the levels of serialization. At this point we don't know how to deserialize the request (because we don't know its type) but we still need to wrap it somehow and so we end up with a nested Vec<u8> field which contains the actual request data. This will then be deserialized by the rpc handler
There was a problem hiding this comment.
Technically RPC can be seen as built on top of the req/res paradigm that the IPC client provides at its core, that's why we are actually defining a serilalization format (serde_json) here, because it's RPC over TypedMessages
## 🎟️ Tracking <!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. --> ## 📔 Objective Refactoring the IPC to use serde instead. Putting this in a separate PR to make reviewing easier ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
crates/bitwarden-ipc/Cargo.toml
Outdated
| bitwarden-error = { workspace = true } | ||
| bitwarden-threading = { workspace = true } | ||
| erased-serde = "0.4.6" |
There was a problem hiding this comment.
| erased-serde = "0.4.6" | |
| erased-serde = ">=0.4.6, <0.5" |
| type Response: Serialize + DeserializeOwned + 'static; | ||
|
|
||
| /// Used to identify handlers | ||
| fn name() -> String; |
There was a problem hiding this comment.
For some other traits, instead of a function we use an associated constant, like on PayloadTypeName, should we do the same here?
There was a problem hiding this comment.
Ah, yes, I only recently discovered that you could use constants like this (during a review I think) so I just missed updating this :)
| &self.partial.request_type | ||
| } | ||
|
|
||
| pub fn full<T>(&self) -> Result<RpcRequestMessage<T>, RpcError> |
There was a problem hiding this comment.
full is a bit of a strange name for a function, maybe deserialize_full to be more clear?
|
🎟️ Tracking
📔 Objective
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes