Enumerate errors for peek_with_timeout

[shinghim]

I returned the same values for the new Error case that were previously returned for a None in the peek_default and peek_v1 usages to maintain the same behavior. Maybe it'd be useful to update them to say something about a timeout?

Fixes #396

[coveralls]

Pull Request Test Coverage Report for Build 12780541195

Details

• 19 of 33 (57.58%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.09%) to 60.664%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-directory/src/lib.rs	12	15	80.0%
payjoin-directory/src/db.rs	7	18	38.89%

Totals
Change from base Build 12771456917:	-0.09%
Covered Lines:	2924
Relevant Lines:	4820

---

💛 - Coveralls

[DanGould]

concept ACK 7f27c9d, though it seems like a complete this would really entail defining an Error enum containing both RedisResult's RedisError and Elapsed rather than the Ok value being a RedisResult.

Perhaps add one more commit that does that.

A docstring describing the behavior of peek_v1 (or any function) is absolutely appropriate and encouraged.

[shinghim]

Ah makes sense, ill update this hopefully tomorrow

[DanGould]

Nice, this is what I was looking for.

I think it would be more appropriate for the DbPoolError to be defined in db.rs, (potentially named db::Error) since it's not an error on the whole lib, but rather the db module specifically. Additionally, I would expect a payjoin-directory/src/error.rs to host HandlerError defined as lib.rs too, and since there's only one error type it may not be worth the overhead of new files and imports for the time being.

I also see an opportunity to reduce duplicate code in lib.rs since the pattern is repeated:

fn handle_peek(
    result: Result<Vec<u8>, DbPoolError>,
    timeout_response: Response<BoxBody<Bytes, hyper::Error>>,
) -> Result<Response<BoxBody<Bytes, hyper::Error>>, HandlerError> {
    match result {
        Ok(buffered_req) => Ok(Response::new(full(buffered_req))),
        Err(e) => match e {
            DbPoolError::Redis(re) => Err(HandlerError::BadRequest(re.into())),
            DbPoolError::Timeout(_) => Ok(timeout_response),
        },
    }
}

May you follow up with the module cleanup (putting the new error in db.rs)? The deduplication is optional, since duplicate code is already contained in this repo and keeping it around would not be a regression. But i'd like to see it :)

Thanks for the persistent contributions 🫡

[DanGould]

IF we want to return the Redis error to the client (big IF, @nothingmuch what do you think) I think we can return Redis(e) => Err(HandlerError::BadRequest(e.into()) rather than the top level error, since that's the only returned variant. Then Err(_) => match e is appropriate at the higher level.

[shinghim]

Is the concern here that Redis will be replaced in the near future (#419), or just generally that the client shouldn't have to worry about the underlying implementation?

[DanGould]

exposing internals of the server

1. isn't generally actionable by most callers
2. should just be error logged instead and
3. exposes details about the server state that potentially could be used for an attack. though I'm not sure I can think of one off the top of my head, my idea of best practice is to reveal only that necessary and actionable serverstate for the client to operate and keep everything else private.

[nothingmuch]

Yeah I concur with Dan (sorry for the late response to this), these errors inform the operator of the directory of the right course of action, and they are the only party that can address this, so IMO this should be printed to error log and result in a 500 error from the directory to the client.

(edit to add: additionally if the appropriate response is bad request, i.e. client's responsibility, that should be detected by the directory before redis ever enters the picture)

[DanGould]

I think this needs to be pub(crate) since we don't plan to expose it in the public API.

[shinghim]

Feedback makes sense, I'll move the error and add the dedupe. I'll wait until the Redis error propagation discussion is resolved before pushing all of the updates here at once

[DanGould]

@shinghim Has your question been answered? I think @nothingmuch closed the loop

[shinghim]

Sorry was out on vacation but I pushed the changes up

[DanGould]

tACK bfb4a96

Nicely done. Thanks for going above and beyond and generalizing with handle peek. I hope we didn't put too much pressure on you on vacation. You're a joy to collaborate with and I don't think I'm the only one who was eager for your return.

[DanGould]

If I were forced to come up with one TODO on this PR, or feedback for next time, I would suggest that crate::db::Error were not imported in this file, and rather the function signature was qualified as db::Error since db::Error is not semantically the top level Error for lib.rs. We don't have one, which is why there's no conflict, but HandlerError would be the closest thing.

Writing this also made me notice that some Result types are duplicated in a bunch of places, so there is an argument to be made for

• db::Result = Result<Vec<u8>, Error>
• HandlerResult = Result<Response<BoxBody<Bytes, hyper::Error>>, HandlerError>

The result types are really getting nitty gritty but I figure it can't hurt to share

[shinghim]

I'll open a separate PR to fix these

[shinghim]

Nicely done. Thanks for going above and beyond and generalizing with handle peek. I hope we didn't put too much pressure on you on vacation. You're a joy to collaborate with and I don't think I'm the only one who was eager for your return.

Thanks for being so patient, no pressure at all! I hope to keep contributing and have less churn on my PRs as i get more comfortable with the codebase 👍🏼