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

fix invisible null constraints in sql_table #1660

Merged
merged 1 commit into from
Oct 13, 2023
Merged

fix invisible null constraints in sql_table #1660

merged 1 commit into from
Oct 13, 2023

Conversation

landmaj
Copy link
Contributor

@landmaj landmaj commented Oct 12, 2023

Thix PR fixes issue #1655

Diagram

table: {
    shape: sql_table
    a: int {constraint: null}
    b: int {constraint: [TRUE; False; null; NULL]}
}

Before

before

After

after

Summary

The behavior of null, when used in arrays, is now consistent with how other types work. When used as the only constraint (without array), only types which implement d2ast.String are allowed. Null is also allowed but does nothing, as expected. In arrays all types are allowed but booleans are converted to lower case. Null was previously converted to an empty string, this PR changes it to lower case string "null".

This could also be fixed by changing return value of ScalarString() method for Null type. I tried it and all tests pass, however there is this commit: 9d0c73c

Another solution would be to disallow everything but d2ast.String in constraint arrays, to keep it consistent with how non-array constraints work, however this could break existing diagrams and prevent them from compiling. Let me know if you would prefer this solution.

@landmaj
Copy link
Contributor Author

landmaj commented Oct 12, 2023

BTW your new pipeline rejects SSH signatures on commits. Is this intended and I need to figure out GPG?

@gavin-ts
Copy link
Contributor

gavin-ts commented Oct 12, 2023

BTW your new pipeline rejects SSH signatures on commits. Is this intended and I need to figure out GPG?

this is not intended looking into it

Update: this should be fixed after rebasing on master with #1661

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

perfect PR. thank you @landmaj .

@alixander alixander merged commit 0c7a4b6 into terrastruct:master Oct 13, 2023
3 checks passed
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.

3 participants