diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2018-03-06 20:10:55 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2018-03-06 20:10:55 +0300 |
commit | 65650bc0bea88339991fad68ecc01eeb5dd11877 (patch) | |
tree | 8aa25886d58a240602a2c7e23b761358458811fa /doc/development/ee_features.md | |
parent | be60d106a8b1f1600e8305465e8440689ef30f0a (diff) |
Document a few strategies to extract EE APIs
Diffstat (limited to 'doc/development/ee_features.md')
-rw-r--r-- | doc/development/ee_features.md | 249 |
1 files changed, 249 insertions, 0 deletions
diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 1eb90c30ebd..bbe80bc9c02 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -350,6 +350,255 @@ class beneath the `EE` module just as you would normally. For example, if CE has LDAP classes in `lib/gitlab/ldap/` then you would place EE-specific LDAP classes in `ee/lib/ee/gitlab/ldap`. +### Code in `lib/api/` + +It could be very tricky to extend EE features by a single line of `prepend`, +and for each different [Grape](https://github.com/ruby-grape/grape) features, +we might need different strategies to extend it. To apply different strategies +easily, we would use `extend ActiveSupport::Concern` in the EE module. + +Put the EE module files following the same rule with other EE modules. + +#### EE API routes + +For EE API routes, we could just put them in `prepended` block like: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + params do + requires :id, type: String, desc: 'The ID of a project' + end + resource :projects, requirements: ::API::API::PROJECT_ENDPOINT_REQUIREMENTS do + # ... + end + end + end + end +end +``` + +Note that due to namespace difference, we need to use full qualifier for some +constants. If this is annoying, we could also consider include some namespace +so that we could use shorter name. This is especially true for Grape +data type like: `Grape::API::Boolean`. + +#### EE params + +We could define `params` and use `use` in another params to include EE defined +params. However we need to define the "interface" first in CE in order for +EE to override it. We don't have to do this in other places due to `prepend`, +but Grape is complex internally and we couldn't easily do that, so we'll just +follow regular object-oriented practice that we define interface first here. + +For example, suppose we have a few more optional params for EE, given this CE +API code: + +``` ruby +module API + class MergeRequests < Grape::API + # EE::API::MergeRequests would override the following helpers + helpers do + params :optional_params_ee do + end + end + + prepend EE::API::MergeRequests + + params :optional_params do + # CE specific params go here... + + use :optional_params_ee + end + end +end +``` + +And then we could override it in EE module: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + helpers do + params :optional_params_ee do + # EE specific params go here... + end + end + end + end + end +end +``` + +This way, the only difference in CE and EE for that API file, would be: +`prepend EE::API::MergeRequests` and everything else should be the same. + +#### EE helpers + +To make it easy for EE module to override the CE helpers, we need to define +those helpers we want to extend first. Try to do that just after class +definition to make it easy and clear: + +``` ruby +module API + class JobArtifacts < Grape::API + # EE::API::JobArtifacts would override the following helpers + helpers do + def authorize_download_artifacts! + authorize_read_builds! + end + end + + prepend EE::API::JobArtifacts + end +end +``` + +And then we could just follow regular object-oriented practice to override it: + +``` ruby +module EE + module API + module JobArtifacts + extend ActiveSupport::Concern + + prepended do + helpers do + def authorize_download_artifacts! + super + check_cross_project_pipelines_feature! + end + end + end + end + end +end +``` + +#### EE specific behaviour + +Sometimes we need EE specific behaviour in some of the APIs. Normally we could +use EE methods to override CE methods, however API routes are not methods +therefore we can't simply override them. We would need to extract them into a +standalone method, or just introduce some "hooks" where we could inject +behaviour in CE route. Something like: + +``` ruby +module API + class MergeRequests < Grape::API + helpers do + # EE::API::MergeRequests would override the following helpers + def update_merge_request_ee(merge_request) + 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]) + + # ... + + update_merge_request_ee(merge_request) + + # ... + end + end +end +``` + +See that above `update_merge_request_ee` doesn't do anything in CE, but +then we could override it in EE: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + helpers do + def update_merge_request_ee(merge_request) + # ... + end + end + end + end + end +end +``` + +#### EE route_setting + +It's very hard to extend this by the EE module, and this is simply storing +some meta-data for a particular route. Given that, we could simply leave the +EE `route_setting` in CE as it won't hurt and we are just not going to use +those meta-data in CE. + +We could revisit this policy when we're using `route_setting` more and if +we might really need to extend it from EE. For now we're not using it so much. + +#### Utilizing class methods for setting up EE specific data + +Sometimes we need to use different arguments for a particular API route, and +we can't easily extend it with EE module because Grape has different context +in different blocks. In order to overcome this, we could just use class +methods from the API class. + +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: + +``` ruby +module API + class MergeRequests < Grape::API + def self.update_params_at_least_one_of + %i[ + assignee_id + description + ] + end + + prepend EE::API::MergeRequests + + params do + at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of) + end + end +end +``` + +And then we could easily extend that arguments in EE class method: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + class_methods do + def update_params_at_least_one_of + super.push(*%i[ + squash + ]) + end + end + end + end +end +``` + +It could be annoying if we need this for a lot of routes, but it might be the +simplest solution right now. + ### Code in `spec/` When you're testing EE-only features, avoid adding examples to the |