Skip to content

Add connections between User and Group or Project

Max Woolf requested to merge 215658-graphql-memberships into master

What does this MR do?

Adds some new functionality affecting only the GraphQL API.

Note: After a long discussion from a technical standpoint, it makes sense to scrap the concept of memberships and simply allow a graph traversal between UserType and GroupType or ProjectType.

  • UserType traverses to ProjectType through ProjectMembershipType
  • UserType traverses to GroupType through GroupMembershipType

The updated graph will look a bit like this:

graph TD
  A((User)) --- B((GroupMembership))
  B --- C((Group))
  A --- D((GroupMembership))
  D --- E((Group))

  A --- F((ProjectMembership))

  P((User)) --- Q((ProjectMembership))
  P --- I((GroupMembership))
  I --- C

  Q & F --- G((Project))

  classDef usertype fill:#5B9279;
  class A usertype;
  class P usertype;

  classDef pm fill:#8F8073;
  class F pm;
  class Q pm;

  classDef gt fill:#30C5FF;
  class I,D,B gt;

Examples

This GraphQL query:

{
  user(username: "mwoolf") {
    id
    username
    ...memberships
  }
}


fragment membership on MemberInterface {
  createdAt
  updatedAt
  accessLevel {
    integerValue
    stringValue
  }
  createdBy {
    id
  }
}

fragment memberships on User {
  groupMemberships {
    nodes {
      ...membership
      group {
        id
      }
    }
  }
  
  projectMemberships {
    nodes {
      ...membership
      project {
        id
      }
    }
  }
}

Will generate JSON similar to this:

{
  "data": {
    "user": {
      "id": "gid://gitlab/User/1",
      "name": "Administrator",
      "groupMemberships": {
        "nodes": [
          {
            "createdAt": "2020-04-20T13:00:51Z",
            "updatedAt": "2020-04-20T13:01:01Z",
            "accessLevel": {
              "integerValue": 30,
              "stringValue": "DEVELOPER"
            },
            "createdBy": null,
            "group": {
              "id": "gid://gitlab/Group/28"
            }
          },
          {
            "createdAt": "2020-04-20T13:00:41Z",
            "updatedAt": "2020-04-20T13:00:41Z",
            "accessLevel": {
              "integerValue": 50,
              "stringValue": "OWNER"
            },
            "createdBy": null,
            "group": {
              "id": "gid://gitlab/Group/26"
            }
          },
          {
            "createdAt": "2020-04-20T13:00:02Z",
            "updatedAt": "2020-04-20T13:01:02Z",
            "accessLevel": {
              "integerValue": 40,
              "stringValue": "MAINTAINER"
            },
            "createdBy": null,
            "group": {
              "id": "gid://gitlab/Group/23"
            }
          },
          {
            "createdAt": "2020-04-20T12:59:43Z",
            "updatedAt": "2020-04-20T13:01:02Z",
            "accessLevel": {
              "integerValue": 10,
              "stringValue": "GUEST"
            },
            "createdBy": null,
            "group": {
              "id": "gid://gitlab/Group/22"
            }
          }
        ]
      },
      "projectMemberships": {
        "nodes": [
          {
            "createdAt": "2020-05-05T09:09:03Z",
            "updatedAt": "2020-05-05T09:09:03Z",
            "accessLevel": {
              "integerValue": 40,
              "stringValue": "MAINTAINER"
            },
            "createdBy": null,
            "project": {
              "id": "gid://gitlab/Project/29"
            }
          }
        ]
      }
    }
  }
}

Implementation discussion

Discussion from Slack#questions on 2020-05-26:

Max Woolf May 26th at 9:51 AM Hi folks. Is there a term we use to refer to all of the "things" that a user can be a member of (a project, group, etc.). I'm looking for a single term that encompasses all of them. "User X has 3 _____?" "Sources" is the closest I can come up with, but there's also an agreement that it's not great.

23 replies

smcgivern 13 days ago 'user x has 3 memberships'? (edited)

Max Woolf 13 days ago In my head, a membership represents the connection between a user and its "source". Who granted access? When? When does it expire? etc.

Max Woolf 13 days ago (I'm asking this question with a technical slant in mind, hoping that a non-technical answer will be the one I need!)

Sameer Kamani:family: 13 days ago How about “User X belongs to 3 Workspaces”?

Brian Wald 13 days ago Im not sure what term to use, but I dont think we should use “sources” - that term is already heavily used to mean different things (source repo, branch, MR, etc) :1000: 3

Max Woolf 13 days ago Excellent point @brian Wald

Brian Wald 13 days ago “entity” comes to mind as a more generic term, but also loaded term.

Brian Wald 13 days ago user is a member of 3 entities.. 1 group and 3 projects.

nick:fist::skin-tone-6: 13 days ago we ran into this with repositories a little while back, where snippets were also a concern. we went with 'container', but it wasn't a particularly happy choice, and it's something we're committed to removing the need for in any event

nick:fist::skin-tone-6: 13 days ago my preference is to avoid the need for an abstraction that can cover such different items, I think

Max Woolf 13 days ago I'm slowly coming round to that conclusion myself I think @nick. 🤔

Max Woolf 13 days ago Thanks anyway folks, this has given me lots to chew on either way 🙂

nick:fist::skin-tone-6: 13 days ago what are the crucial items you're pulling out of both groups and projects?

nick:fist::skin-tone-6: 13 days ago If it's something like a Pathable, that might be quite reasonable

Simon Mansfield:spiral_calendar_pad: 13 days ago @max Woolf makes me wonder if we should ditch the abstraction and simply go with: user { projects {} groups {} } (simplified for clarity) (edited)

nick:fist::skin-tone-6: 13 days ago if what it might be used for is a wide range of things, it's pretty difficult to stop it from being a very leaky abstraction

Max Woolf 13 days ago Indeed @simon Mansfield. The issue itself only requires the global ID for each object. Nothing else (edited)

Max Woolf 13 days ago So maybe the abstraction isn't required anyway.

Simon Mansfield:spiral_calendar_pad: 13 days ago If it's causing this much headache naming it, I'd go separates. You can always add another field later on to User that encompasses all, IF/WHEN there is agreement on naming / it makes sense to do so. (And it wouldn't be a breaking change) (edited)

Max Woolf 13 days ago Indeed. I'll need to square it with the PM for the feature who was looking specifically for a memberships array in the API output. But I think would be amenable to having them separate if it meant for a better developer experience in general.

Simon Mansfield:spiral_calendar_pad: 13 days ago I just think that it's possibly breaking the principles, it's a very vague term, representing an implementation detail (a JOIN).

Max Woolf 13 days ago The fact that I can't think of a good name for it is probably indicative of the fact that I shouldn't be trying to find a good name. :plus-one: 2 👍 2

Sameer Kamani:family: 13 days ago I have also found that in some cases a better name for something may exist in a different language. If something takes two words to explain something correctly, I suppose we should consider that to be the way. :clapping_inclusive: 1

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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
Edited by Dan Jensen

Merge request reports