Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLAlchemy: Add support for 'autogenerate_uuid' in column creation #580

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Changes for crate
Unreleased
==========

- Add support for CrateDB's gen_random_text_uuid in SQLAlchemy column generation ORM
- Properly handle Python-native UUID types in SQL parameters
- SQLAlchemy: Fix handling URL parameters ``timeout`` and ``pool_size``
- Permit installation with urllib3 v2, see also `urllib3 v2.0 roadmap`_
Expand Down
3 changes: 3 additions & 0 deletions docs/sqlalchemy.rst
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ system <sa:orm_declarative_mapping>`:
... quote_ft = sa.Column(sa.String)
... even_more_details = sa.Column(sa.String, crate_columnstore=False)
... created_at = sa.Column(sa.DateTime, server_default=sa.func.now())
... uuid = sa.Column(sa.String, crate_autogenerate_uuid=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't id = sa.Column(sa.String, primary_key=True, server_default=func.gen_random_text_uuid()) already work? Why do we need a custom property?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are having a valid point here. Maybe the crate_autogenerate_uuid should be a table- or dialect-level parameter instead, to cover situations like outlined in GH-575, which is a slightly different topic, and can be deferred to another patch.

If the user can write their own model definitions, your proposal to use server_default=func.gen_random_text_uuid() indeed would be completely satisfactory.

So, the outcome for this topic could be essentially to cover that detail about func.gen_random_text_uuid() within the documentation and software tests at

  • docs/sqlalchemy.rst
  • src/crate/client/sqlalchemy/tests/create_table_test.py

and not add any specific functionality for this on the column level. Apologies that GH-533 was giving wrong guidance here.

...
... __mapper_args__ = {
... 'exclude_properties': ['name_ft', 'quote_ft']
Expand All @@ -230,6 +231,8 @@ In this example, we:
the mapping (so SQLAlchemy doesn't try to update them as if they were columns)
- Disable the columnstore of the ``even_more_details`` column using ``crate_columnstore=False``
- Add a ``created_at`` column whose default value is set by CrateDB's ``now()`` function.
- Add a ``uuid`` column whose default value is generated by CrateDB's ``gen_random_text_uuid``
can also be used with ``primary_key=True`` as an alternative of ``default=gen_key``

.. TIP::

Expand Down
16 changes: 16 additions & 0 deletions src/crate/client/sqlalchemy/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,22 @@ def get_column_specification(self, column, **kwargs):
if column.computed is not None:
colspec += " " + self.process(column.computed)

if column.dialect_options['crate'].get('autogenerate_uuid'):
if not isinstance(column.type, (sa.String, sa.Text)):
raise sa.exc.CompileError(
"Auto generate uuid is only supported for column"
"types STRING and TEXT"
)
if default is not None:
raise sa.exc.CompileError(
"Can only have one default value but server_default"
" and crate_generate_uuid are set"
)

colspec += " DEFAULT gen_random_text_uuid()"

# nullable should go after 'DEFAULT' query modifications, otherwise
# invalid statements will be generated, ex: `x STRING NOT NULL DEFAULT 'value'`
if column.nullable is False:
colspec += " NOT NULL"
elif column.nullable and column.primary_key:
Expand Down
33 changes: 33 additions & 0 deletions src/crate/client/sqlalchemy/tests/create_table_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -311,3 +311,36 @@
'pk STRING NOT NULL, \n\t'
'answer INT DEFAULT 42, \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_column_autogenerate_uuid(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable Note test

Variable DummyTable is not used.
Copy link
Member

@amotl amotl Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@amotl amotl Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha. @aibaars just mentioned at github/codeql#11427 (comment):

The advanced-security/dismiss-alerts Action implements support for inline suppression comments and automatically dismisses alerts in GitHub Code Scanning.

Thanks! We may want to give it a shot, depending on mood and time.

Copy link
Member

@amotl amotl Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. Using # codeql[py/unused-local-variable] on line 316 and friends might work already, or not.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they should work, and plain #noqa should work too. Just make sure to run AlertSuppression.ql in addition to the normal CodeQL rules.

__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True, crate_autogenerate_uuid=True)
other = sa.Column(sa.Boolean)

self.Base.metadata.create_all(bind=self.engine)
fake_cursor.execute.assert_called_with(
'\nCREATE TABLE t (\n\t'
'pk STRING DEFAULT gen_random_text_uuid() NOT NULL, \n\t'
'other BOOLEAN, \n\tPRIMARY KEY (pk)\n)\n\n', ()
)

def test_column_autogenerate_uuid_correct_type(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable Note test

Variable DummyTable is not used.
__tablename__ = 't'
pk = sa.Column(sa.Integer, primary_key=True, crate_autogenerate_uuid=True)
other = sa.Column(sa.Boolean)

with self.assertRaises(sa.exc.CompileError):
self.Base.metadata.create_all(bind=self.engine)

def test_column_autogenerate_uuid_clashes_with_server_default(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable Note test

Variable DummyTable is not used.
__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True,
crate_autogenerate_uuid=True,
server_default='value')
Comment on lines +341 to +342
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, using both crate_autogenerate_uuid and server_default at the same time. Yeah, that may want to produce a failure, no? What do you think, @seut?

other = sa.Column(sa.Boolean)

with self.assertRaises(sa.exc.CompileError):
self.Base.metadata.create_all(bind=self.engine)