fix(api): use ID instead of name for GroupLabel & ProjectLabel _id_attr

[ptalbert]

Changes

_id_attr is used when comparing RESTObject objects. Labels will be seen as equal when they have the same name even if they are from different groups or projects.

Instead, use the label's ID as the _id_attr so that comparisions will only be equal if they truly represent the same gitlab label object.

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

• Documentation in the matching docs section
• Unit tests and/or functional tests

[ptalbert]

This change makes sense to me but maybe the original behaviour is preferred for some reason.

I stumbled upon this when fetching all the labels for a number of groups and projects. I put them in a set() and ended up with a lot less items than I expected.

[nejch]

This change makes a lot of sense, thanks @ptalbert. For context, we didn't use to have _repr_attr so this may have some historical reason but I'm not entirely sure at the moment.

I think this might be a breaking change though, as people might be comparing labels for uniqueness, so I also wonder if we should be using iid rather than id, assuming it actually exists for labels (will have to check).

[ptalbert]

https://docs.gitlab.com/ee/api/labels.html only shows an id field, no iid.

I see the functional tests for groups and projects broke. I don't have docker so I cannot immediately run those tests locally. I will try to get something working.

[ptalbert]

Quite a few objects use name as their _id_attr :/

python-gitlab (main)$ grep -nrB1 '_id_attr = "name"' gitlab/
gitlab/v4/objects/branches.py-21-class ProjectBranch(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/branches.py:22:    _id_attr = "name"
--
gitlab/v4/objects/branches.py-37-class ProjectProtectedBranch(SaveMixin, ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/branches.py:38:    _id_attr = "name"
--
gitlab/v4/objects/container_registry.py-35-class ProjectRegistryTag(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/container_registry.py:36:    _id_attr = "name"
--
gitlab/v4/objects/environments.py-62-class ProjectProtectedEnvironment(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/environments.py:63:    _id_attr = "name"
--
gitlab/v4/objects/features.py-19-class Feature(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/features.py:20:    _id_attr = "name"
--
gitlab/v4/objects/groups.py-430-class GroupSAMLGroupLink(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/groups.py:431:    _id_attr = "name"
--
gitlab/v4/objects/tags.py-15-class ProjectTag(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/tags.py:16:    _id_attr = "name"
--
gitlab/v4/objects/tags.py-32-class ProjectProtectedTag(ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/tags.py:33:    _id_attr = "name"
--
gitlab/v4/objects/templates.py-18-class Dockerfile(RESTObject):
gitlab/v4/objects/templates.py:19:    _id_attr = "name"
--
gitlab/v4/objects/templates.py-30-class Gitignore(RESTObject):
gitlab/v4/objects/templates.py:31:    _id_attr = "name"
--
gitlab/v4/objects/templates.py-42-class Gitlabciyml(RESTObject):
gitlab/v4/objects/templates.py:43:    _id_attr = "name"
--
gitlab/v4/objects/labels.py-25-class GroupLabel(SubscribableMixin, SaveMixin, ObjectDeleteMixin, RESTObject):
gitlab/v4/objects/labels.py:26:    _id_attr = "name"
--
gitlab/v4/objects/labels.py-89-):
gitlab/v4/objects/labels.py:90:    _id_attr = "name"

And there is a similar issue with ProjectMergeRequest objects. The _id_attr is iid. This means if you compare two MRs from completely different projects they will be seen as equal if they happen to have the same IID value.

Looks like epics and issues also suffer:

python-gitlab (main)$ grep -nr '_id_attr = "iid"' gitlab/
gitlab/v4/objects/epics.py:29:    _id_attr = "iid"
gitlab/v4/objects/issues.py:117:    _id_attr = "iid"
gitlab/v4/objects/merge_requests.py:155:    _id_attr = "iid"

I imagine most of these objects never get compared but you never know, and certainly a Merge Request might. It seems prudent to fix these.

[nejch]

@ptalbert thanks. In some cases name is intentional, for example when there is no id returned (like for branches). So if there are other cases where this should not be the case, they would need to be analyzed individually.

The iid vs id is a known issue, one solution would be to change the eq/hash dunder methods (#935).

[nejch]

@ptalbert I think for others it mostly makes sense to keep name as _id_attr (e.g. see protected environments => https://docs.gitlab.com/ee/api/protected_environments.html#get-a-single-protected-environment).

But for this one it makes sense probably, we just need to make sure the tests pass and add a breaking change trailer in your commit message so we ensure a major release for this. Let me know if you need any help.