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

DM-43998: Properly validate default values and fix their handling in SQL metadata #61

Merged
merged 3 commits into from Apr 26, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Apr 23, 2024

This fixes the handling of the value field when creating SQLAlchemy metadata. String literals are quoted, except when the value is a SQL function such as CURRENT_TIMESTAMP. In this case, quotes are omitted.

A thorough validation function was also added to the data model for checking value if it was provided. This will make sure that the provided value is valid given the datatype of the column.

Finally, a new test method was added to check the validation function for all of the Felis datatypes, both when valid and invalid data is provided.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-43998 branch 5 times, most recently from ef57f1b to 1151625 Compare April 23, 2024 23:51
@JeremyMcCormick JeremyMcCormick changed the title DM-43998: Handle default values properly when creating SQL metadata DM-43998: Properly validate default values and fix their handling in SQL metadata Apr 23, 2024
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-43998 branch 3 times, most recently from 5dc6957 to 5005bef Compare April 25, 2024 17:45
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

I am concerned with using JSON formatting for default string value. Do we have a test for MetadataBuilder which uses non-empty default for a string column (and generates actual schema)?

python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/metadata.py Outdated Show resolved Hide resolved
python/felis/datamodel.py Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-43998 branch 2 times, most recently from b8c5e8b to 5b6c6b9 Compare April 25, 2024 20:38
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks good, one minor comment.

python/felis/datamodel.py Outdated Show resolved Hide resolved
python/felis/metadata.py Outdated Show resolved Hide resolved
This adds validation of the column `value` field, which provides a
default that is passed to `server_default` when creating a SQLAlchemy
column.

The `MetaDataBuilder` was updated to properly handle these values
depending on their type and content. Strings are quoted, except when
they represent SQL functions such as `CURRENT_TIMESTAMP`.

Tests were added to check that the validation functions properly for
all of the Felis datatypes.
@JeremyMcCormick JeremyMcCormick merged commit 484f9b6 into main Apr 26, 2024
10 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-43998 branch April 26, 2024 00:56
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.

None yet

3 participants