Add latest changes from gitlab-org/gitlab@master

1 files changed, 57 insertions, 7 deletions

diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md
index 37d8a677389..77e44760ef2 100644
--- a/doc/development/api_styleguide.md
+++ b/doc/development/api_styleguide.md
@@ -9,13 +9,13 @@ to access them as we do in Rails views), local variables are fine.
 
 ## Entities
 
-Always use an [Entity] to present the endpoint's payload.
+Always use an [Entity](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/api/entities) to present the endpoint's payload.
 
 ## Documentation
 
 API endpoints must come with [documentation](documentation/styleguide.md#api), unless it is internal or behind a feature flag.
 The docs should be in the same merge request, or, if strictly necessary,
-in a follow-up with the same milestone as the original merge request.  
+in a follow-up with the same milestone as the original merge request.
 
 ## Methods and parameters description
 
@@ -29,7 +29,7 @@ for a good example):
   - If the endpoint is deprecated, and if so, when will it be removed
 
 - `params` for the method parameters. This acts as description,
-  [validation, and coercion of the parameters]
+  [validation, and coercion of the parameters](https://github.com/ruby-grape/grape#parameter-validation-and-coercion)
 
 A good example is as follows:
 
@@ -106,7 +106,7 @@ For `DELETE` requests, you should also generally use the `destroy_conditionally!
 
 ## Using API path helpers in GitLab Rails codebase
 
-Because we support [installing GitLab under a relative URL], one must take this
+Because we support [installing GitLab under a relative URL](../install/relative_url.md), one must take this
 into account when using API path helpers generated by Grape. Any such API path
 helper usage must be in wrapped into the `expose_path` helper call.
 
@@ -121,9 +121,57 @@ For instance:
 The [internal API](./internal_api.md) is documented for internal use. Please keep it up to date so we know what endpoints
 different components are making use of.
 
-[Entity]: https://gitlab.com/gitlab-org/gitlab/blob/master/lib/api/entities
-[validation, and coercion of the parameters]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion
-[installing GitLab under a relative URL]: https://docs.gitlab.com/ee/install/relative_url.html
+## Avoiding N+1 problems
+
+In order to avoid N+1 problems that are common when returning collections
+of records in an API endpoint, we should use eager loading.
+
+A standard way to do this within the API is for models to implement a
+scope called `with_api_entity_associations` that will preload the
+associations and data returned in the API. An example of this scope can
+be seen in
+[the `Issue` model](https://gitlab.com/gitlab-org/gitlab/blob/2fedc47b97837ea08c3016cf2fb773a0300a4a25/app%2Fmodels%2Fissue.rb#L62).
+
+In situations where the same model has multiple entities in the API
+(for instance, `UserBasic`, `User` and `UserPublic`) you should use your
+discretion with applying this scope. It may be that you optimize for the
+most basic entity, with successive entities building upon that scope.
+
+The `with_api_entity_associations` scope will also [automatically preload
+data](https://gitlab.com/gitlab-org/gitlab/blob/19f74903240e209736c7668132e6a5a735954e7c/app%2Fmodels%2Ftodo.rb#L34)
+for `Todo` _targets_ when returned in the Todos API.
+
+For more context and discussion about preloading see
+[this merge request](https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/25711)
+which introduced the scope.
+
+### Verifying with tests
+
+When an API endpoint returns collections, always add a test to verify
+that the API endpoint does not have an N+1 problem, now and in the future.
+We can do this using [`ActiveRecord::QueryRecorder`](query_recorder.md).
+
+Example:
+
+```ruby
+def make_api_request
+  get api('/foo', personal_access_token: pat)
+end
+
+it 'avoids N+1 queries', :request_store do
+  # Firstly, record how many PostgreSQL queries the endpoint will make
+  # when it returns a single record
+  create_record
+
+  control = ActiveRecord::QueryRecorder.new { make_api_request }
+
+  # Now create a second record and ensure that the API does not execute
+  # any more queries than before
+  create_record
+
+  expect { make_api_request }.not_to exceed_query_limit(control)
+end
+```
 
 ## Testing
 
@@ -132,3 +180,5 @@ When writing tests for new API endpoints, consider using a schema [fixture](./te
 ```ruby
 expect(response).to match_response_schema('merge_requests')
 ```
+
+Also see [verifying N+1 performance](#verifying-with-tests) in tests.