-
Notifications
You must be signed in to change notification settings - Fork 398
Transports: DRY HTTP Transport & Client classes #5099
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require_relative 'http/client' | ||
|
|
||
| module Datadog | ||
| module Core | ||
| module Transport | ||
| # Raised when configured with an unknown API version | ||
| class UnknownApiVersionError < StandardError | ||
| attr_reader :version | ||
|
|
||
| def initialize(version) | ||
| super | ||
|
|
||
| @version = version | ||
| end | ||
|
|
||
| def message | ||
| "No matching transport API for version #{version}!" | ||
| end | ||
| end | ||
|
|
||
| # Raised when the API verson to downgrade to does not map to a | ||
| # defined API. | ||
| class NoDowngradeAvailableError < StandardError | ||
| attr_reader :version | ||
|
|
||
| def initialize(version) | ||
| super | ||
|
|
||
| @version = version | ||
| end | ||
|
|
||
| def message | ||
| "No downgrade from transport API version #{version} is available!" | ||
| end | ||
| end | ||
|
|
||
| # Base class for transports. | ||
| class Transport | ||
| attr_reader :client, :apis, :default_api, :current_api_id, :logger | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I think some (all?) of these can be made |
||
|
|
||
| class << self | ||
| # The HTTP client class to use for requests, derived from | ||
| # Core::Transport::HTTP::Client. | ||
| # | ||
| # Important: this attribute is NOT inherited by derived classes - | ||
| # it must be set by every Transport class that wants to have a | ||
| # non-default HTTP::Client instance. | ||
| attr_accessor :http_client_class | ||
| end | ||
|
Comment on lines
+43
to
+51
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This... is a kinda weird pattern. May I suggest making it an argument that gets passed to the constructor, or a regular method that needs to be overwritten by subclasses?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking in general was to make the transports more declarative but I can try a version with an instance method.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think declarative sometimes works really well... In our case, I think it's made it really hard to reason and fix things with the current transport design. My thinking is that at this point we should strip out as much as possible of it until we get to a better state. Then, once we're happier with the transport design, we can reintroduce the declarative features as a nicer DSL for underpinnings we're happy with, and it won't affect with the overall design we're trying to fix. |
||
|
|
||
| def initialize(apis, default_api, logger:) | ||
| @apis = apis | ||
| @default_api = default_api | ||
| @logger = logger | ||
|
|
||
| set_api!(default_api) | ||
| end | ||
|
|
||
| def current_api | ||
| apis[current_api_id] | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def set_api!(api_id) | ||
| raise UnknownApiVersionError, api_id unless apis.key?(api_id) | ||
|
|
||
| @current_api_id = api_id | ||
| client_class = self.class.http_client_class || Core::Transport::HTTP::Client | ||
| @client = client_class.new(current_api, logger: logger) | ||
| end | ||
|
|
||
| def downgrade?(response) | ||
| return false unless apis.fallbacks.key?(current_api_id) | ||
|
|
||
| response.not_found? || response.unsupported? | ||
| end | ||
|
|
||
| def downgrade! | ||
| downgrade_api_id = apis.fallbacks[current_api_id] | ||
| raise NoDowngradeAvailableError, current_api_id if downgrade_api_id.nil? | ||
|
|
||
| set_api!(downgrade_api_id) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I realize this already partially existed before.
Having said that -- these two situations only happen when we have a bug in our code that can't be recovered from, and we don't particularly have a need to access the version or to recover from the individual errors.
So my suggestion is -- let's get rid of these and replace them with
raise ArgumentError, "Unknown API version: #{api_id}"(or similar) below. I think that saves us some code and we can always introduce the more specific classes if we ever need to do anything with them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this, I don't think I'm strongly attached to the existing exceptions.