From bf91b06b1d5ebe95a82eccd98271ce47c781b3c5 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 13 Nov 2018 13:33:02 +0100 Subject: [PATCH] Update docs to put prepend as the last line This updates the documentation for adding EE specific features to clarify that `prepend` should be on the last line, and how to deal with code that depends on class methods redefined in EE. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/53919 --- doc/development/ee_features.md | 153 ++++++++++++++++++++++++++------- 1 file changed, 124 insertions(+), 29 deletions(-) diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index b6f053ff0e9..9aea03139ee 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -119,10 +119,20 @@ This also applies to views. ### EE features based on CE features -For features that build on existing CE features, write a module in the -`EE` namespace and `prepend` it in the CE class. This makes conflicts -less likely to happen during CE to EE merges because only one line is -added to the CE class - the `prepend` line. +For features that build on existing CE features, write a module in the `EE` +namespace and `prepend` it in the CE class, on the last line of the file that +the class resides in. This makes conflicts less likely to happen during CE to EE +merges because only one line is added to the CE class - the `prepend` line. For +example, to prepend a module into the `User` class you would use the following +approach: + +```ruby +class User < ActiveRecord::Base + # ... lots of code here ... +end + +User.prepend(EE::User) +``` Since the module would require an `EE` namespace, the file should also be put in an `ee/` sub-directory. For example, we want to extend the user model @@ -231,7 +241,6 @@ the existing file: ```ruby class ApplicationController < ActionController::Base - prepend EE::ApplicationController # ... def after_sign_out_path_for(resource) @@ -240,6 +249,8 @@ class ApplicationController < ActionController::Base # ... end + +ApplicationController.prepend(EE::ApplicationController) ``` And create a new file in the `ee/` sub-directory with the altered @@ -533,8 +544,6 @@ module API end end - prepend EE::API::MergeRequests - params :optional_params do # CE specific params go here... @@ -542,6 +551,8 @@ module API end end end + +API::MergeRequests.prepend(EE::API::MergeRequests) ``` And then we could override it in EE module: @@ -582,10 +593,10 @@ module API authorize_read_builds! end end - - prepend EE::API::JobArtifacts end end + +API::JobArtifacts.prepend(EE::API::JobArtifacts) ``` And then we can follow regular object-oriented practices to override it: @@ -626,8 +637,6 @@ module API end end - prepend EE::API::MergeRequests - put ':id/merge_requests/:merge_request_iid/merge' do merge_request = find_project_merge_request(params[:merge_request_iid]) @@ -639,6 +648,8 @@ module API end end end + +API::MergeRequests.prepend(EE::API::MergeRequests) ``` Note that `update_merge_request_ee` doesn't do anything in CE, but @@ -676,27 +687,37 @@ or not we really need to extend it from EE. For now we're not using it much. Sometimes we need to use different arguments for a particular API route, and we can't easily extend it with an EE module because Grape has different context in -different blocks. In order to overcome this, we could use class methods from the -API class. +different blocks. In order to overcome this, we need to move the data to a class +method that resides in a separate module or class. This allows us to extend that +module or class before its data is used, without having to place a `prepend` in +the middle of CE code. For example, in one place we need to pass an extra argument to `at_least_one_of` so that the API could consider an EE-only argument as the -least argument. This is not quite beautiful but it's working: +least argument. We would approach this as follows: ```ruby +# api/merge_requests/parameters.rb module API class MergeRequests < Grape::API - def self.update_params_at_least_one_of - %i[ - assignee_id - description - ] + module Parameters + def self.update_params_at_least_one_of + %i[ + assignee_id + description + ] + end end + end +end - prepend EE::API::MergeRequests +API::MergeRequests::Parameters.prepend(EE::API::MergeRequests::Parameters) +# api/merge_requests.rb +module API + class MergeRequests < Grape::API params do - at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of) + at_least_one_of(*Parameters.update_params_at_least_one_of) end end end @@ -708,16 +729,18 @@ And then we could easily extend that argument in the EE class method: module EE module API module MergeRequests - extend ActiveSupport::Concern + module Parameters + extend ActiveSupport::Concern - class_methods do - extend ::Gitlab::Utils::Override + class_methods do + extend ::Gitlab::Utils::Override - override :update_params_at_least_one_of - def update_params_at_least_one_of - super.push(*%i[ - squash - ]) + override :update_params_at_least_one_of + def update_params_at_least_one_of + super.push(*%i[ + squash + ]) + end end end end @@ -728,6 +751,78 @@ end It could be annoying if we need this for a lot of routes, but it might be the simplest solution right now. +This approach can also be used when models define validations that depend on +class methods. For example: + +```ruby +# app/models/identity.rb +class Identity < ActiveRecord::Base + def self.uniqueness_scope + [:provider] + end + + prepend EE::Identity + + validates :extern_uid, + allow_blank: true, + uniqueness: { scope: uniqueness_scope, case_sensitive: false } +end + +# ee/app/models/ee/identity.rb +module EE + module Identity + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + def uniqueness_scope + [*super, :saml_provider_id] + end + end + end +end +``` + +Instead of taking this approach, we would refactor our code into the following: + +```ruby +# ee/app/models/ee/identity/uniqueness_scopes.rb +module EE + module Identity + module UniquenessScopes + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + def uniqueness_scope + [*super, :saml_provider_id] + end + end + end + end +end + +# app/models/identity/uniqueness_scopes.rb +class Identity < ActiveRecord::Base + module UniquenessScopes + def self.uniqueness_scope + [:provider] + end + end +end + +Identity::UniquenessScopes.prepend(EE::Identity::UniquenessScopes) + +# app/models/identity.rb +class Identity < ActiveRecord::Base + validates :extern_uid, + allow_blank: true, + uniqueness: { scope: Identity::UniquenessScopes.scopes, case_sensitive: false } +end +``` + ### Code in `spec/` When you're testing EE-only features, avoid adding examples to the