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: implement nil-safe Context methods on typhon.Request #176

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented Jan 29, 2025

While downstreaming a new slog feature to Monzo's internal monorepo, I discovered that the empty typhon.Request value panics when the various context.Context methods are called on it:

typhon.Request{}.Value("foo") // panics

This happens because typhon.Request embeds context.Context, and so the zero value of typhon.Request has a typed nil context. This PR adds wrappers for the context.Context methods that behave like context.Background() on the empty typhon.Request value instead.

This is arguably not a bug: users of the library are expected to use either typhon.NewRequest or typhon.NewRawRequest to construct request values, these functions explicitly handle the nil context case, and the library makes no promises about the validity of the zero value.

Either way, we already have unit tests in our internal monorepo that build typhon.Request values manually and with the current typhon.Request implementation those tests are unstable: introducing a new call to one of the context.Context methods in a library (as I did in monzo/slog#22), can indirectly cause consumers of that library to panic. It's not difficult to add wrapper methods that work correctly for the empty typhon.Request value, so that's exactly what this PR does.

@craigfe craigfe marked this pull request as ready for review January 29, 2025 09:58
@craigfe craigfe requested a review from EddShaw January 30, 2025 08:54
go-version: [1.17.x, 1.18.x]
go-version: [1.19.x, 1.23.x]
Copy link
Member Author

Choose a reason for hiding this comment

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

Fly-by change: the minimum supported version in go.mod is already 1.19, and I've introduced a use of any that causes this library to fail to compile on 1.17.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there may be references to 1.17 and 1.18 elsewhere, breaking CI 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, no. Not sure why CI is blocked on actions that no longer exist. 🤔

Copy link
Contributor

@EddShaw EddShaw left a comment

Choose a reason for hiding this comment

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

Nice!

@EddShaw
Copy link
Contributor

EddShaw commented Jan 30, 2025

I think we might need someone with repo permissions to fix branch protection rules.

@craigfe craigfe merged commit 8b5d72a into monzo:master Jan 31, 2025
2 checks passed
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.

2 participants