diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-08-16 17:14:14 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-08-24 19:49:18 +0200 |
commit | 1ee18c47c7e748339633c2ec3bf98169b56aaa2d (patch) | |
tree | 8168a81001cd38e1d2ef9b56f12873ab94f37dbc | |
parent | 682c6ad931a67f7bbc76baad21af0d3b5d5b449d (diff) | |
download | gitlab-ce-docs/abstraction-levels.tar.gz |
Document abstraction levels and code reusedocs/abstraction-levels
This is based on the discussion in
https://gitlab.com/gitlab-org/gitlab-ce/issues/49653 and the example
merge request found at
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20908. The idea
is fairly straightforward: we define a variety of abstractions, then
state which can reuse which. This should ultimately foster _better_ code
reuse, make it easier to debug problems, and make it harder to
accidentally introduce a performance regression.
-rw-r--r-- | doc/development/README.md | 1 | ||||
-rw-r--r-- | doc/development/reusing_abstractions.md | 158 |
2 files changed, 159 insertions, 0 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index fed3903c771..8449c1dd4f5 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -47,6 +47,7 @@ description: 'Learn how to contribute to GitLab.' - [How to dump production data to staging](db_dump.md) - [Working with the GitHub importer](github_importer.md) - [Working with Merge Request diffs](diffs.md) +- [Guidelines for reusing abstractions](reusing_abstractions.md) ## Performance guides diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md new file mode 100644 index 00000000000..60a0752502b --- /dev/null +++ b/doc/development/reusing_abstractions.md @@ -0,0 +1,158 @@ +# Guidelines for reusing abstractions + +As GitLab has grown, different patterns emerged across the codebase. Service +classes, serializers, and presenters are just a few. These patterns made it easy +to reuse code, but at the same time make it easy to accidentally reuse the wrong +abstraction in a particular place. + +## Why these guidelines are necessary + +Code reuse is good, but sometimes this can lead to shoehorning the wrong +abstraction into a particular use case. This in turn can have a negative impact +on maintainability, the ability to easily debug problems, or even performance. + +An example would be to use `ProjectsFinder` in `IssuesFinder` to limit issues to +those belonging to a set of projects. While initially this may seem like a good +idea, both classes provide a very high level interface with very little control. +This means that `IssuesFinder` may not be able to produce a better optimised +database query, as a large portion of the query is controlled by the internals +of `ProjectsFinder`. + +To work around this problem, you would use the same code used by +`ProjectsFinder`, instead of using `ProjectsFinder` itself directly. This allows +you to compose your behaviour better, giving you more control over the behaviour +of the code. + +To illustrate, consider the following code from `IssuableFinder#projects`: + +```ruby +return @projects = project if project? + +projects = + if current_user && params[:authorized_only].presence && !current_user_related? + current_user.authorized_projects + elsif group + finder_options = { include_subgroups: params[:include_subgroups], only_owned: true } + GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute + else + ProjectsFinder.new(current_user: current_user).execute + end + +@projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) +``` + +Here we determine what projects to scope our data to, using three different +approaches. When a group is specified, we use `GroupProjectsFinder` to retrieve +all the projects of that group. On the surface this seems harmless: it is easy +to use, and we only need two lines of code. + +In reality, things can get hairy very quickly. For example, the query produced +by `GroupProjectsFinder` may start out simple. Over time more and more +functionality is added to this (high level) interface. Instead of _only_ +affecting the cases where this is necessary, it may also start affecting +`IssuableFinder` in a negative way. For example, the query produced by +`GroupProjectsFinder` may include unnecessary conditions. Since we're using a +finder here, we can't easily opt-out of that behaviour. We could add options to +do so, but then we'd need as many options as we have features. Every option adds +two code paths, which means that for four features we have to cover 8 different +code paths. + +A much more reliable (and pleasant) way of dealing with this, is to simply use +the underlying bits that make up `GroupProjectsFinder` directly. This means we +may need a little bit more code in `IssuableFinder`, but it also gives us much +more control and certainty. + +## End goal + +The guidelines in this document are meant to foster _better_ code reuse, by +clearly defining what can be reused where, and what to do when you can not reuse +something. Clearly separating abstractions makes it harder to use the wrong one, +makes it easier to debug the code, and (hopefully) result in fewer performance +problems. + +## Abstractions + +Now let's take a look at the various abstraction levels available, and what they +can (or can not) reuse. For this we can use the following table, which defines +the various abstractions and what they can (not) reuse: + +| Abstraction | Service classes | Finders | Presenters | Serializers | Model class methods | Model instance method | Active Record | Worker +|:-----------------------|:-----------------|:---------|:------------|:--------------|:----------------------|:------------------------|:----------------|:-------- +| Controller | Yes | Yes | Yes | Yes | No | Yes | No | No +| Grape endpoint | Yes | Yes | Yes | Yes | No | Yes | No | No +| Service class | Yes | Yes | No | No | No | Yes | No | Yes +| Finder | No | No | No | No | Yes | Yes | No | No +| Presenter | No | Yes | No | No | Yes | Yes | No | No +| Serializer | No | Yes | No | No | Yes | Yes | No | No +| Model class method | No | No | No | No | Yes | Yes | Yes | No +| Model instance method | Yes | Yes | No | No | Yes | Yes | Yes | Yes +| Worker | Yes | Yes | No | No | No | Yes | No | Yes + +### Controllers + +Everything in `app/controllers`. + +Controllers should not do much work on their own, instead they simply pass input +to other classes and present the results. + +## Grape endpoint + +Everything in `lib/api`. + +### Service classes + +Everything that resides in `app/services`. + +### Finders + +Everything in `app/finders`, typically used for retrieving data from a database. + +Finders can not reuse other finders in an attempt to better control the SQL +queries they produce. Instead, finders should use model class methods to +construct their queries. + +### Presenters + +Everything in `app/presenters`, used for exposing complex data to a Rails view, +without having to create many instance variables. + +### Serializers + +Everything in `app/serializers`, used for presenting the response to a request, +typically in JSON. + +### Model class methods + +These are class methods defined by _GitLab itself_, including the following +methods provided by Active Record: + +* `find` +* `find_by_id` +1. `save` +1. `update` +1. `delete` +1. `delete_all` +1. `destroy` +1. `destroy_all` + +Any other methods such as `find_by(some_column: X)` are not included, and +instead fall under the "Active Record" abstraction. + +### Model instance methods + +Instance methods defined on Active Record models by _GitLab itself_. Methods +provided by Active Record are not included, instead they fall under the "Active +Record" abstraction. + +## Active Record + +The API provided by Active Record itself, such as the `where` method, `save`, +`delete_all`, etc. + +### Sidekiq + +Everything in `app/workers`. + +The scheduling of Sidekiq jobs using `SomeWorker.perform_async`, `perform_in`, +etc. Directly invoking a worker using `SomeWorker.new.perform` should be avoided +at all times in application code, though this is fine to use in tests. |