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

Document default values for model columns #395

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Document default values for model columns #395

merged 2 commits into from
Aug 11, 2020

Conversation

stephendolan
Copy link
Member

This PR aims to serve as a starting point to close #189.

I'm a little bit fuzzy on how the new functionality in luckyframework/avram#424 actually works in conjunction with the default option you can add to a database migration, but I'm hoping this is a good enough starting point to give me a bit of guidance and take some work off of the plate of the folks actually building all of this nifty stuff.

This PR aims to serve as a starting point to close #189.

I'm a little bit fuzzy on how the new functionality in luckyframework/avram#424 actually works in conjunction with the `default` option you can add to a database migration, but I'm hoping this is a good enough starting point to give me a bit of guidance and take some work off of the plate of the folks actually building all of this nifty stuff.
column email
column encrypted_password : String

column greeting : String = "Hello there!"
Copy link
Member

Choose a reason for hiding this comment

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

General Kenobi

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

I think this pretty much nails it.

I'd say in general though the default in the model should also be set in the database for consistency. All the default does in Avram is make sure it is set in operations so it doesn't require you to set it manually.

Does that make sense?

@stephendolan
Copy link
Member Author

@paulcsmith / @jwoertink I guess I may still be a bit fuzzy on the "value prop" of this feature... Just want to be sure I get it right, then we can get this thing merged!

If I have a default value at the DB level, what do I have to do differently when using model default values than when not using them? Is the main purpose for a not null column with a default, where Avram would complain if I try to not provide a value previously?

@paulcsmith
Copy link
Member

@stephendolan No prob. Happy to try again because I'm not explaining it well.

Before we allowed defaults in the model the process went like this:

  • users table with an admin column that is NOT NULL and has a default of false
  • Try to save SaveUser.create!(name: "John")
  • Will fail to pass validations because admin is nil and Avram will say "hey, that needs to be filled in because it is not allowed to be nil

So you'd have to manually set it SaveUser.create!(name: "John", admin: false) or set a default in the operation in a before_save. This was unexpected and kind of a PITA.

So now you can set the default in the column definition and Avram will set the default to whatever you put. This makes it so validations pass and you have the default value set in your operations.

So the default is really just to tell Avram what the database already knows. The idea is that the default should be set in both the database and the model and should match 99% of the time.

We plan to add a check to SchemaEnforcer eventually so that it will notify you if you have a missing default or an incorrect default (where the db default and the one in Avram are not the same). But right now it is up to the user to do keep them in sync.

You could of course add a default to the model and not in the db, and that'd be fine, but I'm not 100% sure why you'd want it in the model and not the db..maybe if it allows nil and you want to more easily change what the default value is? I've never done that but it is possible to do. I mention this in case it is helpful but if I'm just confusing things more, just ignore it :P

@stephendolan
Copy link
Member Author

That's crystal™️ clear, thanks @paulcsmith!

I might reframe this a bit, and will have this ready to go sometime this week.

@stephendolan
Copy link
Member Author

In reviewing once more, I actually think this is good to merge @paulcsmith! It'll be worth adding some callouts to this section when we enforce defaults between the model and database, but until then this will be helpful enough!

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Sounds good! 👍

@paulcsmith paulcsmith merged commit 53bafc8 into luckyframework:master Aug 11, 2020
@jwoertink
Copy link
Member

I just realized this issue was tagged as next release. We can't deploy these docs 😭

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.

Document how to use default columns
3 participants