From 2574d52074cf1e9581c810bf508d70dcfef15cb0 Mon Sep 17 00:00:00 2001
From: Kevin Soltysiak <kevin@ksol.fr>
Date: Mon, 15 Apr 2024 11:17:58 +0200
Subject: [PATCH] Rework the DB api access (#62)

---
 CHANGELOG.md                                  | 10 +++-
 lib/scalingo/api/client.rb                    | 21 ++------
 lib/scalingo/api/endpoint.rb                  |  1 -
 lib/scalingo/client.rb                        | 51 +++++++-----------
 lib/scalingo/core_client.rb                   |  7 ---
 .../{regional_database.rb => database.rb}     |  6 +--
 .../backups.rb                                |  2 +-
 .../databases.rb                              |  2 +-
 lib/scalingo/regional/addons.rb               | 18 +++----
 lib/scalingo/token_holder.rb                  | 25 +--------
 spec/scalingo/api/client_spec.rb              | 54 +++----------------
 spec/scalingo/core_client_spec.rb             | 23 --------
 .../backups_spec.rb                           |  6 +--
 .../databases_spec.rb                         |  6 +--
 ...onal_database_spec.rb => database_spec.rb} |  2 +-
 spec/scalingo/regional/addons_spec.rb         | 32 +++++++++++
 spec/scalingo/token_holder_spec.rb            | 36 -------------
 spec/support/shared.rb                        |  2 +-
 18 files changed, 87 insertions(+), 217 deletions(-)
 rename lib/scalingo/{regional_database.rb => database.rb} (50%)
 rename lib/scalingo/{regional_database => database}/backups.rb (81%)
 rename lib/scalingo/{regional_database => database}/databases.rb (72%)
 delete mode 100644 spec/scalingo/core_client_spec.rb
 rename spec/scalingo/{regional_database => database}/backups_spec.rb (87%)
 rename spec/scalingo/{regional_database => database}/databases_spec.rb (81%)
 rename spec/scalingo/{regional_database_spec.rb => database_spec.rb} (83%)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d236569..cc9878e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,7 +1,13 @@
 ## Unreleased
 
-** Breaking change: automatic digging of the value if the reponse body is an object with a single key
-** Breaking change: remove `Scalingo::API::Reponse` in favor of `Faraday::Response`
+* Breaking change: rework DB api exposition
+* Specs: rewrite all specs
+* Breaking change: endpoint methods declaration is simplified:
+    * based on URI templates
+    * argument and method names are unified
+    * one "main" internal method, `Endpoint#request`
+* Breaking change: automatic digging of the value if the reponse body is an object with a single key
+* Breaking change: remove `Scalingo::API::Reponse` in favor of `Faraday::Response`
 
 ## 3.5.0 - 2023-12-28
 
diff --git a/lib/scalingo/api/client.rb b/lib/scalingo/api/client.rb
index ed59840..7d78ff5 100644
--- a/lib/scalingo/api/client.rb
+++ b/lib/scalingo/api/client.rb
@@ -9,10 +9,12 @@ module API
     class Client
       include TokenHolder
 
-      attr_reader :config, :token_holder, :url
+      attr_reader :config, :token_holder, :url, :region
 
-      def initialize(url, scalingo: nil, config: {})
+      def initialize(url, scalingo: nil, region: nil, config: {})
         @url = url
+        @region = region
+
         parent_config = Scalingo.config
 
         if scalingo
@@ -119,21 +121,6 @@ def authenticated_connection
           conn.adapter(config.faraday_adapter) if config.faraday_adapter
         }
       end
-
-      def database_connection(database_id)
-        raise Error::Unauthenticated unless token_holder.authenticated_for_database?(database_id)
-
-        @database_connections ||= {}
-        @database_connections[database_id] ||= Faraday.new(connection_options) { |conn|
-          conn.response :extract_root_value
-          conn.response :extract_meta
-          conn.response :json, content_type: /\bjson$/, parser_options: {symbolize_names: true}
-          conn.request :json
-          conn.request :authorization, "Bearer", -> { token_holder.database_tokens[database_id]&.value }
-
-          conn.adapter(config.faraday_adapter) if config.faraday_adapter
-        }
-      end
     end
   end
 end
diff --git a/lib/scalingo/api/endpoint.rb b/lib/scalingo/api/endpoint.rb
index 41177c9..2c00bf5 100644
--- a/lib/scalingo/api/endpoint.rb
+++ b/lib/scalingo/api/endpoint.rb
@@ -38,7 +38,6 @@ def initialize(client)
       end
 
       def_delegator :client, :connection
