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

surister · 2023-09-17T18:16:25Z

Where by setting the column:

Column('x', sa.Text, primary_key=True, crate_autogenerate_uuid=True)

will generate:

x STRING DEFAULT gen_random_text_uuid() NOT NULL

Checklist

CLA is signed
Docs
Tests

amotl · 2023-09-17T18:31:17Z

src/crate/client/sqlalchemy/tests/create_table_test.py

@@ -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):


Please forget about those, they are just false positives, and can neither be suppressed, nor remedied otherwise.

References

[False positive] py/unused-local-variable on SQLAlchemy model definition classes github/codeql#11407

Conversation opened by @github-code-scanning bot does not resolve/collapse properly github/codeql-action#1411

CodeQL is missing an inline mechanism to suppress warnings github/codeql#11427

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.

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

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

src/crate/client/sqlalchemy/tests/create_table_test.py

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


src/crate/client/sqlalchemy/tests/create_table_test.py

+            self.Base.metadata.create_all(bind=self.engine)
+
+    def test_column_autogenerate_uuid_clashes_with_server_default(self):
+        class DummyTable(self.Base):


surister · 2023-09-17T18:29:04Z

Circling back to this, I'm effectively checking against

Column(..., server_default='value')

with

if default is not None:
    raise sa.exc.CompileError(
            "Can only have one default value but server_default"
            " and crate_generate_uuid are set"
               )

But It'd probably also clash with Column(..., default=lambda:str(uuid.uuid4()))

I will have a deeper look at this.

amotl

Hi @surister,

thanks for your patch. It looks good to me so far.

Regarding your question about option clashes: I don't know if and how SQLAlchemy prevents to use both default and server_default, or if it doesn't do at all.

Depending on how SQLAlchemy handles that, we should not necessarily add anything on top - only if it makes sense, and doesn't overcomplicate the code.

With kind regards,
Andreas.

amotl · 2023-09-18T07:35:27Z

src/crate/client/sqlalchemy/tests/create_table_test.py

+                           crate_autogenerate_uuid=True,
+                           server_default='value')


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?

surister · 2023-09-18T07:50:52Z

Hi @surister,

thanks for your patch. It looks good to me so far.

Regarding your question about option clashes: I don't know if and how SQLAlchemy prevents to use both default and server_default, or if it doesn't do at all.

Depending on how SQLAlchemy handles that, we should not necessarily add anything on top - only if it makes sense, and doesn't overcomplicate the code.

With kind regards, Andreas.

Using default + crate_autogenerate_uuid doesn't really 'clash', it seems that CrateDB just ignores the DEFAULT directive and puts in whatever value it gets on the INSERT

mfussenegger · 2023-09-18T08:25:29Z

docs/sqlalchemy.rst

@@ -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)


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?

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.

surister · 2023-09-18T08:43:56Z

More importantly when using it as:

Column(..., crate_autogenerate_uuid=True, primary_key=True)

We get the following warning:

SAWarning: Column 'aaaa3.x' is marked as a member of the primary key for table 'aaaa3', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True', and no explicit value is passed.  Primary key columns typically may not store NULL.

Because technically, there is no autoincrement_column and server_default nor default is set, so SQLAlchemy cannot assure that the value will not be Null (and nullable in this case is set to False by default)

SQLAlchemy warning source

amotl · 2023-09-28T19:05:28Z

Dear @surister,

So, the outcome for this topic could be essentially to cover that detail about func.gen_random_text_uuid() within the documentation at, for example, docs/sqlalchemy.rst.

I've just submitted #585, so I am closing this. Apologies again that I was giving wrong guidance on this matter through crate/sqlalchemy-cratedb#102.

With kind regards,
Andreas.

SQLAlchemy: Add support for 'autogenerate_uuid' in column creation

8790924

github-advanced-security bot found potential problems Sep 17, 2023

View reviewed changes

amotl mentioned this pull request Sep 18, 2023

SQLAlchemy: Polyfill for AUTOINCREMENT columns crate/sqlalchemy-cratedb#77

Open

amotl approved these changes Sep 18, 2023

View reviewed changes

amotl reviewed Sep 18, 2023

View reviewed changes

amotl requested a review from seut September 18, 2023 07:35

mfussenegger reviewed Sep 18, 2023

View reviewed changes

amotl self-requested a review September 18, 2023 10:20

amotl mentioned this pull request Sep 28, 2023

Documentation: Add section about using gen_random_text_uuid as auto-PK #585

Merged

amotl closed this Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

surister commented Sep 17, 2023

amotl Sep 17, 2023 •

edited

Loading

amotl Sep 17, 2023 •

edited

Loading

amotl Sep 17, 2023 •

edited

Loading

aibaars Sep 18, 2023

surister commented Sep 17, 2023

amotl left a comment

amotl Sep 18, 2023

surister commented Sep 18, 2023

mfussenegger Sep 18, 2023

amotl Sep 18, 2023

surister commented Sep 18, 2023

amotl commented Sep 28, 2023

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

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

Conversation

surister commented Sep 17, 2023

Checklist

amotl Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

References

amotl Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

amotl Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

aibaars Sep 18, 2023

Choose a reason for hiding this comment

surister commented Sep 17, 2023

amotl left a comment

Choose a reason for hiding this comment

amotl Sep 18, 2023

Choose a reason for hiding this comment

surister commented Sep 18, 2023

mfussenegger Sep 18, 2023

Choose a reason for hiding this comment

amotl Sep 18, 2023

Choose a reason for hiding this comment

surister commented Sep 18, 2023

amotl commented Sep 28, 2023

amotl Sep 17, 2023 •

edited

Loading

amotl Sep 17, 2023 •

edited

Loading

amotl Sep 17, 2023 •

edited

Loading