Remove dependency on project from (at least) repository, blob, and commit objects
Summary
Follow-up to !23678 (comment 282219639)
We're in the process of transforming Snippets so they are Git repositories. This is an interesting problem, because until now, all repositories have had a project. With Personal Snippets, this assumption is broken in the code.
We're working around this by turning the idea of "project" into "anything that includes the HasRepository
concern", which we're calling "Container" (following a brief flirtation with Repositorable
). However, there's no getting around the fact that some pieces of code that previously expected to have a project passed in alongside their repository or git objects, will have their write-time expectations violated.
Example: Sometimes we expect project.id
to be globally unique, while container.id
will not be globally unique
I also expect this to be fairly painful in ongoing development - every time a repository attached to a personal snippet is applied to code, it's bringing along a surprising implication that is unlikely to have been thought about in design or tests, in a similar way to how things often break for forked MRs, where "this object has exactly one project" is not a safe assumption.
Improvements
::Commit
, and ::Blob
currently track both project
and repository
. The project
is being renamed to container
and specified as "anything that includes the HasRepository
concern.
I think we could merge the HasRepository
model into Repository
, or make it into a simple wrapper class that takes a repository
and acts upon it. In theory, the Commit
and Blob
then only have to track the Repository
, and we can delete the project
/ container
members completely.
In practice, some functionality that uses these classes is going to actually depend on the project being present. Some will be rewritable to depend on the Repository instead. For each of the remaining cases, we'll have to work out why it depends on the project, determine what makes sense instead in the personal snippet case, and either introduce a new abstraction, or pass in a project
/ nil
alongside the commit
/ blob
and ensure the nil case is handled correctly.
It's a big refactoring, but it forces us to consider the implications of a nil
project everywhere it's actually used (since all routes relying on it being present will break when we remove ::Commit#project
), and I think we'll have a more loosely coupled, more maintainable codebase as a result.
We can handle this incrementally, with one big "broken" MR for discovery and and individual MRs for required refactor.
It's the same story for ::Repository
, but the dependencies are more extensive. We might not be able to remove project
/ container
entirely, but I think it's worth approaching in the same way as above, at least to begin with. We will discover new abstractions (like "we need a globally unique key for caching") with implications that are likely to be missed if we approach this in a piecemeal way by passing in either a PersonalSnippet
or Project
into places that previously only expected Project
instances.
Risks
We can always get these kinds of refactorings wrong, and this one will touch a great many lines of code, and make some code more convoluted or less direct. However, in this case, I think the risks of proceeding without a refactoring like this are higher! Since we're introducing git repositories for personal snippets, and since they simply don't have a project, and creatring a dummy project per personal snippet isn't an option, we're not able to maintain the status quo.
@fjsanpedro has some other ideas for refactoring too; I think this one is mostly orthogonal to those, but I'll list them here for completeness:
Involved components
::Commit
::Blob
::Repository
::HasRepository
::Project
::Snippet
@reprazent I know you've been involved with some of the snippet git repository review work so far; can I poll you for opinion here? @gitlab-com/create-stage more generally might be interested.