Merge branch '1541-deal-with-repo-size-limit-as-byte' into 'master'

Deal with repository size limits as Bytes instead Megabytes

Closes #1541

See merge request !1067

app/controllers/admin/application_settings_controller.rb

@@ -5,7 +5,9 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   end
 
   def update
-    if @application_setting.update_attributes(application_setting_params)
+    result = ::ApplicationSettings::UpdateService.new(@application_setting, current_user, application_setting_params).execute
+
+    if result[:status] == :success
       redirect_to admin_application_settings_path,
         notice: 'Application settings saved successfully'
     else

app/controllers/concerns/lfs_request.rb

@@ -136,7 +136,7 @@ module LfsRequest
 
       size_of_objects = objects.sum { |o| o[:size] }
 
-      @limit_exceeded = (project.repository_and_lfs_size + size_of_objects.to_mb) > project.actual_size_limit
+      @limit_exceeded = (project.repository_and_lfs_size + size_of_objects) > project.actual_size_limit
     end
   end
 

app/models/project.rb

@@ -1541,8 +1541,10 @@ class Project < ActiveRecord::Base
     actual_size_limit != 0
   end
 
-  def changes_will_exceed_size_limit?(size_mb)
-    size_limit_enabled? && (size_mb > actual_size_limit || size_mb + repository_and_lfs_size > actual_size_limit)
+  def changes_will_exceed_size_limit?(size_in_bytes)
+    size_limit_enabled? &&
+      (size_in_bytes > actual_size_limit ||
+       size_in_bytes + repository_and_lfs_size > actual_size_limit)
   end
 
   def environments_for(ref, commit: nil, with_tags: false)

app/services/application_settings/base_service.rb

+module ApplicationSettings
+  class BaseService < ::BaseService
+    attr_accessor :application_setting, :current_user, :params
+
+    def initialize(application_setting, user, params = {})
+      @application_setting, @current_user, @params = application_setting, user, params.dup
+    end
+  end
+end

app/services/application_settings/update_service.rb

+module ApplicationSettings
+  class UpdateService < ApplicationSettings::BaseService
+    def execute
+      # Repository size limit comes as MB from the view
+      assign_repository_size_limit_as_bytes(application_setting)
+
+      if application_setting.update(params)
+        success
+      else
+        error('Application settings could not be updated')
+      end
+    end
+  end
+end

app/services/base_service.rb

@@ -46,6 +46,13 @@ class BaseService
 
   private
 
