Skip to content

fix generated boolean default value in mssql#10

Merged
hunyadi merged 6 commits into
masterfrom
mssql_bool_default_literal
Jul 11, 2025
Merged

fix generated boolean default value in mssql#10
hunyadi merged 6 commits into
masterfrom
mssql_bool_default_literal

Conversation

@szro0
Copy link
Copy Markdown
Collaborator

@szro0 szro0 commented Jul 10, 2025

Hi Levente,

I have created a possible fix for a bug or not-yet-implemented feature.

Issue: When a Python class contains a boolean field with a default value, e.g. boolean_false: bool = False or nullable_boolean_true: Optional[bool] = True then the CREATE TABLE statement generated by pysqlsync for MSSQL contains an invalid syntax:

  • Before:
    • "boolean_false" bit NOT NULL CONSTRAINT "df_boolean_false" DEFAULT FALSE -- invalid in MSSQL
    • "nullable_boolean_true" bit CONSTRAINT "df_nullable_boolean_true" DEFAULT TRUE -- invalid in MSSQL
  • After my fix:
    • "boolean_false" bit NOT NULL CONSTRAINT "df_boolean_false" DEFAULT 0 -- valid in MSSQL
    • "nullable_boolean_true" bit CONSTRAINT "df_nullable_boolean_true" DEFAULT 1 -- valid in MSSQL

Other dialects (PostgreSQL etc) are fine.

The implementation I created is my best effort understanding and extending the code base, please advise how it could be made even better.

Thanks, Robert

@szro0 szro0 requested a review from hunyadi July 10, 2025 17:13
@szro0 szro0 marked this pull request as ready for review July 10, 2025 17:51
Copy link
Copy Markdown
Owner

@hunyadi hunyadi left a comment

Choose a reason for hiding this comment

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

Thanks for identifying this bug with Boolean default values in Microsoft SQL Server! I did not spot any deal-breaker issues, I am happy to merge tomorrow. Nonetheless, I have proposed some suggestions to make the code easier to read, and better convey the developer's intent.

Comment thread pysqlsync/formation/py_to_sql.py Outdated
Comment thread pysqlsync/model/data_types.py Outdated
Comment thread pysqlsync/dialect/mssql/data_types.py Outdated
Comment thread pysqlsync/dialect/postgresql/discovery.py Outdated
Copy link
Copy Markdown
Owner

@hunyadi hunyadi left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Drop me a message if you need a new version released to PyPI with the changes included.

Comment thread pysqlsync/dialect/mssql/data_types.py Outdated
Comment on lines +29 to +37
if value is True:
return "1"

if value is False:
return "0"

raise NotImplementedError(
f"Cannot convert boolean value to SQL literal: {value}"
)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

A more elegant approach to this problem is to test for valid type first:

if not isinstance(value, bool):
    raise TypeError(f"expected: value of type `bool`, got: {type(value)}")

This allows type checkers to infer that value is either True or False in subsequent code (the only allowed values for type bool).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Cool thanks, done.

@hunyadi hunyadi merged commit a57c877 into master Jul 11, 2025
7 checks passed
@hunyadi hunyadi deleted the mssql_bool_default_literal branch July 11, 2025 13:27
@szro0
Copy link
Copy Markdown
Collaborator Author

szro0 commented Jul 14, 2025

Thank you very much Levente, I'm still testing some use cases, I'll let you know when you can go ahead with the pypi release.

@szro0
Copy link
Copy Markdown
Collaborator Author

szro0 commented Jul 14, 2025

@hunyadi, I found an issue still in MSSQL; the DEFAULT-constraint-name is not table-specific which prevents the creation of multiple tables with the same column-name and those columns having default values. I'm looking for a possible solution. Please don't release a new PyPI version until this gets resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants