Skip to content
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

Convert StatusCode to Status #1472

Open
thomas-zahner opened this issue Jul 30, 2024 · 5 comments
Open

Convert StatusCode to Status #1472

thomas-zahner opened this issue Jul 30, 2024 · 5 comments
Labels
question Further information is requested request-for-comments

Comments

@thomas-zahner
Copy link
Member

thomas-zahner commented Jul 30, 2024

Currently I don't see a possibility to convert a StatusCode into a Status.
It only provides the following function to construct a status code without doing it manually:

pub fn new(response: &Response, accepted: Option<HashSet<StatusCode>>) -> Self { }

So reqwest::Response and the accepted map are coupled with the creation of Status. It would be better in my opinion if http::StatusCode was enough to create Status. Then a new function could be provided in Status to apply an accept map to convert the enum value. This could also be done in a response chain step in the future.

I noticed this issue when implementing the FlareSolverr handler where only the status code is available and not a full reqwest::Response. In this instance we don't know the accept map and the match statement's logic would have to be copy-pasted.

@mre
Copy link
Member

mre commented Jul 30, 2024

I like it. Does it mean that the status is not the definitive "final" status based on what are the accepted status codes, though?
I don't have an opinion on whether we need that invariant. Just checking.

@thomas-zahner
Copy link
Member Author

Well yeah, that's true. I didn't really consider that.
Thought about it a bit more and I now think that we should not change it that way. Instead we might consider making the accept_map available somehow to the chain. Maybe with a new context parameter in the chain?

@mre
Copy link
Member

mre commented Aug 7, 2024

You mean using context a bit like in Go as a request-scoped key-value store across API boundaries?
That would be neat, yeah. The plugin authors could pass arbitrary values down the chain.
Question is what the values would look like, but maybe we could have a ContextValue trait to make it typesafe. Then all types could implement it. If that's too complicated, we could also use a string or so, which later would need to be deserialized.
Or we use the trait of a serialization framework like Serde.

@thomas-zahner
Copy link
Member Author

Yes it could be like that. But for the problem I've mentioned above it would suffice if the context had accepted: Option<HashSet<StatusCode>>
So we don't necessarily need a key-value store unless we want users to be able to pass custom data between chain elements.

@mre
Copy link
Member

mre commented Aug 19, 2024

My thinking was that if we add context, we might as well just make it a key-value store. If accepted is the only thing we need for now, that's fine, but there's very minimal overhead to use a HashMap in that case, which can be extended in the future without having to introduce a breaking change. The downside is that it's stringy-typed, so accepted would just be a key lookup, but I think it's fine. If we want, we can also do

struct Context {
  // Well-known fields
  accepted: Option<HashSet<StatusCode>>,
  
  // Arbitrary  key-value pairs for customizability
  values: HashMap<String, ContextValue>,
}

then accepted always needs to be set because it's a "known" field. But it might be a bit too much as a start?

@mre mre added question Further information is requested request-for-comments labels Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested request-for-comments
Projects
None yet
Development

No branches or pull requests

2 participants