Discuss: Are abstractions always necessary?

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Problem

In !116650 (comment 1352242799) and !117634 (comment 1356814448) @DylanGriffith provided an example when https://docs.gitlab.com/ee/development/reusing_abstractions.html#guidelines-for-reusing-abstractions would not apply for "very simple" CRUD operations. This is not state in our development guidelines and could lead to confusion during review.

Proposed solution

1. Add a section to our guidelines and clearly state when abstractions can be avoided
2. Introduce finder, services etc. in zoekt code

Discussion

The following discussion from !116650 (merged) should be addressed:

• @splattael started a discussion: (+2 comments)

  Question Any reason why this logic should not be in a service? 🤔

  It seems we don't have any services and finders for Zoekt (yet). Are we planning to add them later?

  Refs https://docs.gitlab.com/ee/development/reusing_abstractions.html#guidelines-for-reusing-abstractions.

@DylanGriffith

For now there is very little logic and I honestly don't expect there to be much added in future. These are very simple adminsitrative CRUD operations and they won't ever be exposed to non-admin users. So we don't need to implement things like authorization and so-on which is normally found in the Finder class. I realise the Finder can also be useful when building lots of filtering options and I don't necessarily think we'll need that any time soon. Similarly with Service there isn't really anything beyond the normal rails create methods that we'll need here.

For this code I was mostly following the patterns we have with ElasticsearchIndexedNamespace which is basically the same model that defines a namespace that is indexed in Elasticsearch and allowed us to control the rollout of Elasticsearch. These models have been in the codebase for a few years now and have very little churn and I don't think we ever really felt the need to introduce Service/Finder classes for them.

From my experience there is no hard rules in the GitLab codebase about always using service classes to perform an operation and from what I see here the small amount of logic encapsulated in the model itself seems sufficient and would probably still serve in the long term.

@splattael

So we don't need to implement things like authorization and so-on which is normally found in the Finder class.

The finder could ensure that the user is an admin, no?

From my experience there is no hard rules in the GitLab codebase about always using service classes to perform an operation and from what I see here the small amount of logic encapsulated in the model itself seems sufficient and would probably still serve in the long term.

Personally, I disagree here and I think we should be using services, finders etc. regardless of the scope/amount of logic.

Since that's just personal preference I'd fine to not use abstractions, however, it seems that our guidelines is missing a section "When not to use abstractions" (or similar).

I don't want to block this MR because of this so let's follow-up on this and

• Either introduce finder, services etc.
• OR add a section to our guidelines and clearly state when abstractions can be avoided