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

Better handling of default values #170

Open
mhenrixon opened this issue Jan 27, 2024 · 7 comments · May be fixed by #171
Open

Better handling of default values #170

mhenrixon opened this issue Jan 27, 2024 · 7 comments · May be fixed by #171

Comments

@mhenrixon
Copy link

mhenrixon commented Jan 27, 2024

Thank you for a lovely gem!

I have one issue so far: how the gem suggests I add presence validation to columns with default values. I use default values because I don't care about having a presence validation, which is how I thought everyone did it.

I still want to use that rake task but managing the ignores is cumbersome. Am I doing something wrong?

https://x.com/mhenrixon/status/1751157307566637102?s=20

@mhenrixon
Copy link
Author

I double-checked, and with default column values, rails do the expected thing. If I explicitly try to set the value to null, it blows up in the database.

This is perfectly fine for me, as I am trying to avoid defensive programming.

What do I mean by that?

Take the following

create_table "users", force: :cascade do |t|
  t.integer "articles_count", default: 0, null: false
end

Why should I validate its presence? This is an internal Rails thing, and I want to go ahead and trust that the core Ruby on Rails programmers know how to ensure this is working correctly.

@gregnavis
Copy link
Owner

@mhenrixon, thank you for opening the issue.

As you said, we shouldn't be testing Rails implementation details, and that's not the goal of this validator.

In your example, it's still possible that the column would end up being set to nil, for example via:

User.create!(articles_count: nil)

That nil can come from return value, for instance. Additionally, for column types like string, the notion of being NOT NULL and present diverge considerably. A username of "" is NOT NULL but it's not present. In those cases a presence validator is even more necessary as that NOT NULL constraint captures only a part of the real requirement.

I personally provide user-facing default values in controllers, but I can see your approach working too.

For the reason explained above, I think, skipping columns with default values would be a mistake. However, I'm open to adding a setting to the validator like ignore_columns_with_defaults that would allow you to silence those warnings in one whole swoop.

How does that sound?

@mhenrixon
Copy link
Author

In your example, it's still possible that the column would end up being set to nil, for example via:

It isn't possible because the column is not null in the database.

I'm happy with a setting to control this. Personally, I want to write as little code as possible.

Love the gem btw, phenomenal tool!

@gregnavis
Copy link
Owner

In your example, it's still possible that the column would end up being set to nil, for example via:

It isn't possible because the column is not null in the database.

What I meant was "a nil value may still be passed for insertion, and the default would not be applied (resulting in an exception)". Sorry for sloppy wording!

I'm happy with a setting to control this. Personally, I want to write as little code as possible.

Love the gem btw, phenomenal tool!

Thank you for your kind words! I'm happy you find it useful in your work!

@mhenrixon
Copy link
Author

What I meant was "a nil value may still be passed for insertion, and the default would not be applied (resulting in an exception)".

Yes, and while it isn't ideal, it is exceptional enough that I see it as wasteful to add a rails validation for that specific case.

Now I have to duplicate setting the default value in ruby, which is unnecessary.

Remember that not having the default in the database adds complexity to database operations on top of the additional default values and validations in Ruby on Rails.

In my opinion, this specific case (integer column with default value) is a false positive, and pursuing it adds unnecessary complexity.

I used to be super defensive in my programming, but lately, I see things differently.

You can blame it on studying the campfire code if you want.

It was liberating to see that much of the code I write wouldn't be written by the creators of the framework we all love to use.

It made me look at this differently.

After adding a presence validation, I could no longer rely on the default value and had to add a method to ensure that rails set the counter to zero on creation.

I thought about it and realized I'm not too fond of this suggestion. Hence, this issue.

@mhenrixon
Copy link
Author

I'm not trying to convince you of anything, @gregnavis, but I figured I should at least share how I ended up creating this issue.

I didn't address your comment about the string column. I agree that preventing an empty string is a good practice.

That said, a length validator might be more suitable here anyway, and validators often must be bypassed (update columns).

If we want to avoid an empty string, we should constrain that in the database regardless.

I appreciate the feedback and the discussion.

@fatkodima
Copy link
Contributor

Another case just discovered - not null boolean columns with a default. Makes little sense to me having presence validations for these too.

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 a pull request may close this issue.

3 participants