+  def assign_repository_size_limit_as_bytes(model)
+    repository_size_limit = @params.delete(:repository_size_limit)
+    new_value = repository_size_limit.to_i.megabytes if repository_size_limit.present?
+
+    model.repository_size_limit = new_value
+  end
+
   def error(message, http_status = nil)
     result = {
       message: message,

app/services/groups/create_service.rb

@@ -12,6 +12,9 @@ module Groups
         return @group
       end
 
+      # Repository size limit comes as MB from the view
+      assign_repository_size_limit_as_bytes(@group)
+
       if @group.parent && !can?(current_user, :admin_group, @group.parent)
         @group.parent = nil
         @group.errors.add(:parent_id, 'manage access required to create subgroup')

app/services/groups/update_service.rb

@@ -12,6 +12,9 @@ module Groups
         end
       end
 
+      # Repository size limit comes as MB from the view
+      assign_repository_size_limit_as_bytes(group)
+
       group.assign_attributes(params)
 
       begin

app/services/projects/create_service.rb

@@ -22,6 +22,9 @@ module Projects
         return @project
       end
 
+      # Repository size limit comes as MB from the view
+      assign_repository_size_limit_as_bytes(@project)
+
       # Set project name from path
       if @project.name.present? && @project.path.present?
         # if both name and path set - everything is ok

app/services/projects/update_service.rb

@@ -13,6 +13,9 @@ module Projects
         end
       end
 
+      # Repository size limit comes as MB from the view
+      assign_repository_size_limit_as_bytes(project)
+
       new_branch = params.delete(:default_branch)
       new_repository_storage = params.delete(:repository_storage)
 

app/views/admin/application_settings/_form.html.haml

@@ -67,7 +67,7 @@
       = f.label :repository_size_limit, class: 'control-label col-sm-2' do
         Size limit per repository (MB)
       .col-sm-10
-        = f.number_field :repository_size_limit, class: 'form-control', min: 0
+        = f.number_field :repository_size_limit, value: f.object.repository_size_limit.try(:to_mb), class: 'form-control', min: 0
         %span.help-block#repository_size_limit_help_block
           Includes LFS objects. It can be overridden per group, or per project. 0 for unlimited.
           = link_to icon('question-circle'), help_page_path("user/admin_area/settings/account_and_limit_settings")

app/views/groups/_repository_size_limit_setting.html.haml

@@ -3,6 +3,6 @@
     = f.label :repository_size_limit, class: 'control-label' do
       Repository size limit (MB)
     .col-sm-10
-      = f.number_field :repository_size_limit, class: 'form-control', min: 0
+      = f.number_field :repository_size_limit, value: f.object.repository_size_limit.try(:to_mb), class: 'form-control', min: 0
       %span.help-block#repository_size_limit_help_block
         = size_limit_message_for_group(@group)

app/views/projects/edit.html.haml

@@ -38,7 +38,7 @@
           .form-group
             = f.label :repository_size_limit, class: 'label-light' do
               Repository size limit (MB)
-            = f.number_field :repository_size_limit, class: 'form-control', min: 0
+            = f.number_field :repository_size_limit, value: f.object.repository_size_limit.try(:to_mb), class: 'form-control', min: 0
             %span.help-block#repository_size_limit_help_block
               = size_limit_message(@project)
 

db/migrate/20170118194941_convert_application_settings_repository_size_limit_to_bytes.rb

+class ConvertApplicationSettingsRepositorySizeLimitToBytes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    connection.transaction do
+      rename_column :application_settings, :repository_size_limit, :repository_size_limit_mb
+      add_column :application_settings, :repository_size_limit, :integer, default: 0, limit: 8
+    end
+
+    bigint_string = if Gitlab::Database.postgresql?
+                      'repository_size_limit_mb::bigint * 1024 * 1024'
+                    else
+                      'repository_size_limit_mb * 1024 * 1024'
+                    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new(bigint_string)
+
+    connection.transaction do
+      update_column_in_batches(:application_settings, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_mb].not_eq(nil))
+      end
+
+      remove_column :application_settings, :repository_size_limit_mb
+    end
+  end
+
+  def down
+    connection.transaction do
+      rename_column :application_settings, :repository_size_limit, :repository_size_limit_bytes
+      add_column :application_settings, :repository_size_limit, :integer, default: 0, limit: nil
+    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new('repository_size_limit_bytes / 1024 / 1024')
+
+    connection.transaction do
+      update_column_in_batches(:application_settings, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_bytes].not_eq(nil))
+      end
+
+      remove_column :application_settings, :repository_size_limit_bytes
+    end
+  end
+end

db/migrate/20170118200338_convert_projects_repository_size_limit_to_bytes.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ConvertProjectsRepositorySizeLimitToBytes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    connection.transaction do
+      rename_column :projects, :repository_size_limit, :repository_size_limit_mb
+      add_column :projects, :repository_size_limit, :integer, limit: 8
+    end
+
+    bigint_string = if Gitlab::Database.postgresql?
+                      'repository_size_limit_mb::bigint * 1024 * 1024'
+                    else
+                      'repository_size_limit_mb * 1024 * 1024'
+                    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new(bigint_string)
+
+    connection.transaction do
+      update_column_in_batches(:projects, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_mb].not_eq(nil))
+      end
+
+      remove_column :projects, :repository_size_limit_mb
+    end
+  end
+
+  def down
+    connection.transaction do
+      rename_column :projects, :repository_size_limit, :repository_size_limit_bytes
+      add_column :projects, :repository_size_limit, :integer, limit: nil
+    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new('repository_size_limit_bytes / 1024 / 1024')
+
+    connection.transaction do
+      update_column_in_batches(:projects, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_bytes].not_eq(nil))
+      end
+
+      remove_column :projects, :repository_size_limit_bytes
+    end
+  end
+end

db/migrate/20170118200412_convert_namespaces_repository_size_limit_to_bytes.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ConvertNamespacesRepositorySizeLimitToBytes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    connection.transaction do
+      rename_column :namespaces, :repository_size_limit, :repository_size_limit_mb
+      add_column :namespaces, :repository_size_limit, :integer, limit: 8
+    end
+
+    bigint_string = if Gitlab::Database.postgresql?
+                      'repository_size_limit_mb::bigint * 1024 * 1024'
+                    else
+                      'repository_size_limit_mb * 1024 * 1024'
+                    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new(bigint_string)
+
+    connection.transaction do
+      update_column_in_batches(:namespaces, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_mb].not_eq(nil))
+      end
+
+      remove_column :namespaces, :repository_size_limit_mb
+    end
+  end
+
+  def down
+    connection.transaction do
+      rename_column :namespaces, :repository_size_limit, :repository_size_limit_bytes
+      add_column :namespaces, :repository_size_limit, :integer, limit: nil
+    end
+
+    sql_expression = Arel::Nodes::SqlLiteral.new('repository_size_limit_bytes / 1024 / 1024')
+
+    connection.transaction do
+      update_column_in_batches(:namespaces, :repository_size_limit, sql_expression) do |t, query|
+        query.where(t[:repository_size_limit_bytes].not_eq(nil))
+      end
+
+      remove_column :namespaces, :repository_size_limit_bytes
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170106172237) do
+ActiveRecord::Schema.define(version: 20170118200412) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -102,7 +102,6 @@ ActiveRecord::Schema.define(version: 20170106172237) do
     t.boolean "usage_ping_enabled", default: true, null: false
     t.boolean "koding_enabled"
     t.string "koding_url"
-    t.integer "repository_size_limit", default: 0
     t.text "sign_in_text_html"
     t.text "help_page_text_html"
     t.text "shared_runners_text_html"
@@ -119,6 +118,7 @@ ActiveRecord::Schema.define(version: 20170106172237) do
     t.string "plantuml_url"
     t.boolean "plantuml_enabled"
     t.integer "shared_runners_minutes", default: 0, null: false
+    t.integer "repository_size_limit", limit: 8, default: 0
   end
 
   create_table "approvals", force: :cascade do |t|
@@ -853,10 +853,10 @@ ActiveRecord::Schema.define(version: 20170106172237) do
     t.datetime "ldap_sync_last_sync_at"
     t.datetime "deleted_at"
     t.boolean "lfs_enabled"
-    t.integer "repository_size_limit"
     t.text "description_html"
     t.integer "parent_id"
     t.integer "shared_runners_minutes_limit"
+    t.integer "repository_size_limit", limit: 8
   end
 
   add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
@@ -1105,8 +1105,8 @@ ActiveRecord::Schema.define(version: 20170106172237) do
     t.boolean "repository_read_only"
     t.boolean "lfs_enabled"
     t.text "description_html"
-    t.integer "repository_size_limit"
     t.boolean "only_allow_merge_if_all_discussions_are_resolved"
+    t.integer "repository_size_limit", limit: 8
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree

lib/gitlab/git_access.rb

@@ -185,7 +185,7 @@ module Gitlab
         end
       end
 
-      if project.changes_will_exceed_size_limit?(push_size_in_bytes.to_mb)
+      if project.changes_will_exceed_size_limit?(push_size_in_bytes)
         raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).new_changes_error
       end
     end

lib/gitlab/repository_size_error.rb

@@ -55,7 +55,7 @@ module Gitlab
     end
 
     def format_number(number)
-      number_to_human_size(number * 1.megabyte, delimiter: ',', precision: 2)
+      number_to_human_size(number, delimiter: ',', precision: 2)
     end
   end
 end

spec/controllers/admin/application_settings_controller_spec.rb

@@ -6,6 +6,36 @@ describe Admin::ApplicationSettingsController do
   let(:admin) { create(:admin) }
   let(:user) { create(:user)}
 
+  describe 'PUT #update' do
+    before do
+      sign_in(admin)
+    end
+
+    context 'with valid params' do
+      subject { put :update, application_setting: { repository_size_limit: '100' } }
+
+      it 'redirect to application settings page' do
+        is_expected.to redirect_to(admin_application_settings_path)
+      end
+
+      it 'set flash notice' do
+        is_expected.to set_flash[:notice].to('Application settings saved successfully')
+      end
+    end
+
+    context 'with invalid params' do
+      subject! { put :update, application_setting: { repository_size_limit: '-100' } }
+
+      it 'render show template' do
+        is_expected.to render_template(:show)
+      end
+
+      it 'assigned @application_settings has errors' do
+        expect(assigns(:application_setting).errors[:repository_size_limit]).to be_present
+      end
+    end
+  end
+
   describe 'GET #usage_data with no access' do
     before do
       sign_in(user)

