Replace all unwraps #147

adam-cattermole · 2024-11-14T14:02:04Z

Enable clippy lint unwrap_used - this is in a separate commit so we can drop it before we merge
Most cases was replacing unwraps with expect

src/configuration.rs

alexsnaps · 2024-11-15T11:54:44Z

src/data/attribute.rs

-            result.push((current_prefix, serde_json::to_string(&v).unwrap()));
+            result.push((
+                current_prefix,
+                serde_json::to_string(&v).expect("failed to serialize json Value"),


Would this contain any information about v? If not, maybe .inspect or otherwise use the error to log?
I'd maybe even go as far as dropping that mapping on the floor, log at error! Or explicitly add a null value or... rather than having the whole process crash?

In what cases would the serialization fail?

Right, given the above, it can only be String, Bool, null or some Number... so I guess it shouldn't ever fail indeed... for as long as that invariant remains true tho...

src/data/cel.rs

alexsnaps · 2024-11-15T11:58:36Z

src/service.rs

@@ -57,7 +57,8 @@ impl GrpcService {
    ) -> Result<GrpcResult, StatusCode> {
        let failure_mode = operation.get_failure_mode();
        if let Some(res_body_bytes) =
-            hostcalls::get_buffer(BufferType::GrpcReceiveBuffer, 0, resp_size).unwrap()
+            hostcalls::get_buffer(BufferType::GrpcReceiveBuffer, 0, resp_size)
+                .expect("failed to get_buffer on GrpcReceiveBuffer")


Should this leverage FailureMode?

Yep good point, this and below should be handled

I've handled this by wrapping the Some with an Ok... however I think this is the wrong way to be doing this. This one function is handling multiple different error cases internally and either sending an error or resuming the request based on failure mode, eventually returning an Err(StatusCode::InternalServerError) to the caller - I feel like this could/should be handled by the caller instead but needs some more serious thought in #127

src/service.rs

src/lib.rs

alexsnaps · 2024-11-20T12:00:33Z

src/service/auth.rs

@@ -64,7 +64,7 @@ impl AuthService {
        let mut request = AttributeContext_Request::default();
        let mut http = AttributeContext_HttpRequest::default();
        let headers: HashMap<String, String> = hostcalls::get_map(MapType::HttpRequestHeaders)
-            .unwrap()
+            .expect("failed to retrieve HttpRequestHeaders from host")


Let's leave this for now (and the other 2 calls below, as well as the one in rate_limit.rs), but I think this should eventually become some kind of FailureMode dependent behavior...

100% agreed again here

The expect adds some info, but still causes a panic...which is what we're looking to avoid, no?

maybe unwrap_or() and provide some sane default (an empty map?)?

The primary problem is this should perform an action based on the FailureMode defined for the service - if this service is set to Deny then we don't want to populate the request with a default value and let it through. This is a much larger refactor though than can be solved in this PR

provide some sane default (an empty map?)?

there is no "sane default" in this case, as it will just propagate the error to the next inline... i.e. in this case probably the CEL expression is going to fail. This error here tho, would mean something went wrong with envoy.

As @adam-cattermole rightly pointed out, this would need to fallback to some user configured behavior (analogous to e.g. Limitador being unreachable). So we need to get rid of this one when we figured out that part of the issue (see this "issue" for more context)

alexsnaps

But let's wait on what others in the team think wrt the lint

andrewdavidmackenzie · 2024-11-20T12:43:04Z

I use it, and recommend it, but it can be quite a bit of work ...

I proposed this quite a while back, before we (if we were going to support more the library model) stabilized the API, as I expected a lot of return values to have to change to Result<>

Signed-off-by: Adam Cattermole <[email protected]>

didierofrivia

👍🏼

alexsnaps · 2024-11-20T15:07:13Z

if we were going to support more the library model) stabilized the API, as I expected a lot of return values to have to change to Result<>

This is definitively true for Limitador... here, things a slightly different tho. But yes this is more work, yet useful work... if we don't just replace .unwrap() with .expect() out of ... wanting to do that work, which is think about the issue hard and what it means to the user. Where in this case the user is a moving target: the kuadrant user, the downstream client, which can be an evil actor (e.g. badly encoded query strings...). Anyways, we need to all agree on this lint being in and all doing the work right away. Otherwise, I still believe we are better off doing a pass such as this one e.g. every release? I prefer a glaring .unwrap() than some evil if let Ok(only_care_about_happy_path_err_is_gone) = result {...

adam-cattermole force-pushed the remove-unwrap branch 2 times, most recently from ef8d06f to 726f66a Compare November 14, 2024 14:25

adam-cattermole marked this pull request as ready for review November 15, 2024 10:27

adam-cattermole requested a review from a team as a code owner November 15, 2024 10:27

alexsnaps reviewed Nov 15, 2024

View reviewed changes

adam-cattermole force-pushed the remove-unwrap branch 2 times, most recently from c2fe283 to d245019 Compare November 19, 2024 13:44

maleck13 assigned adam-cattermole Nov 20, 2024

adam-cattermole force-pushed the remove-unwrap branch 2 times, most recently from 4e8e92e to d5c5404 Compare November 20, 2024 09:53

alexsnaps reviewed Nov 20, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

alexsnaps reviewed Nov 20, 2024

View reviewed changes

alexsnaps approved these changes Nov 20, 2024

View reviewed changes

adam-cattermole added 2 commits November 20, 2024 14:43

Replace all unwraps

5c3ea61

Signed-off-by: Adam Cattermole <[email protected]>

Expect in tests

b3d4307

Signed-off-by: Adam Cattermole <[email protected]>

adam-cattermole force-pushed the remove-unwrap branch from d5c5404 to b3d4307 Compare November 20, 2024 14:43

didierofrivia approved these changes Nov 20, 2024

View reviewed changes

adam-cattermole merged commit de863de into main Nov 20, 2024
13 checks passed

adam-cattermole deleted the remove-unwrap branch November 20, 2024 15:17

adam-cattermole added the enhancement New feature or request label Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all unwraps #147

Replace all unwraps #147

adam-cattermole commented Nov 14, 2024 •

edited

Loading

alexsnaps Nov 15, 2024

chirino Nov 15, 2024

alexsnaps Nov 19, 2024

alexsnaps Nov 15, 2024

adam-cattermole Nov 15, 2024

adam-cattermole Nov 19, 2024 •

edited

Loading

alexsnaps Nov 20, 2024

adam-cattermole Nov 20, 2024

andrewdavidmackenzie Nov 20, 2024

andrewdavidmackenzie Nov 20, 2024

adam-cattermole Nov 20, 2024

alexsnaps Nov 20, 2024 •

edited

Loading

alexsnaps left a comment

andrewdavidmackenzie commented Nov 20, 2024

didierofrivia left a comment

alexsnaps commented Nov 20, 2024 •

edited

Loading

Replace all unwraps #147

Replace all unwraps #147

Conversation

adam-cattermole commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-cattermole Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsnaps Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

alexsnaps left a comment

Choose a reason for hiding this comment

andrewdavidmackenzie commented Nov 20, 2024

didierofrivia left a comment

Choose a reason for hiding this comment

alexsnaps commented Nov 20, 2024 • edited Loading

adam-cattermole commented Nov 14, 2024 •

edited

Loading

adam-cattermole Nov 19, 2024 •

edited

Loading

alexsnaps Nov 20, 2024 •

edited

Loading

alexsnaps commented Nov 20, 2024 •

edited

Loading