Skip to content

Remove the IntoAbsForm trait#480

Merged
zaharidichev merged 2 commits intozd/use-auth-overridefrom
ver/transparent-override-absolute
Apr 20, 2020
Merged

Remove the IntoAbsForm trait#480
zaharidichev merged 2 commits intozd/use-auth-overridefrom
ver/transparent-override-absolute

Conversation

@olix0r
Copy link
Member

@olix0r olix0r commented Apr 16, 2020

@zaharidichev What do you think of this?

This change modifies the HasSettings trait to return a Settings
(rather than a reference). The Settings type is a simple type with a
few bools, so it's safe to copy freely.

This enables us to implement HasSettings for http::HttpEndpoint by
overriding the was_absolute_from when appropriate without having to
introduce an additional trait

This change modifies the `HasSettings` trait to return a `Settings`
(rather than a reference). The `Settings` type is a simple type with a
few bools, so it's safe to copy freely.

This enables us to implement `HasSettings` for `http::HttpEndpoint` by
overriding the `was_absolute_from` when appropriate without having to
introduce an additional trait
@olix0r olix0r requested review from hawkw and zaharidichev April 16, 2020 00:14
@hawkw
Copy link
Contributor

hawkw commented Apr 16, 2020

Looks like this might've broken something?

@olix0r
Copy link
Member Author

olix0r commented Apr 16, 2020

@hawkw afaict that test may be flakey (passes locally)

Copy link
Member

@zaharidichev zaharidichev left a comment

Choose a reason for hiding this comment

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

I see how this is better and removed the unnecessary indirection. Thanks!

The extractor pattern is useful when we need to use additional
configuration or when we don't control the type over which we're
extracting a value (like an http::Request). But in the case of authority
overrides, we have all of the information we need on the target type.

This change replaces the ExtractAuthority strategy with a
CanOverrideAuthority trait that must be implemented by targets.

I've also modified logging to be at the debug level (request
modification seems useful to observe when debugging), and I've renamed
some variables to match the tracing output.

I think, as a follow up, I'd prefer that we call this module
`override_authority` (rather than "overwrite"), as this matches the
naming in the controller/API.

I've also moved `layer => Layer::new`, which is the newer idiom we're
targetting.
@zaharidichev zaharidichev merged commit 6b25de1 into zd/use-auth-override Apr 20, 2020
@olix0r olix0r deleted the ver/transparent-override-absolute branch May 25, 2021 15:46
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