refactor/remove unnecessary syncserverrequest impl #1295
Conversation
src/db/transaction.rs
Outdated
| @@ -240,7 +240,7 @@ impl FromRequest for DbTransactionPool { | |||
| } | |||
| }; | |||
| let method = req.method().clone(); | |||
| let user_id = match req.get_hawk_id() { | |||
| let user_id = match HawkIdentifier::try_from(&req) { | |||
There was a problem hiding this comment.
A slightly more "rusty"(?) approach to this would be:
let user_id = HawkIdentifier::try_from(&req).map_err(|e| {
warn!("⚠️ Bad Hawk Id: {:?}", e; "user_agent"=> useragent);
e
})?;
Clippy will sometimes flag constructs like these, where you're passing the Ok value back as is.
src/web/extractors.rs
Outdated
| return Ok(HawkIdentifier::cmd_dummy()); | ||
| } | ||
| let method = req.method().clone(); | ||
| // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so |
There was a problem hiding this comment.
Looking at what goes on here, the sequence is all blocking code. Is the concern that we don't want the risk of something modifying the extensions?
There was a problem hiding this comment.
I don't think Taddes added that comment (or the .clone()), but I agree that we don't need a clone here. Ref represents an immutable borrow to something in a RefCell, and many Refs can exist for a single RefCell at any given time so I don't think any locking is involved: https://docs.rs/rustc-std-workspace-std/1.0.1/std/cell/struct.RefCell.html#method.borrow
src/web/extractors.rs
Outdated
| return Ok(HawkIdentifier::cmd_dummy()); | ||
| } | ||
| let method = req.method().clone(); | ||
| // NOTE: `connection_info()` gets a mutable reference lock on `extensions()`, so |
There was a problem hiding this comment.
I don't think Taddes added that comment (or the .clone()), but I agree that we don't need a clone here. Ref represents an immutable borrow to something in a RefCell, and many Refs can exist for a single RefCell at any given time so I don't think any locking is involved: https://docs.rs/rustc-std-workspace-std/1.0.1/std/cell/struct.RefCell.html#method.borrow
src/web/extractors.rs
Outdated
| HawkIdentifier::extrude(req, method.as_str(), req.uri(), &ci, secrets) | ||
| } | ||
| } | ||
|
|
||
| impl FromRequest for HawkIdentifier { |
There was a problem hiding this comment.
It looks like our new TryFrom impl almost exactly duplicates this extractor (save the dockerflow bit) -- do you think it's worth combining them into a single trait impl?
taddes
changed the title
|
I think this needs another rebase after the merging of #1281. Happy to assist if it would be helpful :) |
…emoved use statements in middleware
taddes
force-pushed
the
refactor/remove-unnecessary-syncserverrequest-impl
branch
from
ae126cc to
a21073d
Compare
Description
This task entailed removing unnecessary impl blocks for syncserverrequest, as they were not used. The implementation of the TryFrom trait for HawkIdentifier followed by moving it into
extractors.rssufficiently tidied up the code inmod.rsscoping the functionality to where the structure is defined and various traits implemented forHawkIdentifierIn the Makefile, added a quoting to avoid certain paths with space characters breaking the build. @ethowitz was able to clear this issue up and had dealt with it in the past.
Testing
Ensuring compilation should be sufficient. Ran into some formatting deltas between CircleCI and clippy in initial tests.
Issue(s)
Closes link.
https://mozilla-hub.atlassian.net/browse/CONSVC-1693