Create a issue board list by milestone_id still depends on label_id

Description of the problem, including code/CLI snippet

I followed the API documentation which says that the usage of label_id, milestone_id and assignee_id is mutually exclusive. So I wanted to create a board which would relate on milestones to have a timeline. The function goes like:

def create_board(repository):
  milestones = repository.milestones.list()
  for milestone in milestones:
    print(f'ID: {milestone.id}')
    timeline_board.lists.create({
      'milestone_id': milestone.id
    })

Expected Behavior

Normally the board should be created according to the documentation.

Actual Behavior

AttributeError: Missing attributes: label_id

Specifications

python-gitlab==3.1.1
Gitlab server version (or gitlab.com): 14.2.4-ee

This is due to:

python-gitlab/gitlab/v4/objects/boards.py

Line 26 in bd1ecdd

_create_attrs = RequiredOptional(required=("label_id",))

So that would need to be changed. Probably should make it optional and also add the other optional values to the tuple.

That's exactly what I was going to post. I can do the changes if it's possible

@kurisukun

@kurisukun feel free to go ahead :)

IIUC this will technically also be incorrect since at least one of them is required. So (as a follow-up maybe) we could also introduce mutually exclusive/"requires one of" type of flow, ala Ansible:
https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#dependencies-between-module-options

@kurisukun

@kurisukun I've assigned this to you for now for the initial simple change as you expressed interest, feel free to unassign if you don't feel like working on this though :)

the same issue exists for creating assignee boards

this_board.lists.create({ 'assignee_id': user.id })

Traceback (most recent call last):
  File "/path/to/my/script.py", line 147, in <module>
    this_board.lists.create({ 'assignee_id': user.id })
  File "/usr/local/lib/python3.10/site-packages/gitlab/exceptions.py", line 311, in wrapped_f
    return f(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/gitlab/mixins.py", line 300, in create
    self._check_missing_create_attrs(data)
  File "/usr/local/lib/python3.10/site-packages/gitlab/mixins.py", line 276, in _check_missing_create_attrs
    raise AttributeError(f"Missing attributes: {', '.join(missing)}")
AttributeError: Missing attributes: label_id

There is explicit reference to being able to create assignee and milestone lists in the API docs.

https://gitlab.com/gitlab-org/gitlab/-/issues/6397

https://docs.gitlab.com/ee/api/boards.html#create-a-board-list

Attribute	Type	Required	Description
id	integer/string	yes	The ID or URL-encoded path of the project owned by the authenticated user
board_id	integer	yes	The ID of a board
label_id	integer	no	The ID of a label
assignee_id	integer	no	The ID of a user
milestone_id	integer	no	The ID of a milestone

@kurisukun

@kurisukun feel free to go ahead :)

IIUC this will technically also be incorrect since at least one of them is required. So (as a follow-up maybe) we could also introduce mutually exclusive/"requires one of" type of flow, ala Ansible:

https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#dependencies-between-module-options

@nejch .. where would we implement mutually exclusive/requires? argparse has add_mutually_exclusive_group(required=True) which does exactly how it sounds. The args are mutually exclusive, but one of them is required.

https://docs.python.org/3/library/argparse.html#mutual-exclusion

@walterrowe

@walterrowe thanks for looking into this! For an initial fix, I think we'd be happy with first just making the arguments optional and let the API response guide the user (the user will get a 4xx response with the details I believe).

However if you're willing to work on a fix in the code itself, that'd be awesome :) We do already make some limited use of this in our CLI, see:

python-gitlab/gitlab/cli.py

Lines 229 to 246 in 387a140

    
           tokens = parser.add_mutually_exclusive_group() 
        
           tokens.add_argument( 
        
               "--private-token", 
        
               help=("GitLab private access token [env var: GITLAB_PRIVATE_TOKEN]"), 
        
               required=False, 
        
               default=os.getenv("GITLAB_PRIVATE_TOKEN"), 
        
           ) 
        
           tokens.add_argument( 
        
               "--oauth-token", 
        
               help=("GitLab OAuth token [env var: GITLAB_OAUTH_TOKEN]"), 
        
               required=False, 
        
               default=os.getenv("GITLAB_OAUTH_TOKEN"), 
        
           ) 
        
           tokens.add_argument( 
        
               "--job-token", 
        
               help=("GitLab CI job token [env var: CI_JOB_TOKEN]"), 
        
               required=False, 
        
           )

However, we would need to handle this in the API client itself, not just the CLI wrapper, and I'm not sure we should bring in argparse as I'd like to make the API independent of CLI-focused libraries. One place to store mutually exclusive args would be to extend our RequiredOptional class:

python-gitlab/gitlab/base.py

Lines 333 to 336 in 387a140

    
           @dataclass(frozen=True) 
        
           class RequiredOptional: 
        
               required: Tuple[str, ...] = () 
        
               optional: Tuple[str, ...] = ()

Currently, we validate args in _check_missing_create_attrs and _check_missing_update_attrs, so this would probably be the place to do it, but refactoring that might also make sense :)

@nejch

