Skip to content

Replace ExtractAuthority with CanOverrideAuthority#481

Merged
zaharidichev merged 1 commit intover/transparent-override-absolutefrom
ver/no-extractor
Apr 20, 2020
Merged

Replace ExtractAuthority with CanOverrideAuthority#481
zaharidichev merged 1 commit intover/transparent-override-absolutefrom
ver/no-extractor

Conversation

@olix0r
Copy link
Member

@olix0r olix0r commented Apr 16, 2020

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.

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.
@olix0r olix0r requested a review from zaharidichev April 16, 2020 16:36
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.

LGTM!

@zaharidichev zaharidichev merged commit 8bc4aa0 into ver/transparent-override-absolute Apr 20, 2020
zaharidichev pushed a commit that referenced this pull request Apr 20, 2020
* Remove the IntoAbsForm 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

* Replace ExtractAuthority with CanOverrideAuthority (#481)

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.
@olix0r olix0r deleted the ver/no-extractor 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.

2 participants