diff --git a/Gemfile.lock b/Gemfile.lock index 07f96b2e0..483be0935 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -2,7 +2,6 @@ PATH remote: . specs: shipit-engine (0.33.0) - active_model_serializers (~> 0.9.3) ansi_stream (~> 0.0.6) attr_encrypted (~> 3.1.0) autoprefixer-rails (~> 6.4.1) @@ -15,6 +14,7 @@ PATH lodash-rails (~> 4.6.1) octokit (~> 4.15) omniauth-github (~> 1.4) + panko_serializer (~> 0.7.5) pubsubstub (~> 0.2.0) rails (~> 6.0.0) rails-timeago (~> 2.13.0) @@ -69,9 +69,6 @@ GEM erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) - active_model_serializers (0.9.8) - activemodel (>= 3.2) - concurrent-ruby (~> 1.0) activejob (6.0.3.5) activesupport (= 6.0.3.5) globalid (>= 0.3.6) @@ -185,6 +182,7 @@ GEM octokit (4.20.0) faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) + oj (3.11.2) omniauth (1.9.1) hashie (>= 3.4.6) rack (>= 1.6.2, < 3) @@ -194,6 +192,9 @@ GEM omniauth-oauth2 (1.7.1) oauth2 (~> 1.4) omniauth (>= 1.9, < 3) + panko_serializer (0.7.5) + activesupport + oj (~> 3.11.0) parallel (1.19.2) parser (2.7.2.0) ast (~> 2.4.1) diff --git a/app/controllers/concerns/shipit/active_model_serializers_patch.rb b/app/controllers/concerns/shipit/active_model_serializers_patch.rb deleted file mode 100644 index e4e5c625b..000000000 --- a/app/controllers/concerns/shipit/active_model_serializers_patch.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true -module Shipit - module ActiveModelSerializersPatch - private - - def namespace_for_serializer - # TODO: This is a monkey patch for active_model_serializers 0.9.7. - # It's really outdated and newer versions aren't really a suitable replacement. - # We should look into simply getting rid of it. - @namespace_for_serializer ||= self.class.module_parent unless self.class.module_parent == Object - end - end -end diff --git a/app/controllers/concerns/shipit/api/rendering.rb b/app/controllers/concerns/shipit/api/rendering.rb index 137e0b4d6..c0e303160 100644 --- a/app/controllers/concerns/shipit/api/rendering.rb +++ b/app/controllers/concerns/shipit/api/rendering.rb @@ -5,7 +5,7 @@ module Rendering private def render_resources(resources, options = {}) - options[:json] = resources + options[:json] = Panko::ArraySerializer.new(resources, each_serializer: Serializer.for(resources)).to_json render(options) end @@ -15,7 +15,8 @@ def render_resource(resource, options = {}) elsif resource.errors.any? render(options.reverse_merge(status: :unprocessable_entity, json: { errors: resource.errors })) else - render(options.reverse_merge(json: resource)) + serializer = Serializer.for(resource).new + render(options.reverse_merge(json: serializer.serialize(resource))) end end end diff --git a/app/controllers/shipit/api/hooks_controller.rb b/app/controllers/shipit/api/hooks_controller.rb index 033615052..14bc02e0c 100644 --- a/app/controllers/shipit/api/hooks_controller.rb +++ b/app/controllers/shipit/api/hooks_controller.rb @@ -10,7 +10,7 @@ def index end def show - render(json: hook) + render_resource(hook) end params do diff --git a/app/controllers/shipit/tasks_controller.rb b/app/controllers/shipit/tasks_controller.rb index 0a2923c77..8fed4a029 100644 --- a/app/controllers/shipit/tasks_controller.rb +++ b/app/controllers/shipit/tasks_controller.rb @@ -48,7 +48,7 @@ def abort end def tail - render(json: TailTaskSerializer.new(task, context: { last_byte: params[:last_byte].to_i })) + render(json: TailTaskSerializer.new(context: { last_byte: params[:last_byte].to_i }).serialize(task)) end def lookup diff --git a/app/models/shipit/anonymous_user.rb b/app/models/shipit/anonymous_user.rb index a117224ac..7e39b301b 100644 --- a/app/models/shipit/anonymous_user.rb +++ b/app/models/shipit/anonymous_user.rb @@ -1,6 +1,12 @@ # frozen_string_literal: true module Shipit class AnonymousUser + class << self + def attribute_aliases + {}.freeze + end + end + def present? false end diff --git a/app/models/shipit/hook.rb b/app/models/shipit/hook.rb index 3fd2aae68..9b188282d 100644 --- a/app/models/shipit/hook.rb +++ b/app/models/shipit/hook.rb @@ -101,8 +101,8 @@ def listening_event(event) def coerce_payload(payload) coerced_payload = payload.dup payload.each do |key, value| - if serializer = ActiveModel::Serializer.serializer_for(value) - coerced_payload[key] = serializer.new(value) + if serializer = Serializer.for(value) + coerced_payload[key] = serializer.new.serialize(value) end end coerced_payload.to_json diff --git a/app/serializers/concerns/shipit/conditional_attributes.rb b/app/serializers/concerns/shipit/conditional_attributes.rb deleted file mode 100644 index 588f56fbf..000000000 --- a/app/serializers/concerns/shipit/conditional_attributes.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true -module Shipit - module ConditionalAttributes - extend ActiveSupport::Concern - - module ClassMethods - def inclusion_method_for(attribute) - @inclusion_cache ||= {} - @inclusion_cache.fetch(attribute) do - method = "include_#{attribute}?" - method_defined?(method) && method - end - end - end - - def filter(*) - super.reject do |attribute| - method = self.class.inclusion_method_for(attribute) - method && !send(method) - end - end - end -end diff --git a/app/serializers/shipit/commit_serializer.rb b/app/serializers/shipit/commit_serializer.rb index e215720a5..f620de05b 100644 --- a/app/serializers/shipit/commit_serializer.rb +++ b/app/serializers/shipit/commit_serializer.rb @@ -2,16 +2,13 @@ module Shipit class CommitSerializer < ShortCommitSerializer include GithubUrlHelper - include ConditionalAttributes - has_one :author - has_one :committer + has_one :author, serializer: UserSerializer + has_one :committer, serializer: UserSerializer attributes :additions, :deletions, :authored_at, :committed_at, :html_url, :pull_request, :status, :deployed - def deployed - object.deployed? - end + aliases deployed?: :deployed def status object.status.state @@ -22,14 +19,14 @@ def html_url end def pull_request - { - number: object.pull_request_number, - html_url: github_pull_request_url(object), - } - end - - def include_pull_request? - object.pull_request? + if object.pull_request? + { + number: object.pull_request_number, + html_url: github_pull_request_url(object), + } + else + SKIP + end end end end diff --git a/app/serializers/shipit/deploy_serializer.rb b/app/serializers/shipit/deploy_serializer.rb index 7b13376e4..77bda39e8 100644 --- a/app/serializers/shipit/deploy_serializer.rb +++ b/app/serializers/shipit/deploy_serializer.rb @@ -3,9 +3,10 @@ module Shipit class DeploySerializer < TaskSerializer include GithubUrlHelper - has_many :commits + has_many :commits, serializer: CommitSerializer + has_one :rollback_once_aborted_to, serializer: DeploySerializer - attributes :compare_url, :rollback_url, :additions, :deletions, :rollback_once_aborted_to + attributes :compare_url, :rollback_url, :additions, :deletions def html_url stack_deploy_url(object.stack, object) @@ -22,11 +23,5 @@ def rollback_url def type :deploy end - - def rollback_once_aborted_to - return nil unless object.rollback_once_aborted_to - - DeploySerializer.new(object.rollback_once_aborted_to) - end end end diff --git a/app/serializers/shipit/hook_serializer.rb b/app/serializers/shipit/hook_serializer.rb index e81362151..e09900ef0 100644 --- a/app/serializers/shipit/hook_serializer.rb +++ b/app/serializers/shipit/hook_serializer.rb @@ -1,10 +1,11 @@ # frozen_string_literal: true module Shipit - class HookSerializer < ActiveModel::Serializer - include ConditionalAttributes + class HookSerializer < Serializer + attributes :id, :stack, :url, :delivery_url, :content_type, :events, :insecure_ssl, :created_at, :updated_at - has_one :stack - attributes :id, :url, :delivery_url, :content_type, :events, :insecure_ssl, :created_at, :updated_at + def stack + object.stack || SKIP + end def url if object.scoped? @@ -13,9 +14,5 @@ def url api_hook_url(object) end end - - def include_stack? - object.scoped? - end end end diff --git a/app/serializers/shipit/merge_request_serializer.rb b/app/serializers/shipit/merge_request_serializer.rb index 6a0937b69..8860213f9 100644 --- a/app/serializers/shipit/merge_request_serializer.rb +++ b/app/serializers/shipit/merge_request_serializer.rb @@ -1,10 +1,9 @@ # frozen_string_literal: true module Shipit - class MergeRequestSerializer < ActiveModel::Serializer + class MergeRequestSerializer < Serializer include GithubUrlHelper - include ConditionalAttributes - has_one :merge_requested_by + has_one :merge_requested_by, serializer: UserSerializer has_one :head, serializer: ShortCommitSerializer attributes :id, :number, :title, :github_id, :additions, :deletions, :state, :merge_status, :mergeable, @@ -14,8 +13,12 @@ def html_url github_pull_request_url(object) end - def include_rejection_reason? - object.rejection_reason? + def rejection_reason + if object.rejection_reason? + object.rejection_reason + else + SKIP + end end end end diff --git a/app/serializers/shipit/pull_request_serializer.rb b/app/serializers/shipit/pull_request_serializer.rb index f21653543..c73924081 100644 --- a/app/serializers/shipit/pull_request_serializer.rb +++ b/app/serializers/shipit/pull_request_serializer.rb @@ -1,18 +1,21 @@ # frozen_string_literal: true module Shipit - class PullRequestSerializer < ActiveModel::Serializer + class PullRequestSerializer < Serializer include GithubUrlHelper - include ConditionalAttributes - has_one :user + has_one :user, serializer: UserSerializer has_one :head, serializer: ShortCommitSerializer has_many :assignees, serializer: UserSerializer attributes :id, :number, :title, :github_id, :additions, :deletions, :state, :html_url def html_url - github_pull_request_url(object) if object.stack.present? + if object.stack.present? + github_pull_request_url(object) + else + SKIP + end end end end diff --git a/app/serializers/shipit/review_stack_serializer.rb b/app/serializers/shipit/review_stack_serializer.rb index fa12f8d9b..f95898cd2 100644 --- a/app/serializers/shipit/review_stack_serializer.rb +++ b/app/serializers/shipit/review_stack_serializer.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Shipit - class ReviewStackSerializer < Shipit::StackSerializer - has_one :pull_request + class ReviewStackSerializer < StackSerializer + has_one :pull_request, serializer: PullRequestSerializer end end diff --git a/app/serializers/shipit/rollback_serializer.rb b/app/serializers/shipit/rollback_serializer.rb index 5efddf1d0..25956c738 100644 --- a/app/serializers/shipit/rollback_serializer.rb +++ b/app/serializers/shipit/rollback_serializer.rb @@ -5,8 +5,8 @@ def type :rollback end - def include_rollback_url? - false + def rollback_url + SKIP end end end diff --git a/app/serializers/shipit/serializer.rb b/app/serializers/shipit/serializer.rb new file mode 100644 index 000000000..d92a09586 --- /dev/null +++ b/app/serializers/shipit/serializer.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true +module Shipit + class Serializer < Panko::Serializer + include Engine.routes.url_helpers + class << self + def for(object) + if object.nil? + self + elsif object.is_a?(Array) + self.for(object.first) + else + "#{object.class.name}Serializer".safe_constantize + end + end + + def build(object) + self.for(object).new.serialize(object) + end + end + end +end diff --git a/app/serializers/shipit/short_commit_serializer.rb b/app/serializers/shipit/short_commit_serializer.rb index 6f70609fa..0c9c7372a 100644 --- a/app/serializers/shipit/short_commit_serializer.rb +++ b/app/serializers/shipit/short_commit_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module Shipit - class ShortCommitSerializer < ActiveModel::Serializer + class ShortCommitSerializer < Serializer attributes :sha, :message def message diff --git a/app/serializers/shipit/stack_serializer.rb b/app/serializers/shipit/stack_serializer.rb index 0ac46bd56..dd746818a 100644 --- a/app/serializers/shipit/stack_serializer.rb +++ b/app/serializers/shipit/stack_serializer.rb @@ -1,14 +1,10 @@ # frozen_string_literal: true module Shipit - class StackSerializer < ActiveModel::Serializer - include ConditionalAttributes - - has_one :lock_author - + class StackSerializer < Serializer attributes :id, :repo_owner, :repo_name, :environment, :html_url, :url, :tasks_url, :deploy_url, - :merge_requests_url, :deploy_spec, :undeployed_commits_count, :is_locked, :lock_reason, :continuous_deployment, - :created_at, :updated_at, :locked_since, :last_deployed_at, :branch, :merge_queue_enabled, :is_archived, - :archived_since + :merge_requests_url, :deploy_spec, :undeployed_commits_count, :is_locked, :lock_reason, :lock_author, + :continuous_deployment, :created_at, :updated_at, :locked_since, :last_deployed_at, :branch, + :merge_queue_enabled, :is_archived, :archived_since def url api_stack_url(object) @@ -26,24 +22,36 @@ def merge_requests_url api_stack_merge_requests_url(object) end - def is_locked - object.locked? + def is_archived + object.archived? end - def include_lock_reason? + def is_locked object.locked? end - def include_lock_author? - object.locked? + def lock_reason + if object.locked? + object.lock_reason + else + SKIP + end end - def include_locked_since? - object.locked? + def lock_author + if object.locked? + Serializer.build(object.lock_author) + else + SKIP + end end - def is_archived - object.archived? + def locked_since + if object.locked? + object.locked_since + else + SKIP + end end def deploy_spec diff --git a/app/serializers/shipit/tail_task_serializer.rb b/app/serializers/shipit/tail_task_serializer.rb index 0a98a8474..0384257c0 100644 --- a/app/serializers/shipit/tail_task_serializer.rb +++ b/app/serializers/shipit/tail_task_serializer.rb @@ -1,14 +1,13 @@ # frozen_string_literal: true module Shipit - class TailTaskSerializer < ActiveModel::Serializer + class TailTaskSerializer < Serializer include ChunksHelper - include ConditionalAttributes attributes :url, :status, :output, :rollback_url def url return @url if defined? @url - @url = next_chunks_url(task, last_byte: next_offset) + @url = next_chunks_url(task, last_byte: next_offset) || SKIP end def include_url? @@ -20,11 +19,11 @@ def output end def rollback_url - stack_deploy_path(stack, rollback) - end - - def include_rollback_url? - !rollback.nil? + if rollback.nil? + SKIP + else + stack_deploy_path(stack, rollback) + end end private diff --git a/app/serializers/shipit/task_serializer.rb b/app/serializers/shipit/task_serializer.rb index 792936b47..e3a091852 100644 --- a/app/serializers/shipit/task_serializer.rb +++ b/app/serializers/shipit/task_serializer.rb @@ -1,17 +1,11 @@ # frozen_string_literal: true module Shipit - class TaskSerializer < ActiveModel::Serializer - include ConditionalAttributes - - has_one :author - has_one :revision, serializer: ShortCommitSerializer + class TaskSerializer < Serializer + has_one :author, serializer: UserSerializer + has_one :until_commit, serializer: ShortCommitSerializer, name: :revision attributes(:id, :url, :html_url, :output_url, :type, :status, :action, :title, :description, :started_at, :ended_at, :updated_at, :created_at, :env, :ignored_safeties, :max_retries, :retry_attempt) - def revision - object.until_commit - end - def url api_stack_task_url(object.stack, object) end @@ -29,19 +23,19 @@ def type end def action - object.definition&.action - end - - def include_action? - type == :task + if type == :task + object.definition&.action + else + SKIP + end end def description - object.definition&.action - end - - def include_description? - type == :task + if type == :task + object.definition&.action + else + SKIP + end end end end diff --git a/app/serializers/shipit/user_serializer.rb b/app/serializers/shipit/user_serializer.rb index a18112fd8..f8d6446d4 100644 --- a/app/serializers/shipit/user_serializer.rb +++ b/app/serializers/shipit/user_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module Shipit - class UserSerializer < ActiveModel::Serializer + class UserSerializer < Serializer attributes :id, :name, :email, :login, :avatar_url, :created_at, :updated_at, :github_id, :anonymous def anonymous diff --git a/lib/shipit.rb b/lib/shipit.rb index aca1f0808..cd14f7112 100644 --- a/lib/shipit.rb +++ b/lib/shipit.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true require 'active_support/all' -require 'active_model_serializers' +require 'panko_serializer' require 'state_machines-activerecord' require 'validate_url' require 'responders' diff --git a/lib/shipit/engine.rb b/lib/shipit/engine.rb index d213e41e9..348fa8370 100644 --- a/lib/shipit/engine.rb +++ b/lib/shipit/engine.rb @@ -28,10 +28,6 @@ class Engine < ::Rails::Engine ActionDispatch::ExceptionWrapper.rescue_responses[Shipit::TaskDefinition::NotFound.name] = :not_found - ActiveModel::Serializer._root = false - ActiveModel::ArraySerializer._root = false - ActiveModel::Serializer.include(Engine.routes.url_helpers) - if Shipit.github.oauth? OmniAuth::Strategies::GitHub.configure(path_prefix: '/github/auth') app.middleware.use(OmniAuth::Builder) do @@ -42,10 +38,6 @@ class Engine < ::Rails::Engine if Shipit.enable_samesite_middleware? app.config.middleware.insert_after(::Rack::Runtime, Shipit::SameSiteCookieMiddleware) end - - app.config.after_initialize do - ActionController::Base.include(Shipit::ActiveModelSerializersPatch) - end end end end diff --git a/shipit-engine.gemspec b/shipit-engine.gemspec index 802df0ee4..2cad48891 100644 --- a/shipit-engine.gemspec +++ b/shipit-engine.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |s| s.files = Dir["{app,config,db,lib,vendor}/**/*", "LICENSE", "Rakefile", "README.md"] s.test_files = Dir["test/**/*"] - Dir["test/dummy/tmp/**/*"] - Dir["test/dummy/log/**/*"] - s.add_dependency('active_model_serializers', '~> 0.9.3') + s.add_dependency('panko_serializer', '~> 0.7.5') s.add_dependency('ansi_stream', '~> 0.0.6') s.add_dependency('attr_encrypted', '~> 3.1.0') s.add_dependency('autoprefixer-rails', '~> 6.4.1') diff --git a/test/serializers/shipit/pull_request_serializer_test.rb b/test/serializers/shipit/pull_request_serializer_test.rb index a85d04931..e48edc5bf 100644 --- a/test/serializers/shipit/pull_request_serializer_test.rb +++ b/test/serializers/shipit/pull_request_serializer_test.rb @@ -3,23 +3,23 @@ require "test_helper" module Shipit - class PulLRequestSerializerTest < ActiveSupport::TestCase + class PullRequestSerializerTest < ActiveSupport::TestCase test "structure" do pull_request = shipit_pull_requests(:review_stack_review) - serialized = serializer.new(pull_request).as_json + serialized = serializer.new.serialize(pull_request).as_json - assert_includes serialized.keys, :id - assert_includes serialized.keys, :number - assert_includes serialized.keys, :title - assert_includes serialized.keys, :github_id - assert_includes serialized.keys, :additions - assert_includes serialized.keys, :deletions - assert_includes serialized.keys, :state - assert_includes serialized.keys, :html_url - assert_includes serialized.keys, :user - assert_includes serialized.keys, :assignees - assert_includes serialized.keys, :head + assert_includes serialized.keys, 'id' + assert_includes serialized.keys, 'number' + assert_includes serialized.keys, 'title' + assert_includes serialized.keys, 'github_id' + assert_includes serialized.keys, 'additions' + assert_includes serialized.keys, 'deletions' + assert_includes serialized.keys, 'state' + assert_includes serialized.keys, 'html_url' + assert_includes serialized.keys, 'user' + assert_includes serialized.keys, 'assignees' + assert_includes serialized.keys, 'head' end def serializer diff --git a/test/unit/anonymous_user_serializer_test.rb b/test/unit/anonymous_user_serializer_test.rb index b0dbb650c..a68a279e7 100644 --- a/test/unit/anonymous_user_serializer_test.rb +++ b/test/unit/anonymous_user_serializer_test.rb @@ -5,9 +5,9 @@ module Shipit class AnonymousUserSerializerTest < ActiveSupport::TestCase test 'sets anonymous to true' do user = AnonymousUser.new - serializer = ActiveModel::Serializer.serializer_for(user) + serializer = Serializer.for(user) assert_equal AnonymousUserSerializer, serializer - serialized = serializer.new(user).to_json + serialized = serializer.new.serialize(user).to_json assert_json("anonymous", true, document: serialized) end end diff --git a/test/unit/commit_serializer_test.rb b/test/unit/commit_serializer_test.rb index 5b3c82260..fe15540fc 100644 --- a/test/unit/commit_serializer_test.rb +++ b/test/unit/commit_serializer_test.rb @@ -6,9 +6,9 @@ class CommitSerializerTest < ActiveSupport::TestCase test 'commit includes author object' do commit = shipit_commits(:first) - serializer = ActiveModel::Serializer.serializer_for(commit) + serializer = Serializer.for(commit) assert_equal CommitSerializer, serializer - serialized = serializer.new(commit).to_json + serialized = serializer.new.serialize(commit).to_json assert_json("author.name", commit.author.name, document: serialized) end diff --git a/test/unit/deploy_serializer_test.rb b/test/unit/deploy_serializer_test.rb index d15be9fa2..a55bdf368 100644 --- a/test/unit/deploy_serializer_test.rb +++ b/test/unit/deploy_serializer_test.rb @@ -7,9 +7,9 @@ class DeploySerializerTest < ActiveSupport::TestCase deploy = shipit_deploys(:shipit) first_commit_author = deploy.commits.first.author - serializer = ActiveModel::Serializer.serializer_for(deploy) + serializer = Serializer.for(deploy) assert_equal DeploySerializer, serializer - serialized = serializer.new(deploy).to_json + serialized = serializer.new.serialize(deploy).to_json assert_json("commits.0.author.name", first_commit_author.name, document: serialized) end diff --git a/test/unit/user_serializer_test.rb b/test/unit/user_serializer_test.rb index 3dc95472b..fe988454e 100644 --- a/test/unit/user_serializer_test.rb +++ b/test/unit/user_serializer_test.rb @@ -5,9 +5,9 @@ module Shipit class UserSerializerTest < ActiveSupport::TestCase test 'includes anonymous key' do user = User.new - serializer = ActiveModel::Serializer.serializer_for(user) + serializer = Serializer.for(user) assert_equal UserSerializer, serializer - serialized = serializer.new(user).to_json + serialized = serializer.new.serialize(user).to_json assert_json("anonymous", false, document: serialized) end end