@nejch I agree.

add an exclusive attr to RequiredOptional class

@dataclass(frozen=True)
class RequiredOptional:
    required: Tuple[str, ...] = ()
    optional: Tuple[str, ...] = ()
    exclusive: Tuple[str, ...] = ()

refactor _check_missing_create_attrs and _check_missing_update_attrs
perhaps rename _check_missing_create_attrs and _check_missing_update_attrs (remove _missing from the name)?

Then in places like boards.pyL26 we would see:

    _create_attrs = RequiredOptional(exclusive=("label_id", "assignee_id", "milestone_id"))

I don't want to fork and work on it until we agree on direction.

@walterrowe

@walterrowe that sounds good!

add an exclusive attr to RequiredOptional class

@dataclass(frozen=True)
class RequiredOptional:
    required: Tuple[str, ...] = ()
    optional: Tuple[str, ...] = ()
    exclusive: Tuple[str, ...] = ()

I think we might even need a list/tuple of tuples here, because there could be several sets of mutually exclusive attributes for busier endpoints, but I'm not 100%.

refactor _check_missing_create_attrs and _check_missing_update_attrs

perhaps rename _check_missing_create_attrs and _check_missing_update_attrs (remove _missing from the name)?

Sounds great. Another option, since these two are mostly just copies of each other with the id removed for update, would be to extract them into a common validate_attrs helper with an optional exclude argument that can be supplied to ignore certain elements, if that makes sense. This would also make it useful later if we need to do the same for get() or list() filters.

@nejch

@nejch took a simple stab at this for _check_missing_create_attrs()

    def _check_missing_create_attrs(self, data: Dict[str, Any]) -> None:
        missing = [ attr for attr in self._create_attrs.required if attr not in data ]
        if len(missing) > 0:
            raise AttributeError(f"Missing attributes: {', '.join(missing)}")
        exclusives = [ attr for attr in data if attr in self._create_attrs.exclusive ]
        if len(exclusives) > 1:
            raise AttributeError(f"Provide only one of these attributes: {', '.join(exclusives)}")
        if len(exclusives) == 0:
            raise AttributeError(f"Must provide one of these attributes: {', '.join(self._create_attrs.exclusive)}")

If exclusives has more than 1 item we raise an error on mutually exclusive attrs.
If exclusives is empty, we raise an error on missing attrs.

Does this achieve the desired effect?

I'm thinking about your proposal for a common validate_attrs(). That seems to make a lot of sense on the surface.

@nejch

@nejch - is this what you are thinking for a common _validate_attrs() .. ?

How do we discern whether to reference _update_attrs vs _create_attrs? Any direction on this would be helpful in pushing this forward.

    def _validate_attrs(self, data: Dict[str, Any], excludes=[]) -> None:
        if TYPE_CHECKING:
            assert self._obj_cls is not None
        # Remove the id field from the required list as it was previously moved
        # to the http path.
        required = tuple(
            [k for k in self._update_attrs.required if k != excludes]
        )
        missing = [ attr for attr in required if attr not in data ]
        if len(missing) > 0:
            raise AttributeError(f"Missing attributes: {', '.join(missing)}")
        exclusives = [ attr for attr in data if attr in self._create_attrs.exclusive ]
        if len(exclusives) > 1:
            raise AttributeError(f"Provide only one of these attributes: {', '.join(exclusives)}")
        if len(exclusives) == 0:
            raise AttributeError(f"Must provide one of these attributes: {', '.join(self._create_attrs.exclusive)}")

@walterrowe

@walterrowe I was thinking we could supply attributes explicitly as an argument, e.g.:

def _validate_attrs(
        self,
        data: Dict[str, Any],
        attributes: RequiredOptional,
        excludes: Optional[list] = None
    ) -> None:
    if excludes is None:
        excludes = []

    required = [k for k in attributes.required if k not in excludes]
    missing = [attr for attr in required if attr not in data]

    if missing:
        raise AttributeError(f"Missing attributes: {', '.join(missing)}")
    
    exclusives = [attr for attr in data if attr in attributes.exclusive]
    if len(exclusives) > 1:
        raise AttributeError(f"Provide only one of these attributes: {', '.join(exclusives)}")
    if not exclusives:
        raise AttributeError(f"Must provide one of these attributes: {', '.join(attributes.exclusive)}")

So generally that looks good but I'm also thinking there might be some completely optional but mutually exclusive parameters, in which case we'd need to change this logic to simply check both required and optional data provided against the lists of mutually exclusive groups.

We need to check the upstream API docs first though to see where this appears, e.g. https://docs.gitlab.com/search/?query=Mutually%20exclusive, I'll take a look today as well.

Ah .. would _check_update_attrs and _check_create_attrs call this and pass in the requisite attributes and excludes lists?

@nejch

@nejch

trying to get a sense of right place to do this

create _validate_attrs() in the parent manager class in base.py
in mixin.py, call it from _check_missing_create_attrs() in the child class CreateMixin and _check_missing_update_attrs() in child class UpdateMixin.