spec/lib/gitlab/git_access_spec.rb

@@ -751,7 +751,7 @@ describe Gitlab::GitAccess, lib: true do
 
       describe 'repository size restrictions' do
         before do
-          project.update_attribute(:repository_size_limit, 50)
+          project.update_attribute(:repository_size_limit, 50.megabytes)
         end
 
         it 'returns false when blob is too big' do

spec/lib/gitlab/repository_size_error_spec.rb

@@ -2,14 +2,14 @@ require 'spec_helper'
 
 describe Gitlab::RepositorySizeError, lib: true do
   let(:project) do
-    create(:empty_project, statistics: build(:project_statistics, repository_size: 15))
+    create(:empty_project, statistics: build(:project_statistics, repository_size: 15.megabytes))
   end
 
   let(:message) { Gitlab::RepositorySizeError.new(project) }
   let(:base_message) { 'because this repository has exceeded its size limit of 10 MB by 5 MB' }
 
   before do
-    allow(project).to receive(:actual_size_limit).and_return(10)
+    allow(project).to receive(:actual_size_limit).and_return(10.megabytes)
   end
 
   describe 'error messages' do

spec/models/application_setting_spec.rb

@@ -176,4 +176,14 @@ describe ApplicationSetting, models: true do
       expect(setting.domain_blacklist).to contain_exactly('example.com', 'test.com', 'foo.bar')
     end
   end
+
+  describe '#repository_size_limit column' do
+    it 'support values up to 8 exabytes' do
+      setting.update_column(:repository_size_limit, 8.exabytes - 1)
+
+      setting.reload
+
+      expect(setting.repository_size_limit).to eql(8.exabytes - 1)
+    end
+  end
 end

spec/models/ee/group_spec.rb

@@ -103,4 +103,15 @@ describe Group, models: true do
       expect(group.actual_size_limit).to eq(75)
     end
   end
+
+  describe '#repository_size_limit column' do
+    it 'support values up to 8 exabytes' do
+      group = create(:group)
+      group.update_column(:repository_size_limit, 8.exabytes - 1)
+
+      group.reload
+
+      expect(group.repository_size_limit).to eql(8.exabytes - 1)
+    end
+  end
 end

spec/models/project_spec.rb

@@ -623,6 +623,17 @@ describe Project, models: true do
     end
   end
 
+  describe '#repository_size_limit column' do
+    it 'support values up to 8 exabytes' do
+      project = create(:empty_project)
+      project.update_column(:repository_size_limit, 8.exabytes - 1)
+
+      project.reload
+
+      expect(project.repository_size_limit).to eql(8.exabytes - 1)
+    end
+  end
+
   describe '#default_issues_tracker?' do
     it "is true if used internal tracker" do
       project = build(:empty_project)

spec/services/application_settings/update_service_spec.rb

+require 'spec_helper'
+
+describe ApplicationSettings::UpdateService, services: true do
+  let(:user)    { create(:user) }
+  let(:setting) { ApplicationSetting.create_from_defaults }
+  let(:service) { described_class.new(setting, user, opts) }
+
+  describe '#execute' do
+    context 'common params' do
+      let(:opts) { { home_page_url: 'http://foo.bar' } }
+
+      it 'properly updates settings with given params' do
+        service.execute
+
+        expect(setting.home_page_url).to eql(opts[:home_page_url])
+      end
+    end
+
+    context 'with valid params' do
+      let(:opts) { { repository_size_limit: '100' } }
+
+      it 'returns success params' do
+        result = service.execute
+
+        expect(result).to eql(status: :success)
+      end
+    end
+
+    context 'with invalid params' do
+      let(:opts) { { repository_size_limit: '-100' } }
+
+      it 'returns error params' do
+        result = service.execute
+
+        expect(result).to eql(message: "Application settings could not be updated", status: :error)
+      end
+    end
+
+    context 'repository_size_limit assignment as Bytes' do
+      let(:service) { described_class.new(setting, user, opts) }
+
+      context 'when param present' do
+        let(:opts) { { repository_size_limit: '100' } }
+
+        it 'converts from MB to Bytes' do
+          service.execute
+
+          expect(setting.reload.repository_size_limit).to eql(100 * 1024 * 1024)
+        end
+      end
+
+      context 'when param not present' do
+        let(:opts) { { repository_size_limit: '' } }
+
+        it 'does not update due to invalidity' do
+          service.execute
+
+          expect(setting.reload.repository_size_limit).to be_zero
+        end
+
+        it 'assign nil value' do
+          service.execute
+
+          expect(setting.repository_size_limit).to be_nil
+        end
+      end
+    end
+  end
+end

