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

fix(http server): omit jsonrpc details in health API #785

Merged
merged 4 commits into from
Jun 1, 2022
Merged

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented May 26, 2022

Closing #784

Ordinary HTTP GET requests doesn't expect the body the be formatted as JSON-RPC responses which this fixes i.e, just regular JSON.

Closing #784
Ordinary GET requests doesn't expect the body the be formatted as
`JSON-RPC` responses which this fixes.
@niklasad1 niklasad1 requested a review from a team as a code owner May 26, 2022 11:37
@niklasad1 niklasad1 changed the title fix(http server): omit jsonrpc details health API fix(http server): omit jsonrpc details in health API May 26, 2022
http-server/src/server.rs Outdated Show resolved Hide resolved
Comment on lines +181 to +183
if !path.starts_with("/") {
return Err(Error::Custom(format!("Health endpoint path must start with `/` to work, got: {}", path)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering; why do we care about the path starting with '/'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have this check, if / is missing that will fail and it's quite tricky to debug.

I have already done that myself but more ideally would be Path type instead of a String

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok gotcha; that makes sense!

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a suggestion on plucking the result out without the intermediate Value stuff :)

@niklasad1 niklasad1 self-assigned this May 31, 2022
http-server/src/server.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants