tableau / server-client-python

A Python library for the Tableau Server REST API
https://tableau.github.io/server-client-python/
MIT License
660 stars 421 forks source link

Bug - Virtual Connection default permissions #1525

Open TrimPeachu opened 2 weeks ago

TrimPeachu commented 2 weeks ago

Describe the bug In #1463 I've added support for virtual connection project permissions. I've now realized that they don't work.

Default virtualconnection permissions have been defined in project_item as follows:

self._default_virtualconnection_permissions = None
......
@property
def default_virtualconnection_permissions(self):
    if self._default_virtualconnection_permissions is None:
        raise UnpopulatedPropertyError(self.ERROR_MSG)
    return self._default_virtualconnection_permissions()

While populating permissions, we use Resource.VirtualConection referencing models/tableau_types

@api(version="3.23")
def populate_virtualconnection_default_permissions(self, item: ProjectItem) -> None:
    self._default_permissions.populate_default_permissions(item, Resource.VirtualConnection)
class Resource:
    ...
    VirtualConnection = "virtualConnection"

Issue occurs while populating the permissions, as _set_default_permissions function tries to set the attribute _default_virtualConnection_permissions instead of _default_virtualconnection_permissions

def _set_default_permissions(self, permissions, content_type):
    attr = f"_default_{content_type}_permissions"
    setattr(
        self,
        attr,
        permissions,
    )

Possible solutions:

  1. Change virtualConnection definition in models/tableautypes.py to virtualconnection - could this be a breaking change for sth?_
  2. Change default_virtualconnection_permissions to default_virtualConnection_permissions - snake case / camel case hybrid? not a good look
  3. Change attr = f"_default_{content_type}_permissions" so it uses only lowercase

I've created the issue instead of directly raising a PR as I am not sure which one of these changes would be prefered

jorwoods commented 3 days ago

My opinion is option 3. I agree that mixing camel and snake case is visually unappealing, inconsistent, and possibly confusing later.