spec/services/groups/create_service_spec.rb

@@ -40,4 +40,29 @@ describe Groups::CreateService, '#execute', services: true do
       end
     end
   end
+
+  context 'repository_size_limit assignment as Bytes' do
+    let(:admin_user) { create(:user, admin: true) }
+    let(:service) { described_class.new(admin_user, group_params.merge(opts)) }
+
+    context 'when param present' do
+      let(:opts) { { repository_size_limit: '100' } }
+
+      it 'assign repository_size_limit as Bytes' do
+        group = service.execute
+
+        expect(group.repository_size_limit).to eql(100 * 1024 * 1024)
+      end
+    end
+
+    context 'when param not present' do
+      let(:opts) { { repository_size_limit: '' } }
+
+      it 'assign nil value' do
+        group = service.execute
+
+        expect(group.repository_size_limit).to be_nil
+      end
+    end
+  end
 end

spec/services/groups/update_service_spec.rb

@@ -38,6 +38,31 @@ describe Groups::UpdateService, services: true do
     end
   end
 
+  context 'repository_size_limit assignment as Bytes' do
+    let(:group) { create(:group, :public, repository_size_limit: 0) }
+    let(:service) { described_class.new(group, user, opts) }
+
+    context 'when param present' do
+      let(:opts) { { repository_size_limit: '100' } }
+
+      it 'converts from MB to Bytes' do
+        service.execute
+
+        expect(group.reload.repository_size_limit).to eql(100 * 1024 * 1024)
+      end
+    end
+
+    context 'when param not present' do
+      let(:opts) { { repository_size_limit: '' } }
+
+      it 'assign nil value' do
+        service.execute
+
+        expect(group.reload.repository_size_limit).to be_nil
+      end
+    end
+  end
+
   context "unauthorized visibility_level validation" do
     let!(:service) { described_class.new(internal_group, user, visibility_level: 99) }
     before do

spec/services/projects/create_service_spec.rb

@@ -98,6 +98,30 @@ describe Projects::CreateService, '#execute', services: true do
     end
   end
 
+  context 'repository_size_limit assignment as Bytes' do
+    let(:admin_user) { create(:user, admin: true) }
+
+    context 'when param present' do
+      let(:opts) { { repository_size_limit: '100' } }
+
+      it 'assign repository_size_limit as Bytes' do
+        project = create_project(admin_user, opts)
+
+        expect(project.repository_size_limit).to eql(100 * 1024 * 1024)
+      end
+    end
+
+    context 'when param not present' do
+      let(:opts) { { repository_size_limit: '' } }
+
+      it 'assign nil value' do
+        project = create_project(admin_user, opts)
+
+        expect(project.repository_size_limit).to be_nil
+      end
+    end
+  end
+
   context 'restricted visibility level' do
     before do
       stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])

spec/services/projects/update_service_spec.rb

@@ -127,6 +127,31 @@ describe Projects::UpdateService, services: true do
     end
   end
 
+  context 'repository_size_limit assignment as Bytes' do
+    let(:admin_user) { create(:user, admin: true) }
+    let(:project) { create(:empty_project, repository_size_limit: 0) }
+
+    context 'when param present' do
+      let(:opts) { { repository_size_limit: '100' } }
+
+      it 'converts from MB to Bytes' do
+        update_project(project, admin_user, opts)
+
+        expect(project.reload.repository_size_limit).to eql(100 * 1024 * 1024)
+      end
+    end
+
+    context 'when param not present' do
+      let(:opts) { { repository_size_limit: '' } }
+
+      it 'assign nil value' do
+        update_project(project, admin_user, opts)
+
+        expect(project.reload.repository_size_limit).to be_nil
+      end
+    end
+  end
+
   it 'returns an error result when record cannot be updated' do
     result = update_project(project, admin, { name: 'foo&bar' })