-      def_delegator :client, :database_connection
 
       # Perform a request to the API.
       # path can be an URI template; and faraday expect valid URIs - the parser raises when templates aren't fully expanded.
diff --git a/lib/scalingo/client.rb b/lib/scalingo/client.rb
index bd029db..9c13cf5 100644
--- a/lib/scalingo/client.rb
+++ b/lib/scalingo/client.rb
@@ -2,53 +2,38 @@
 require "scalingo/auth"
 require "scalingo/billing"
 require "scalingo/regional"
-require "scalingo/regional_database"
+require "scalingo/database"
 
 module Scalingo
   class Client < CoreClient
+    URLS = {
+      auth: "https://auth.scalingo.com/v1",
+      billing: "https://cashmachine.scalingo.com",
+      regional: {
+        osc_fr1: "https://api.osc-fr1.scalingo.com/v1",
+        osc_secnum_fr1: "https://api.osc-secnum-fr1.scalingo.com/v1"
+      },
+      database: {
+        osc_fr1: "https://db-api.osc-fr1.scalingo.com/api",
+        osc_secnum_fr1: "https://db-api.osc-secnum-fr1.scalingo.com/api"
+      }
+    }
+
     ## API clients
     def auth
-      @auth ||= Auth.new(
-        "https://auth.scalingo.com/v1",
-        scalingo: self
-      )
+      @auth ||= Auth.new(URLS[:auth], scalingo: self)
     end
 
     def billing
-      @billing ||= Billing.new(
-        "https://cashmachine.scalingo.com",
-        scalingo: self
-      )
+      @billing ||= Billing.new(URLS[:billing], scalingo: self)
     end
 
     def osc_fr1
-      @osc_fr1 ||= Regional.new(
-        "https://api.osc-fr1.scalingo.com/v1",
-        scalingo: self
-      )
+      @osc_fr1 ||= Regional.new(URLS[:regional][:osc_fr1], scalingo: self, region: :osc_fr1)
     end
-    alias_method :apps_api_osc_fr1, :osc_fr1
 
     def osc_secnum_fr1
-      @osc_secnum_fr1 ||= Regional.new(
-        "https://api.osc-secnum-fr1.scalingo.com/v1",
-        scalingo: self
-      )
-    end
-    alias_method :apps_api_osc_secnum_fr1, :osc_secnum_fr1
-
-    def db_api_osc_fr1
-      @db_api_osc_fr1 ||= RegionalDatabase.new(
-        "https://db-api.osc-fr1.scalingo.com/api",
-        scalingo: self
-      )
-    end
-
-    def db_api_osc_secnum_fr1
-      @db_api_osc_secnum_fr1 ||= RegionalDatabase.new(
-        "https://db-api.osc-secnum-fr1.scalingo.com/api",
-        scalingo: self
-      )
+      @osc_secnum_fr1 ||= Regional.new(URLS[:regional][:osc_secnum_fr1], scalingo: self, region: :osc_secnum_fr1)
     end
 
     ## Helpers
diff --git a/lib/scalingo/core_client.rb b/lib/scalingo/core_client.rb
index 7a9914c..cae74d8 100644
--- a/lib/scalingo/core_client.rb
+++ b/lib/scalingo/core_client.rb
@@ -42,10 +42,6 @@ def region(name = nil)
       public_send(name || config.default_region)
     end
 
-    def database_region(name = nil)
-      public_send(name || "db_api_#{config.default_region}")
-    end
-
     ## Authentication helpers / Token management
     def authenticate_with(access_token: nil, bearer_token: nil, expires_at: nil)
       if !access_token && !bearer_token
@@ -107,8 +103,5 @@ def authenticate_with(access_token: nil, bearer_token: nil, expires_at: nil)
     def_delegator :region, :notifiers
     def_delegator :region, :operations
     def_delegator :region, :scm_repo_links
-
-    def_delegator :database_region, :databases
-    def_delegator :database_region, :backups
   end
 end
diff --git a/lib/scalingo/regional_database.rb b/lib/scalingo/database.rb
similarity index 50%
rename from lib/scalingo/regional_database.rb
rename to lib/scalingo/database.rb
index 20fe7e6..a8498e3 100644
--- a/lib/scalingo/regional_database.rb
+++ b/lib/scalingo/database.rb
@@ -1,9 +1,9 @@
 require "scalingo/api/client"
 
 module Scalingo
-  class RegionalDatabase < API::Client
-    require "scalingo/regional_database/databases"
-    require "scalingo/regional_database/backups"
+  class Database < API::Client
+    require "scalingo/database/databases"
+    require "scalingo/database/backups"
 
     register_handlers!(
       databases: Databases,
diff --git a/lib/scalingo/regional_database/backups.rb b/lib/scalingo/database/backups.rb
similarity index 81%
rename from lib/scalingo/regional_database/backups.rb
rename to lib/scalingo/database/backups.rb
index d95d75f..e0a908a 100644
--- a/lib/scalingo/regional_database/backups.rb
+++ b/lib/scalingo/database/backups.rb
@@ -1,7 +1,7 @@
 require "scalingo/api/endpoint"
 
 module Scalingo
-  class RegionalDatabase::Backups < API::Endpoint
+  class Database::Backups < API::Endpoint
     get :list, "databases/{addon_id}/backups"
     post :create, "databases/{addon_id}/backups"
     get :archive, "databases/{addon_id}/backups/{id}/archive"
diff --git a/lib/scalingo/regional_database/databases.rb b/lib/scalingo/database/databases.rb
similarity index 72%
rename from lib/scalingo/regional_database/databases.rb
rename to lib/scalingo/database/databases.rb
index 00fdfe6..c2c842a 100644
--- a/lib/scalingo/regional_database/databases.rb
+++ b/lib/scalingo/database/databases.rb
@@ -1,7 +1,7 @@
 require "scalingo/api/endpoint"
 
 module Scalingo
-  class RegionalDatabase::Databases < API::Endpoint
+  class Database::Databases < API::Endpoint
     get :find, "databases/{id}"
     post :upgrade, "databases/{id}/upgrade"
   end
diff --git a/lib/scalingo/regional/addons.rb b/lib/scalingo/regional/addons.rb
index 8a3d6c5..3b9ee5b 100644
--- a/lib/scalingo/regional/addons.rb
+++ b/lib/scalingo/regional/addons.rb
@@ -12,18 +12,16 @@ class Regional::Addons < API::Endpoint
     get :categories, "addon_categories", connected: false
     get :providers, "addon_providers", connected: false
 
-    def authenticate!(**params, &block)
-      response = token(**params, &block)
-      return response unless response.status == 200
+    def database_client_for(app_id:, id:)
+      response = token(app_id: app_id, id: id)
 
-      client.token_holder.authenticate_database_with_bearer_token(
-        params[:id],
-        response.body[:token],
-        expires_at: Time.now + 1.hour,
-        raise_on_expired_token: client.config.raise_on_expired_token
-      )
+      return nil unless response.status == 200
 
-      response
+      db_url = Scalingo::Client::URLS.fetch(:database).fetch(client.region)
+
+      db_client = Scalingo::Database.new(db_url, region: client.region)
+      db_client.token = response.body.fetch(:token)
+      db_client
     end
   end
 end
diff --git a/lib/scalingo/token_holder.rb b/lib/scalingo/token_holder.rb
index b3d8cf7..2730807 100644
--- a/lib/scalingo/token_holder.rb
+++ b/lib/scalingo/token_holder.rb
@@ -4,45 +4,22 @@ module Scalingo
   module TokenHolder
     def self.included(base)
       base.attr_reader :token
-      base.attr_reader :database_tokens
     end
 
     def token=(input)
       @token = bearer_token(input)
     end
 
-    def add_database_token(database_id, token)
-      @database_tokens ||= {}
-      @database_tokens[database_id] = bearer_token(token)
-    end
-
     def authenticated?
-      valid?(token)
-    end
-
-    def authenticated_for_database?(database_id)
-      return false if database_tokens.nil?
-      return false unless database_tokens.has_key?(database_id)
-
-      valid?(database_tokens[database_id])
+      token.present? && !token.expired?
     end
 
     def authenticate_with_bearer_token(bearer_token, expires_at:, raise_on_expired_token:)
       self.token = build_bearer_token(bearer_token, expires_at: expires_at, raise_on_expired_token: raise_on_expired_token)
     end
 
-    def authenticate_database_with_bearer_token(database_id, bearer_token, expires_at:, raise_on_expired_token:)
-      bearer_token = build_bearer_token(bearer_token, expires_at: expires_at, raise_on_expired_token: raise_on_expired_token)
-
-      add_database_token(database_id, bearer_token)
-    end
-
     private
 
-    def valid?(token)
-      token.present? && !token.expired?
-    end
-
     def bearer_token(token)
       token.is_a?(BearerToken) ? token : BearerToken.new(token.to_s, raise_on_expired: config.raise_on_expired_token)
     end
diff --git a/spec/scalingo/api/client_spec.rb b/spec/scalingo/api/client_spec.rb
index 2851a6a..38cc9f9 100644
--- a/spec/scalingo/api/client_spec.rb
+++ b/spec/scalingo/api/client_spec.rb
@@ -66,6 +66,13 @@
         subject
       end
     end
+
+    describe "region" do
+      it "keeps track of the region if supplied" do
+        instance = described_class.new(:url, region: "my-region")
+        expect(instance.region).to eq("my-region")
+      end
+    end
   end
 
   describe "self.register_handler(s)!" do
@@ -179,53 +186,6 @@
     end
   end
 
-  describe "database_connection" do
-    let(:database_id) { "db-id-1234" }
-
-    context "without bearer token" do
-      let(:bearer_token) { nil }
-
-      it "raises" do
-        expect {
-          subject.database_connection(database_id)
-        }.to raise_error(Scalingo::Error::Unauthenticated)
-      end
-    end
-
-    context "with bearer token" do
-      before { stub_request(:any, "localhost") }
-
-      it "has an authentication header set with a bearer scheme" do
-        scalingo.authenticate_database_with_bearer_token(
-          database_id,
-          "1234",
-          expires_at: Time.now + 1.hour,
-          raise_on_expired_token: false
-        )
-
-        request_headers = subject.database_connection(database_id).get("/").env.request_headers
-        expected = "Bearer #{subject.token_holder.database_tokens[database_id].value}"
-
-        expect(request_headers["Authorization"]).to eq(expected)
-      end
-    end
-
-    context "with wrong bearer token" do
-      it "raises" do
-        database_id_2 = "db-id-5678"
-        scalingo.authenticate_database_with_bearer_token(
-          database_id_2,
-          "1234",
-          expires_at: Time.now + 1.hour,
-          raise_on_expired_token: false
-        )
-        expect {
-          subject.database_connection(database_id)
-        }.to raise_error(Scalingo::Error::Unauthenticated)
-      end
-    end
-  end
-
   describe "connection" do
     context "when logged" do
       context "without fallback to guest" do
diff --git a/spec/scalingo/core_client_spec.rb b/spec/scalingo/core_client_spec.rb
deleted file mode 100644
index 818a18d..0000000
--- a/spec/scalingo/core_client_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-require "spec_helper"
-
-RSpec.describe Scalingo::CoreClient do
-  subject { described_class.new }
-
-  describe "#database_region" do
-    it "forwards call to the specified region" do
-      allow(subject).to receive("db_api_osc_secnum_fr1")
-
-      subject.database_region("db_api_osc_secnum_fr1")
-
-      expect(subject).to have_received("db_api_osc_secnum_fr1")
-    end
-
-    it "forwards call to default db_api region" do
-      allow(subject).to receive("db_api_#{subject.config.default_region}")
-
-      subject.database_region
-
-      expect(subject).to have_received("db_api_#{subject.config.default_region}")
-    end
-  end
-end
diff --git a/spec/scalingo/regional_database/backups_spec.rb b/spec/scalingo/database/backups_spec.rb
similarity index 87%
rename from spec/scalingo/regional_database/backups_spec.rb
rename to spec/scalingo/database/backups_spec.rb
index 2964951..8fc2b2e 100644
--- a/spec/scalingo/regional_database/backups_spec.rb
+++ b/spec/scalingo/database/backups_spec.rb
@@ -1,12 +1,8 @@
 require "spec_helper"
 
-RSpec.describe Scalingo::RegionalDatabase::Backups, type: :endpoint do
+RSpec.describe Scalingo::Database::Backups, type: :endpoint do
   let(:addon_id) { "the-addon-id" }
 
-  before do
-    scalingo_client.add_database_token(addon_id, "the-bearer-token")
-  end
-
   describe "list" do
     subject(:response) { instance.list(**arguments) }
 
diff --git a/spec/scalingo/regional_database/databases_spec.rb b/spec/scalingo/database/databases_spec.rb
similarity index 81%
rename from spec/scalingo/regional_database/databases_spec.rb
rename to spec/scalingo/database/databases_spec.rb
index 283d35b..799a668 100644
--- a/spec/scalingo/regional_database/databases_spec.rb
+++ b/spec/scalingo/database/databases_spec.rb
@@ -1,12 +1,8 @@
 require "spec_helper"
 
-RSpec.describe Scalingo::RegionalDatabase::Databases, type: :endpoint do
+RSpec.describe Scalingo::Database::Databases, type: :endpoint do
   let(:id) { "database-id" }
 
-  before do
-    scalingo_client.add_database_token(id, "the-bearer-token")
-  end
-
   describe "find" do
     subject(:response) { instance.find(**arguments) }
 
diff --git a/spec/scalingo/regional_database_spec.rb b/spec/scalingo/database_spec.rb
similarity index 83%
rename from spec/scalingo/regional_database_spec.rb
rename to spec/scalingo/database_spec.rb
index 2528f8d..c9d224a 100644
--- a/spec/scalingo/regional_database_spec.rb
+++ b/spec/scalingo/database_spec.rb
@@ -1,6 +1,6 @@
 require "spec_helper"
 
-RSpec.describe Scalingo::RegionalDatabase do
+RSpec.describe Scalingo::Database do
   subject { described_class.new("url") }
 
   %w[databases backups].each do |section|
diff --git a/spec/scalingo/regional/addons_spec.rb b/spec/scalingo/regional/addons_spec.rb
index b78ad36..3f329c4 100644
--- a/spec/scalingo/regional/addons_spec.rb
+++ b/spec/scalingo/regional/addons_spec.rb
@@ -81,6 +81,38 @@
     include_examples "requires some params", :app_id, :id
 
     it { is_expected.to have_requested(:post, api_path.merge("/apps/my-app-id/addons/addon-id/token")) }
+
+    describe "database_client_for" do
+      subject(:db_client) { instance.database_client_for(**params) }
+
+      before do
+        allow(Scalingo::Client::URLS).to receive(:fetch).with(:database).and_return({some_region: "http://localhost"})
+      end
+
+      context "with a valid response" do
+        before do
+          stub_request(:post, "http://localhost/apps/my-app-id/addons/addon-id/token").to_return(
+            body: {addon: {id: "addon-id", token: "some-token"}}.to_json,
+            status: 200,
+            headers: {content_type: "application/json"}
+          )
+        end
+
+        it "returns a database client" do
+          expect(subject).to be_a(Scalingo::Database)
+          expect(subject).to be_authenticated
+          expect(subject.token.value).to eq "some-token"
+        end
+      end
+
+      context "with invalid params" do
+        before do
+          stub_request(:post, "http://localhost/apps/my-app-id/addons/addon-id/token").to_return(status: 404)
+        end
+
+        it { is_expected.to be_nil }
+      end
+    end
   end
 
   describe "delete" do
diff --git a/spec/scalingo/token_holder_spec.rb b/spec/scalingo/token_holder_spec.rb
index 27924d7..7fbb764 100644
--- a/spec/scalingo/token_holder_spec.rb
+++ b/spec/scalingo/token_holder_spec.rb
@@ -42,40 +42,4 @@
       end
     end
   end
-
-  describe "authenticate_database_with_bearer_token" do
-    subject { token_holder.authenticate_database_with_bearer_token(database_id, token, expires_at: expires_at, raise_on_expired_token: false) }
-
-    let(:token_holder) do
-      holder = token_holder_dummy_class.new
-      holder.config = Scalingo::Configuration.new
-
-      holder
-    end
-
-    let(:database_id) { "db-id-1234" }
-
-    context "without expiration date" do
-      let(:token) { "1234" }
-      let(:expires_at) { nil }
-
-      it "set the database auth token" do
-        expect(token_holder.authenticated_for_database?(database_id)).to be false
-        subject
-        expect(token_holder.authenticated_for_database?(database_id)).to be true
-      end
-    end
-
-    context "with an expiration date" do
-      let(:token) { "1234" }
-      let(:expires_at) { Time.now + 1.hour }
-
-      it "refresh the database token" do
-        token_holder.authenticate_database_with_bearer_token(database_id, token, expires_at: 1.hour.ago, raise_on_expired_token: false)
-        expect(token_holder.authenticated_for_database?(database_id)).to be false
-        subject
-        expect(token_holder.authenticated_for_database?(database_id)).to be true
-      end
-    end
-  end
 end
diff --git a/spec/support/shared.rb b/spec/support/shared.rb
index 3f52955..fdab09c 100644
--- a/spec/support/shared.rb
+++ b/spec/support/shared.rb
@@ -25,7 +25,7 @@
 
   let(:api_path) { URI.parse("http://localhost") }
 
-  let(:api_client) { described_class.module_parent.new(api_path.to_s, scalingo: scalingo_client) }
+  let(:api_client) { described_class.module_parent.new(api_path.to_s, scalingo: scalingo_client, region: :some_region) }
   let(:instance) { described_class.new(api_client) }
 end