this introduces the least change and implements it once. class inheritance is new to me so trying to get a handle on how this should be done.

in base.py RequiredOptional dataclass class add the exclusive attribute:

@dataclass(frozen=True)
class RequiredOptional:
    required: Tuple[str, ...] = ()
    optional: Tuple[str, ...] = ()
    exclusive: Tuple[str, ...] = ()

in base.py in class RESTManager we add:

def _validate_attrs(
        self,
        data: Dict[str, Any],
        attributes: RequiredOptional,
        excludes: Optional[list] = None
    ) -> None:
    if excludes is None:
        excludes = []

    required = [k for k in attributes.required if k not in excludes]
    missing = [attr for attr in required if attr not in data]

    if missing:
        raise AttributeError(f"Missing attributes: {', '.join(missing)}")
    
    exclusives = [attr for attr in data if attr in attributes.exclusive]
    if len(exclusives) > 1:
        raise AttributeError(f"Provide only one of these attributes: {', '.join(exclusives)}")
    if not exclusives:
        raise AttributeError(f"Must provide one of these attributes: {', '.join(attributes.exclusive)}")

in mixin.py we change _check_missing_create_attrs() to:

    def _check_missing_create_attrs(self, data: Dict[str, Any]) -> None:
        self._validate_attrs(data, attributes=self._create_required)

in mixin.py we change _check_update_create_attrs() to:

    def _check_missing_update_attrs(self, data: Dict[str, Any]) -> None:
        if TYPE_CHECKING:
            assert self._obj_cls is not None
        excludes = [ self._obj_cls._id_attr ]
        self._validate_attrs(data, attributes=self._update_required, excludes=excludes)

in gitlab/v4/objects/boards.py we change GroupBoardListManager class _create_attrs as follows:

    _create_attrs = RequiredOptional(exclusive=("label_id","assignee_id","milestone_id"))

as I said class inheritance is new to me so looking for some more seasoned guidance on proper syntax, etc.

@nejch

@nejch - I spent quite a bit of time reading Python docs and perusing base.py and mixin.py to get up to speed on how all of this is glued together. I feel pretty confident about my proposal above. I have a fork of python-gitlab with local changes I have not yet committed. Let me know if you agree with the above, let me know what else I should include (doc updates?), etc, and I will commit, push, and submit a merge request.

This doesn't handle the more complex case of multiple mutually exclusive sets of params. I think that can come later. We have people waiting on this specific enhancement.

Here is a proposed commit message:

add support for mutually exclusive attributes, consolidate attribute validation, fix boards.py _create_attr - closes #1897

Probably will need to be something like this:

feat: support mutually exclusive attributes, consolidate attribute validation, fix boards.py _create_attr

A paragraph or two giving more details

Closes: #1897

@JohnVillalovos

Thanks @JohnVillalovos!

@walterrowe I'd say it's even simpler, you can remove the _check* methods completely and just have a normal function in utils.py that gets imported in client.py. This will have the argument passed explicitly instead of relying on self..

I think you can go ahead and open a PR, it's actually easier to do code review once we have something pushed ;)

feat: support mutually exclusive attributes and consolidate validatio…

Navigation Menu

Repository navigation

Create a issue board list by milestone_id still depends on label_id #1897

Description of the problem, including code/CLI snippet

Expected Behavior

Actual Behavior

Specifications

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	tokens = parser.add_mutually_exclusive_group()
	tokens.add_argument(
	"--private-token",
	help=("GitLab private access token [env var: GITLAB_PRIVATE_TOKEN]"),
	required=False,
	default=os.getenv("GITLAB_PRIVATE_TOKEN"),
	)
	tokens.add_argument(
	"--oauth-token",
	help=("GitLab OAuth token [env var: GITLAB_OAUTH_TOKEN]"),
	required=False,
	default=os.getenv("GITLAB_OAUTH_TOKEN"),
	)
	tokens.add_argument(
	"--job-token",
	help=("GitLab CI job token [env var: CI_JOB_TOKEN]"),
	required=False,
	)

	@dataclass(frozen=True)
	class RequiredOptional:
	required: Tuple[str, ...] = ()
	optional: Tuple[str, ...] = ()

Create a issue board list by milestone_id still depends on label_id #1897

Description

Description of the problem, including code/CLI snippet

Expected Behavior

Actual Behavior

Specifications

Activity

JohnVillalovos commented on Feb 22, 2022

kurisukun commented on Feb 22, 2022

nejch commented on Mar 31, 2022

nejch commented on Apr 4, 2022

walterrowe commented on May 28, 2022

walterrowe commented on May 29, 2022

nejch commented on May 29, 2022

walterrowe commented on May 29, 2022

nejch commented on May 29, 2022

walterrowe commented on May 29, 2022

walterrowe commented on May 29, 2022

nejch commented on May 29, 2022

walterrowe commented on May 29, 2022

walterrowe commented on May 30, 2022

walterrowe commented on May 30, 2022

walterrowe commented on May 30, 2022

JohnVillalovos commented on May 30, 2022

nejch commented on May 30, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions