Cache Namespace#full_name and Namespace#full_path methods

What does this MR do?

Cache Namespace#full_name and Namespace#full_path methods output

Are there points in the code the reviewer needs to double check?

no

Why was this MR needed?

To avoid extra sql queries (especially for nested groups). We show full path and full name in many places from UI to API.

Does this MR meet the acceptance criteria?

• Changelog entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/2772

changed title from WIP Allow creating nested group via UI to WIP Cache Namespace#full_name and Namespace#full_path methods

added 2 commits

• 64fe637f - Add CacheMethod concern
• 215fd406 - Cache Namespace#full_name and Namespace#full_path methods

Compare with previous version

added 220 commits

• 215fd406...b525aff6 - 218 commits from branch master
• de5f7ffc - Add CacheMethod concern
• 533d7bfb - Cache Namespace#full_name and Namespace#full_path methods

Compare with previous version

changed milestone to %8.17

@yorickpeterse I decided to cache Namespace#full_name and Namespace#full_path since we use it in many places like groups and projects lists, autocomplete, api etc. I copied part of functionality from Repository class and extracted into concern. What do you think about this merge request?

@dzaporozhets Is it possible to perhaps store this on database level? It would remove the need for the complex caching/flushing logic, and would allow you to query data by the full path.

@yorickpeterse I already store full path in routes table and its used to query data. And I want to keep it as single point of truth.

ok I will see if I can make it simpler

@dzaporozhets Ah gotcha. I'm fine with re-using this code, though I think the respond_to? call might depend a bit too much on internal state (the naming of the original methods to be precise). I can see this breaking silently when the names for original methods are changed.

@yorickpeterse makes sense. I will replace respond_to? with direct method call so if it fails -> it fails

added 54 commits

• 533d7bfb...6aec85ac - 52 commits from branch master
• eac00971 - Add CacheMethod concern
• ff262720 - Cache full_name and full_path methods for both Project and Namespace

Compare with previous version

@yorickpeterse I removed respond_to and extracted full_name and full_path into separate concerns for clear picture. If from performance point it (this mr I mean) is ok - then I proceed with adding more tests and getting it ready for review

@dzaporozhets 👍

added 1 commit

• dc86e880 - Cache full_name and full_path methods for both Project and Namespace

Compare with previous version

added 1 commit

• 2c6b21b5 - Change merge_request_diff_cache_service_spec.rb

Compare with previous version

marked the task Added for this feature/bug as completed

marked the task Conform by the merge request performance guides as completed

marked the task Conform by the style guides as completed

marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

marked the task Squashed related commits together as completed

unmarked as a Work In Progress

marked the task All builds are passing as completed

@DouweM please review

assigned to @DouweM

@dzaporozhets Code looks good, but is always doing a Redis read for the full path really faster than doing a SQL query for the related Route, and reading the full path from that?

assigned to @dzaporozhets

@DouweM I need Redis cache for full_name anyway so I decided to use same approach for full_path. Plus for route.path we need to add includes(:route) everywhere to prevent extra queries -> as result I decided its simple and safe to start with redis and then move forward to route.path later

@dzaporozhets The benefit of includes(:route) is that is results in only 1 extra query on a project index, to get the routes for all projects. With the Redis approach, we need N Redis reads, which aren't parallelised. I wouldn't mind storing the full_name on Route too, so that we get don't have the N+1 issue.

@DouweM makes sense. I will create separate MR with routes table approach so we can compare and choose the best one

mentioned in merge request !8979 (merged)

Closed in favor of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8979

closed

mentioned in issue #29554 (closed)