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

Add database_required to field metadata #23213

Merged
merged 16 commits into from Jun 14, 2022

Conversation

snoe
Copy link
Contributor

@snoe snoe commented Jun 7, 2022

Resolves #23125

The purpose of this new column is meant to track if the database believes
that the column is required during writeback. It may be that the user
knows better but that is out of scope and would need
another column so that sync never overwrites what a user would set.

Since writeback isn't planned to be added to non transactional-dbs yet
we do not need to calculate requiredness in other drivers.

This could become more sophisticated in the future (look at triggers
that modify the column) simply looking for not-null and non-default
columns gets us a big win in UI.

The purpose of this new column is meant to track if the database believes
that the column is required during writeback. It may be that the user
knows better but that is out of scope and would need
another column so that sync never overwrites what a user would set.

Since writeback isn't planned to be added to non transactional-dbs yet
we do not need to calculate requiredness in other drivers.

This could become more sophisticated in the future (look at triggers
that modify the column) simply looking for not-null and non-default
columns gets us a big win in UI.
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #23213 (6eb1776) into pod-writeback (28c5a14) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           pod-writeback   #23213   +/-   ##
==============================================
  Coverage          63.88%   63.89%           
==============================================
  Files               2645     2645           
  Lines              83306    83322   +16     
  Branches           10308    10308           
==============================================
+ Hits               53222    53238   +16     
  Misses             25878    25878           
  Partials            4206     4206           
Flag Coverage Δ
front-end 44.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/metabase/driver/sql_jdbc/sync/describe_table.clj 62.66% <100.00%> (+1.50%) ⬆️
src/metabase/sync/interface.clj 98.87% <100.00%> (+0.01%) ⬆️
...abase/sync/sync_metadata/fields/fetch_metadata.clj 83.33% <100.00%> (+0.35%) ⬆️
...abase/sync/sync_metadata/fields/sync_instances.clj 88.69% <100.00%> (+0.09%) ⬆️
...tabase/sync/sync_metadata/fields/sync_metadata.clj 94.00% <100.00%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28c5a14...6eb1776. Read the comment docs.

@camsaul
Copy link
Member

camsaul commented Jun 10, 2022

"database required" in this case means that it's not nullable and it doesn't have a default value, right? I think we could do some interesting stuff in other places outside of the actions stuff with nullable if we recorded that. Maybe displaying a default_value could be neat too in the insert form. Do you think it makes sense to record nullable and default_value separately instead of having a single database_required column?

@snoe
Copy link
Contributor Author

snoe commented Jun 10, 2022

@camsaul

"database required" in this case means that it's not nullable and it doesn't have a default value, right? I think we could do some interesting stuff in other places outside of the actions stuff with nullable if we recorded that. Maybe displaying a default_value could be neat too in the insert form. Do you think it makes sense to record nullable and default_value separately instead of having a single database_required column?

The reason I went with database_required is because of the consideration raised that triggers, check constraints, and other things do affect the requiredness of the field and with the idea that we can improve the accuracy of this value in the future by taking those other things into consideration. I could be wrong about this though, what do you think?

So I think I would keep this column, and if we want explicit knowledge about nullable or default value we can add them too as part of this pr, or as part of a followup pr.

Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Ok, I think the reasoning makes sense. I'm cool with this for now. We can always change it later if we decide that nullable or whatever makes more sense.

I don't know how we're going to be able to sync whether a column is required automatically based on what triggers are doing tho. I guess maybe we'd just have to let people set that manually? Open question I guess... if we let them set it manually then we need to make sure the next sync doesn't stomp on those changes. We can worry about that later tho... this is ok for now I think.

One other question for you. Does COLUMN_DEF return calculated defaults like current_timestamp or now() for things like a created_at column?. Is it database dependent? This becomes a lot less useful if things like created_at with calculated defaults don't get marked as database_required = false

snoe added 4 commits June 13, 2022 14:58
https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html#getColumns(java.lang.String,%20java.lang.String,%20java.lang.String,%20java.lang.String)
Based on these descriptions of `getColumns` and testing across dbs made
the following changes:

`COLUMN_DEF` may or may not show autoincrement defaults
`COLUMN_DEF` may be `nil` or `"NULL"` and the docs say it may be `"null"`
`NULLABLE` is an int value, `0` indicating not null

NULLABLE and AUTOINCREMENT may be unknown (`3`, `""`) in those cases the
field will not be marked as required.
@snoe snoe merged commit d240a53 into pod-writeback Jun 14, 2022
@snoe snoe deleted the writeback-database-required-meta branch June 14, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants