Create Cop to enforce the use of namespaced classes

What does this MR do?

Related to #212156 (closed)

A healthy application is divided into macro and sub components that represent the domains at play. As GitLab code becomes broad with so many features and components it's hard to see what domains are at play.

We should expect any class to be defined inside a module/namespace that represents the domain where it operates.

The lack of namespaces today makes large parts of the codebase to have a flat structure:

• class names from different domains may be confused because X may mean something in one domain and something else in another domain
• it's hard to understand which group is responsible for the direction of the codebase (domain experts)
• it's extremely hard to understand the interactions between components
• etc.

This MR introduces a cop that flags class declarations that are not nested.

It's considered a first step towards defining better boundaries in the codebase.

This current implementation has a limitation. Compact class declarations like class MyDomain::MyClass are considered valid because we can't statically know if MyDomain is a class or a module. However if MyDomain is a class and it's indeed non nested, it will be flagged. Once it's fixed and all nested classes reorganized, the cop will be able to flag invalid declarations.

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

• Changelog entry
• Documentation (if required)
• Code review guidelines
• Merge request performance guidelines
• Style guides
• Database guides
• Separation of EE specific content

Availability and Testing

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• Tested in all supported browsers
• Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

• Label as security and @ mention @gitlab-com/gl-security/appsec
• The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
• Security reports checked/validated by a reviewer from the AppSec team

changed milestone to %13.8

added grouppipeline execution tooling (archive) labels

@grzesiek could you have a look?

cc @ayufan

@gitlab-org/maintainers/rails-backend With this MR we are looking into enforcing the use of namespaced classes from now on. The .rubocop_manual_todo.yml contains the list of non-namesapced classes that we could look into migrating step-by-step into a namespace that represents the product domain.

Feedback is very welcomed!

@fabiopitino thanks for doing this! I initially had a concern about things that do in fact cut across components, but most of the (non-model, non-controller) items that do so are already namespaced under Gitlab.

@smcgivern I think the Gitlab:: namespace contains a lot of offenses to this rule that are masked by the Gitlab:: generic namespace. Ideally in future iterations we should look into excluding Gitlab:: namespace from the rule and see if further namespacing is present:

• good: Gitlab::Ci::Config, Gitlab::Auth::AuthFinder, Gitlab::Database::BulkUpdate
• bad: a lot of files directly under lib/gitlab/ and ee/lib/gitlab/ (not feeling like picking bad examples)

In theory bounded contexts under app/ should have counterparts in lib/: E.g. Ci:: (in app) -> Gitlab::Ci:: (in lib), with exceptions to lib/ having more infrastructure/utility code. A good example is having all database tools under Gitlab::Database.

Maybe we could have Gitlab::Loggers::, Gitlab::GitAccess::, Gitlab::Imports::, Gitlab::Projects::. The problem is that Gitlab:: contains both domain code as well as infrastructure code. These 2 layers should live in different directories.

One goal we could aim to is to eventually move to a different directory structure like:

domains/
  ci/
    models/
    controllers/
    finders/
    lib/
  merge_requests/
  issues/
  projects/
gitlab/
  auth/
  database/
  elasticsearch/

Where each domain has its own lib folder. This means that classes like Gitlab::Ci::Config will become simply Ci::Config as it's part of Ci:: domain. Then Gitlab:: namespace will represent the infrastructure code.

Yes, I think that's a good idea. And I think you're right - Gitlab:: code essentially is at the top-level namespace, for certain things at least

@fabiopitino @smcgivern I agree! We should create an issue about this. How difficult would it be modify this cop to detect offenses in lib/? Do we need to have a separate issue? This might be a reasonable next step.

@grzesiek It depends whether we want to eventually move domain related logic into domains/ (long term strategy) and leave infrastructure/shared-kernel code into Gitlab:: in lib/. In general I think it's good practice to have another nesting aside from the top-level Gitlab:: because it helps identifying the infrastructure domain.

We could try to update the current cop in a different MR and see what other offenses come up. The current logic is masking them for now.

@grzesiek @fabiopitino I created #299181 (closed) to discuss making this check also apply to items directly under Gitlab::.

@fabiopitino I share your desired folder structure goal. A while ago I've made this proposal: #24725

assigned to @grzesiek

added backend label

	1 Warning
	This merge request is quite big (more than 1381 lines changed), please consider splitting it into multiple merge requests.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.

Category	Reviewer	Maintainer
backend	Serena Fang (@serenafang) (UTC-6, 6 hours behind @fabiopitino)	Dmitriy 'DZ' Zaporozhets (@dzaporozhets) (UTC+2, 2 hours ahead of @fabiopitino)

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added development guidelines 🍉 labels

removed 🍉 label

@fabiopitino amazing work! Thank your for this merge request!

I left a few questions.

I really like this as well!

added 1 commit

• 397a6c82 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

added 834 commits

• 397a6c82...dbe2280c - 833 commits from branch master
• b366e736 - Create Cop to enforce use of namespaced classes

Compare with previous version

requested review from @splattael

assigned to @splattael

Thanks a ton for this @fabiopitino

This looks great!

I left some minor comments/nitpicks

added 1 commit

• 4b2947f9 - Create Cop to enforce use of namespaced classes

Compare with previous version

approved this merge request

unassigned @splattael

added 1 commit

• 028050b1 - Create Cop to enforce use of namespaced classes

Compare with previous version

@rymai could you please review this?

assigned to @rymai

Setting label(s) devopsverify sectionops based on ~"group::continuous integration".

added devopsverify sectionops labels

mentioned in merge request !51572 (merged)

mentioned in issue #299181 (closed)

added 1 commit

• fa486439 - Create Cop to enforce use of namespaced classes

Compare with previous version

added 755 commits

• fa486439...c639d5ca - 754 commits from branch master
• 1808d12a - Create Cop to enforce use of namespaced classes

Compare with previous version

changed milestone to %13.9

unassigned @grzesiek and @fabiopitino

resolved all threads

approved this merge request

enabled an automatic merge when the pipeline for 842ca2ea succeeds

@fabiopitino I think we should document this new guideline as well.

merged

mentioned in commit 24c7171d

Thanks @fabiopitino all everyone involved!

mentioned in issue #212156 (closed)

mentioned in merge request !51999 (merged)

resolved all threads

Documentation MR: !51999 (merged)

added workflowstaging label

mentioned in issue #299273 (closed)

mentioned in issue #299290 (closed)

mentioned in merge request !49243 (merged)

added workflowcanary label and removed workflowstaging label

mentioned in issue #201855 (closed)

added workflowproduction label and removed workflowcanary label

mentioned in commit 70e92714

mentioned in merge request !51766 (merged)

@fabiopitino i'm broadly in favour of this change but i think unless we offer clearer guidelines about what constitutes a domain we may run the risk of promoting the lack of clarity into the namespaces themselves.

i think !51999 (merged) is a good start but i don't think it addresses this concern.

i've added a comment to the docs merge request along these lines.

@philipcunningham Thanks for raising the point! I'm going to open a MR and we can all collaborate there

mentioned in commit f3f705f5

mentioned in issue #299398 (closed)

mentioned in merge request !51296 (merged)

mentioned in commit be4341e3

mentioned in merge request !51782 (closed)

mentioned in merge request !52291 (merged)

mentioned in merge request !52359 (merged)

mentioned in issue #299853 (closed)

mentioned in issue #299882

mentioned in merge request !52807 (merged)

@fabiopitino @splattael @ayufan @grzesiek I agree with the intent here, but I think this MR is a little too tight. There are valid cases where we want to embed a class inside another class, not just a module.

In my case I'm extracting and replacing the traversal capabilities of the Namespace model. Namespace is a class, and it contains a collection of recursive query methods. These methods are specific to the Namespace model.

class Namespace < ApplicationRecord
  def ancestors
  end
end

I want to do something like this...

class Namespace < ApplicationRecord
  include Namespace::RecursiveTraversal
end

# app/models/namespace/recursive_traversal.rb
class Namespace
  module RecursiveTraversal
    def ancestors
    end
  end
end

Which is invalid according to this cop. I think not being able to embed within a top level class will be a significant hinderance going forward. Do you agree?

@alexpooley Intersting example, thank you

Personally, I am hesitant to use class also as a namespace and would prefer module instead. Should we create module Namespaces or similar?

Do you mind creating an issue to continue the discussion?

Thanks @splattael, I'm going to roll with Namespaces for now

@alexpooley Thanks for this example! I agree with @splattael. Even Namespace class should probably go under a Namespaces:: context.

mentioned in merge request !52999 (merged)

mentioned in merge request !52854 (merged)

mentioned in issue #300039 (closed)

From the frontend how should we best mirror the backend namespaces. In particular defining constants in frontend code.

We also try to name vue apps and vue components/folders to the feature we are working on. But the strings are a bit fuzzy, not always mapped to any one namespace.

mentioned in issue #321982

I've created a follow-up issue with next steps for fixing the existing cop offenses and foster discussion on more difficult offenses: #321982

added releasedcandidate label

mentioned in merge request !54793 (merged)

mentioned in merge request !55991 (merged)

mentioned in merge request !34042 (merged)

mentioned in merge request !61326 (closed)

mentioned in issue customers-gitlab-com#3324 (closed)

mentioned in merge request !79657 (merged)

added typemaintenance label and removed tooling (archive) label

mentioned in issue #393158