Support setting roles in dbapi and sqlalchemy #212

mdesmet · 2022-08-02T07:27:23Z

fixes #230

README.md

tests/unit/sqlalchemy/test_dialect.py

tests/integration/test_dbapi_integration.py

trino/client.py

hovaesco · 2022-09-23T08:24:04Z

Please rebase on master to resolve CI failures.

mdesmet · 2022-09-23T10:40:30Z

@hashar: This would enable us to support grants in dbt-trino.

hashhar · 2022-09-23T11:38:43Z

trino/client.py

+    def _create_roles(self, roles: Optional[Dict[str, str]]) -> Optional[str]:
+        if roles is not None:
+            return ";".join(map(lambda role: f"{role[0]}={role[1]}", roles.items()))
+        return ""


why not return None? Is the header required even if no role is set?

hashhar · 2022-09-23T11:39:56Z

README.md

@@ -321,6 +321,20 @@ cur.execute('SELECT * FROM system.runtime.nodes')
 rows = cur.fetchall()
 ```

+## Roles
+
+Authorization roles to use for catalogs, specified as a dict with key-value pairs for the catalog and role. For example, `{"catalog1": "roleA", "catalog2": "roleB"}` sets roleA for catalog1 and roleB for catalog2.


Suggested change

Authorization roles to use for catalogs, specified as a dict with key-value pairs for the catalog and role. For example, `{"catalog1": "roleA", "catalog2": "roleB"}` sets roleA for catalog1 and roleB for catalog2.

Authorization roles to use for catalogs, specified as a dict with key-value pairs for the catalog and role. For example, `{"catalog1": "roleA", "catalog2": "roleB"}` sets `roleA` for `catalog1` and `roleB` for `catalog2`.

Also might be useful to link to Trino docs about roles since AFAIK there is also something called a system role.

There seems no specific page about roles, only about the SQL syntax. I think a proper page about how roles work in Trino will be definitely beneficial.

cc: @mosabua @dain regarding some discussion of what roles mean - we can just explain the concept because AFAIK there is no plugin providing roles in Trino - just the SPI.

tests/unit/test_dbapi.py

hashhar · 2022-09-23T11:43:11Z

README.md

@@ -108,7 +109,8 @@ engine = create_engine(
    'trino://user@localhost:8080/system?'
    'session_properties={"query_max_run_time": "1d"}'
    '&client_tags=["tag1", "tag2"]'
-    '&experimental_python_types=true',
+    '&experimental_python_types=true'
+    '&roles={"catalog1": "role1"}'


no test for sqlalchemy integration?

See #212 (comment)

mdesmet · 2022-09-27T11:29:39Z

@hashhar: Please advise on how to get this merged. Note that we had roles already supported (within the session) and they were tested in the same way (now expanded up to http_headers) but not in integration tests as we don't have a connector that supports roles in this project.

As mentioned we need this feature in as we want to support roles security within dbt_trino.

hashhar · 2022-09-28T06:58:46Z

I'm fine with current tests for now but as we start to work more on this area we'd eventually need better integration tests - let's create an issue with current ideas and we can follow up their.

@ebyhr WDYT?

hashhar · 2022-09-28T06:59:26Z

@mdesmet Can you also update PR description with the template + propose a release note?

hashhar · 2022-09-28T07:01:21Z

tests/integration/test_dbapi_integration.py

@@ -19,6 +19,7 @@

 import trino
 from tests.integration.conftest import trino_version
+from trino import constants


nit: Refactor role into roles -> Add support for system and connector roles?

Does it make sense (and is it accurate?)?

@hashhar: It is really just a rename of role into roles. There is no new feature being introduced in that commit actually. The role field actually already contained multiple roles. I thought this is more consistent with the roles being passed on from dbapi.connect, introduced in the next commit.

That's the reason why the first commit by itself fails tests. See

======================================================================================================================================= FAILURES ======================================================================================================================================== _____________________________________________________________________________________________________________________ test_set_role_in_connection_trino_higher_351 ______________________________________________________________________________________________________________________ run_trino = (<Popen: returncode: None args: ['docker', 'run', '--rm', '-p', '56257:8080',...>, 'localhost', 56257) @pytest.mark.skipif(trino_version() == '351', reason="Newer Trino versions return the system role") def test_set_role_in_connection_trino_higher_351(run_trino): _, host, port = run_trino > trino_connection = trino.dbapi.Connection( host=host, port=port, user="test", catalog="tpch", roles={"system": "ALL"} ) E TypeError: Connection.__init__() got an unexpected keyword argument 'roles' tests/integration/test_dbapi_integration.py:1077: TypeError ------------------------------------------------------------------------------------------------------------------------------- Captured stdout teardown -------------------------------------------------------------------------------------------------------------------------------- trino-python-client-tests-637e3b1 ____________________________________________________________________________________________________________________________ test_role_is_set_when_specified ____________________________________________________________________________________________________________________________ mock_client = <MagicMock name='client' id='4532039360'> @patch("trino.dbapi.trino.client") def test_role_is_set_when_specified(mock_client): roles = {"system": "finance"} > with connect("sample_trino_cluster:443", roles=roles) as conn: tests/unit/test_dbapi.py:262: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ args = ('sample_trino_cluster:443',), kwargs = {'roles': {'system': 'finance'}} def connect(*args, **kwargs): """Constructor for creating a connection to the database. See class :py:class:`Connection` for arguments. :returns: a :py:class:`Connection` object. """ > return Connection(*args, **kwargs) E TypeError: Connection.__init__() got an unexpected keyword argument 'roles' trino/dbapi.py:82: TypeError ================================================================================================================================ short test summary info ================================================================================================================================ FAILED tests/integration/test_dbapi_integration.py::test_set_role_in_connection_trino_higher_351 - TypeError: Connection.__init__() got an unexpected keyword argument 'roles' FAILED tests/unit/test_dbapi.py::test_role_is_set_when_specified - TypeError: Connection.__init__() got an unexpected keyword argument 'roles'

I probably messed up something in my fixup comments locally. All good now.

hashhar

LGTM % comments.

mdesmet · 2022-09-29T11:44:34Z

I'm fine with current tests for now but as we start to work more on this area we'd eventually need better integration tests - let's create an issue with current ideas and we can follow up their.

Created #241

hashhar · 2022-09-30T11:20:03Z

@mdesmet Can you rebase, I see some failures when running tests locally. Possibly something changed on master in the meantime.

stale

hashhar · 2022-09-30T14:25:22Z

Seems like some changes in post-397 affect query cancellations causing "most" of the test failures. The last successful run was against 397.

mdesmet · 2022-09-30T14:30:12Z

@hashhar : Unfortunately something fishy is going on: 351 and 395 pass. However 398 does fail consistently (also locally), while 397 works fine.

I checked the release notes but didn't immediately find something. I'll keep digging.

hashhar · 2022-09-30T14:33:09Z

See https://github.com/trinodb/trino/pull/13907/files#r984659618, a "bug" has been fixed - we now don't get result sets for non-SELECT queries like it should've been all along.

mdesmet · 2022-10-03T12:53:27Z

@hashhar: I've realigned the implementation on the session_properties and your changes in #242

It would be nice to get this one in as well.

hashhar · 2022-10-03T15:15:57Z

trino/client.py

+def get_roles_values(headers, header):
+    kvs = get_header_values(headers, header)
+    return [
+        (k.strip(), urllib.parse.unquote(v.strip()))


unquote_plus

cla-bot bot added the cla-signed label Aug 2, 2022

mdesmet requested a review from hovaesco August 2, 2022 07:27

mdesmet marked this pull request as draft August 12, 2022 11:49

mdesmet mentioned this pull request Sep 5, 2022

It's not possible to set role in trino.dbapi.connect method #230

Closed

1 task

mdesmet force-pushed the feature/role branch from 238b1fc to c5355a6 Compare September 16, 2022 13:30

mdesmet marked this pull request as ready for review September 16, 2022 14:08

mdesmet force-pushed the feature/role branch from c5355a6 to 2978889 Compare September 16, 2022 14:45

mdesmet changed the title ~~Support setting role in dbapi and sqlalchemy~~ Support setting roles in dbapi and sqlalchemy Sep 19, 2022

hovaesco reviewed Sep 19, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

tests/unit/sqlalchemy/test_dialect.py Outdated Show resolved Hide resolved

tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved

trino/client.py Outdated Show resolved Hide resolved

mdesmet force-pushed the feature/role branch from 2978889 to d37b0e7 Compare September 22, 2022 11:47

mdesmet requested a review from hovaesco September 23, 2022 07:36

mdesmet force-pushed the feature/role branch 2 times, most recently from 760f1a7 to 9b9c848 Compare September 23, 2022 08:50

hovaesco approved these changes Sep 23, 2022

View reviewed changes

hovaesco requested review from ebyhr and hashhar September 23, 2022 09:10

hashhar reviewed Sep 23, 2022

View reviewed changes

tests/unit/test_dbapi.py Outdated Show resolved Hide resolved

hashhar reviewed Sep 23, 2022

View reviewed changes

mdesmet force-pushed the feature/role branch from 9b9c848 to 7981f6a Compare September 27, 2022 11:28

hashhar reviewed Sep 28, 2022

View reviewed changes

hashhar approved these changes Sep 28, 2022

View reviewed changes

hashhar previously approved these changes Sep 30, 2022

View reviewed changes

mdesmet force-pushed the feature/role branch from 7981f6a to b31d541 Compare September 30, 2022 13:43

hashhar mentioned this pull request Sep 30, 2022

Fix trino-cli to display plans for EXPLAIN ANALYZE for update statements trinodb/trino#13907

Merged

mdesmet force-pushed the feature/role branch from b31d541 to e7a1052 Compare October 2, 2022 21:03

mdesmet requested review from hovaesco and hashhar October 3, 2022 12:52

hashhar reviewed Oct 3, 2022

View reviewed changes

mdesmet force-pushed the feature/role branch from e7a1052 to ce5c351 Compare October 3, 2022 16:42

mdesmet added 3 commits October 3, 2022 19:26

Refactor role into roles

d083123

Implement roles support in dbapi.connect()

d0da6cb

Implement roles support in sqlalchemy

88fa310

mdesmet force-pushed the feature/role branch from ce5c351 to 88fa310 Compare October 3, 2022 17:26

Use unquote_plus for session properties url decoding

1940c61

hashhar approved these changes Oct 4, 2022

View reviewed changes

hashhar merged commit b4bd746 into trinodb:master Oct 4, 2022

hashhar mentioned this pull request Oct 4, 2022

Add 0.317.0 release notes #238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting roles in dbapi and sqlalchemy #212

Support setting roles in dbapi and sqlalchemy #212

mdesmet commented Aug 2, 2022 •

edited

Loading

hovaesco commented Sep 23, 2022

mdesmet commented Sep 23, 2022

hashhar Sep 23, 2022

hashhar Sep 23, 2022

mdesmet Sep 23, 2022

hashhar Sep 28, 2022

hashhar Sep 23, 2022

mdesmet Sep 23, 2022

mdesmet commented Sep 27, 2022

hashhar commented Sep 28, 2022

hashhar commented Sep 28, 2022

hashhar Sep 28, 2022

mdesmet Sep 28, 2022 •

edited

Loading

hashhar Sep 30, 2022

mdesmet Sep 30, 2022

hashhar left a comment

mdesmet commented Sep 29, 2022 •

edited

Loading

hashhar commented Sep 30, 2022

hashhar commented Sep 30, 2022 •

edited

Loading

mdesmet commented Sep 30, 2022

hashhar commented Sep 30, 2022

mdesmet commented Oct 3, 2022

hashhar Oct 3, 2022

	Authorization roles to use for catalogs, specified as a dict with key-value pairs for the catalog and role. For example, `{"catalog1": "roleA", "catalog2": "roleB"}` sets roleA for catalog1 and roleB for catalog2.
	Authorization roles to use for catalogs, specified as a dict with key-value pairs for the catalog and role. For example, `{"catalog1": "roleA", "catalog2": "roleB"}` sets `roleA` for `catalog1` and `roleB` for `catalog2`.

Support setting roles in dbapi and sqlalchemy #212

Support setting roles in dbapi and sqlalchemy #212

Conversation

mdesmet commented Aug 2, 2022 • edited Loading

hovaesco commented Sep 23, 2022

mdesmet commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Sep 27, 2022

hashhar commented Sep 28, 2022

hashhar commented Sep 28, 2022

Choose a reason for hiding this comment

mdesmet Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

mdesmet commented Sep 29, 2022 • edited Loading

hashhar commented Sep 30, 2022

hashhar commented Sep 30, 2022 • edited Loading

mdesmet commented Sep 30, 2022

hashhar commented Sep 30, 2022

mdesmet commented Oct 3, 2022

Choose a reason for hiding this comment

mdesmet commented Aug 2, 2022 •

edited

Loading

mdesmet Sep 28, 2022 •

edited

Loading

mdesmet commented Sep 29, 2022 •

edited

Loading

hashhar commented Sep 30, 2022 •

edited

Loading