Skip to content

Resolve "Importing group as a top level group targeting an existing namespace give false positive status"

What does this MR do and why?

This MR gives correct error message when you're trying to specify import target, which already exists in the system but is invisible to user (i.e. group which is private as an example) See #341185 (closed) for details

Screenshots or screen recordings

import

How to set up and validate locally

  • Enable feature bulk_import via Feature.enable(:bulk_import) (should be enabled by default)
  • (we are assuming you have default GDK setup with private group Commit451 available)
  • Impersonate to any non-admin user
  • Open "New group" -> Import (/groups/new)
  • Use https://gitlab.com as source instance and GeK1Nis4j-SY1X4sqE5c as personal access token (this token is from separate user on GitLab instance with 0 real data available, so we do not expose any security risks here)
  • Try to specify Commit451 as import target name
  • Observe validation error

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Technical details

First of all, I'm super sorry for 💣 huge 💣 size of this MR. Unfortunately this is the scenario when one single feature quickly show all the weaknesses of selected architecture, which resulted in major refactor. I've tried to slice this one in separate pieces which created even more complexity in understanding logic behind changes

It might be easier to get the logic behind code, if you just do a "review" of import_entities/import_groups entire code

Why you are doing such major refactoring?

Well, it started pretty simple... Our group(...) GraphQL query reports group as non-existent even if it exists (which is kind of logical). We have a separate REST api call /api/v4/namespaces/${namespaceName}/exists which returns if group exists Ok, let's make a new client resolver for this... except this will not work:

  • You do not have an ability to cancel graphql query implemented by local resolver. And having "suggestions" api uncancellable creates all kind of the risks - starting from "out-of-order" requests to stuck requests
  • /api/v4/namespaces/${namespaceName}/exists returns suggestion - this is the name which is guaranteed to be available. We want to consume this suggestion on initial load - because we could "auto-generate" import name which was already occupied. This was previously implemented in local resolvers, but what if our suggestion is one of the names which we already use as suggestion (I know it is complex, I hope video makes it clearer). To workaround this we need to iterate across all current suggestions and if they are located in local resolvers ... We simply do not have a clean way to do that - in order to repeat importGroups query we need to be aware of all parameters (filtering, pagination) inside resolver... which puts us to doom

So what was done?

  • All validation logic moved from local resolvers to main table component. This simplified entire flow for understanding so much
  • (while we are here) behavior of local GraphQL was normalized to conform GraphQL standards (so webUrl instead of web_url)
  • Logic of "put group status to pending while XHR request is in flight" also removed from resolvers. Instead we track "pending requests" in component

All these changes unlock are future potential migration to real graphql queries and greatly improves readability of final result

Edited by Illya Klymov

Merge request reports