Empty ids #118

Closed
ajselvig opened this Issue Jan 13, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@ajselvig
Contributor

ajselvig commented Jan 13, 2015

This has caught me twice and I know it's due to bugs in my code, but it would be nice to have an earlier warning. NoBrainer will happily save a document with id='' (empty string). This happens only once, of course, since the next time around it will fail the uniqueness requirement for ids.

However, I cannot possibly think of an actual use for this behavior, and it would be helpful if an exception was raised (or error added for ?-suffix methods) when the document is saved. This will make it easier to detect the error in user code and fix it before there's a dangling document in the database.

Thoughts?

Obligatory example code:

require 'nobrainer'

NoBrainer.configure do |config|
  config.rethinkdb_url          = 'rethinkdb://localhost/nobrainer_playground'
  config.logger                 = Logger.new $stdout
end

class Foo
  include NoBrainer::Document
end

Foo.delete_all

f = Foo.new id: ''
f.save
puts f.id # empty!
@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

According to your comment on #109

To me, a not present value is either nil or "never been set according to whatever internal attribute system is being used". [...] [About Strings:] Isn't that what length validations are for?

It's unclear whether we should allow the empty string as an id based on that.

Here's what I suggest:

  1. refine the meaning of required => true so that for String,Text, and Symbol it means to validate with the presence validator. For all the other types (Array, Boolean, etc.), validate with the non_null validator.
  2. add required => true to the default primary key definition.

@ajselvig What do you think?

@mbrock do you have any opinion of what required => true should do? (c.f. #109 for previous discussion)

Owner

nviennot commented Jan 14, 2015

According to your comment on #109

To me, a not present value is either nil or "never been set according to whatever internal attribute system is being used". [...] [About Strings:] Isn't that what length validations are for?

It's unclear whether we should allow the empty string as an id based on that.

Here's what I suggest:

  1. refine the meaning of required => true so that for String,Text, and Symbol it means to validate with the presence validator. For all the other types (Array, Boolean, etc.), validate with the non_null validator.
  2. add required => true to the default primary key definition.

@ajselvig What do you think?

@mbrock do you have any opinion of what required => true should do? (c.f. #109 for previous discussion)

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

I actually think required: true should just mean "not Object#blank?" as in ActiveModel. It might be slightly weird that this means boolean fields cannot be false, but it seems slightly weird to have required: true on a boolean field to begin with.

My mental model is that setting required: true on a field means that the model's CRUD form would have an asterisk next to the field to mark it as required. Both on text fields and array select boxes, this would mean that the user must specify some non-empty value.

On a boolean field, it would be weird—the only thing I can think of is the classic "I agree to the terms and conditions" box, which of course most applications probably don't save as part of the User model. 😄

But it seems nice to have a single straightforward definition of required that also matches ActiveModel/ActiveRecord.

Contributor

mbrock commented Jan 14, 2015

I actually think required: true should just mean "not Object#blank?" as in ActiveModel. It might be slightly weird that this means boolean fields cannot be false, but it seems slightly weird to have required: true on a boolean field to begin with.

My mental model is that setting required: true on a field means that the model's CRUD form would have an asterisk next to the field to mark it as required. Both on text fields and array select boxes, this would mean that the user must specify some non-empty value.

On a boolean field, it would be weird—the only thing I can think of is the classic "I agree to the terms and conditions" box, which of course most applications probably don't save as part of the User model. 😄

But it seems nice to have a single straightforward definition of required that also matches ActiveModel/ActiveRecord.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Jan 14, 2015

Contributor

@nviennot I think that's probably a good solution. I guess I was thinking of id as a special field, different from the regular validations, but this works as well.

@mbrock Can you provide a situation where the ActiveModel required behavior for booleans is actually useful? It seems like you're specifying that a field can have only one possible value, so it might as well be a constant. Plus, this is a common source of confusion to those not familiar with the implementation.

Booleans in Ruby are ternary: true, false, and nil. By implementing the required validation the way we have, it becomes binary (and still useful).

Matching ActiveModel for the sake of matching it seems misguided. I like that NoBrainer chooses features that are useful and offer the least surprise. Besides, the ActiveModel reasoning for having this behavior seems to be "it lets us implement the validation with one function". Seems weak.

Contributor

ajselvig commented Jan 14, 2015

@nviennot I think that's probably a good solution. I guess I was thinking of id as a special field, different from the regular validations, but this works as well.

@mbrock Can you provide a situation where the ActiveModel required behavior for booleans is actually useful? It seems like you're specifying that a field can have only one possible value, so it might as well be a constant. Plus, this is a common source of confusion to those not familiar with the implementation.

Booleans in Ruby are ternary: true, false, and nil. By implementing the required validation the way we have, it becomes binary (and still useful).

Matching ActiveModel for the sake of matching it seems misguided. I like that NoBrainer chooses features that are useful and offer the least surprise. Besides, the ActiveModel reasoning for having this behavior seems to be "it lets us implement the validation with one function". Seems weak.

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

I'm currently working on a NoBrainer adapter for rails_admin, which seems to make me biased in favor of having similar behaviors across different ORMs. But of course, we are aren't working with a common standard, and we probably don't want to.

Since the ActiveModel behavior for required booleans is so weird and seemingly useless, and since they have decided that they don't want to change it, I wouldn't mind deviating from it.

The NoBrainer.io documentation currently states that:

Validations works the same way as in ActiveRecord because NoBrainer reuses the ActiveModel validation logic.

But I guess all that would have to change is the explanation of the required shorthand?

Contributor

mbrock commented Jan 14, 2015

I'm currently working on a NoBrainer adapter for rails_admin, which seems to make me biased in favor of having similar behaviors across different ORMs. But of course, we are aren't working with a common standard, and we probably don't want to.

Since the ActiveModel behavior for required booleans is so weird and seemingly useless, and since they have decided that they don't want to change it, I wouldn't mind deviating from it.

The NoBrainer.io documentation currently states that:

Validations works the same way as in ActiveRecord because NoBrainer reuses the ActiveModel validation logic.

But I guess all that would have to change is the explanation of the required shorthand?

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

So the non nil vs presence validator discussion makes sense only for String, Text, Binary, Array, Hash, Object, Boolean. The other types have the same behavior.

So we all agree that for Boolean, we want non nil, otherwise, it's not useful.
For String, I definitely think that "" as a required id or username is a little far off in terms of expected behavior. Text and Binary are strings so it makes sense that they follow the same rule as String.

For Object, we'll do whatever is convenient to make the definition of required => true in the documentation to be as succinct as possible.

Which leaves us with Array, Hash, which should behave the same. I don't think there is a clear cut to what it possibly could mean.

My mental model is that setting required: true on a field means that the model's CRUD form would have an asterisk next to the field to mark it as required. Both on text fields and array select boxes, this would mean that the user must specify some non-empty value.

I like this, it's good way to think about it.

The easiest definition that kind of fits everything well and will be easy to remember is: required => true means to use the presence validation except for Boolean types which use a non nil validation.

Owner

nviennot commented Jan 14, 2015

So the non nil vs presence validator discussion makes sense only for String, Text, Binary, Array, Hash, Object, Boolean. The other types have the same behavior.

So we all agree that for Boolean, we want non nil, otherwise, it's not useful.
For String, I definitely think that "" as a required id or username is a little far off in terms of expected behavior. Text and Binary are strings so it makes sense that they follow the same rule as String.

For Object, we'll do whatever is convenient to make the definition of required => true in the documentation to be as succinct as possible.

Which leaves us with Array, Hash, which should behave the same. I don't think there is a clear cut to what it possibly could mean.

My mental model is that setting required: true on a field means that the model's CRUD form would have an asterisk next to the field to mark it as required. Both on text fields and array select boxes, this would mean that the user must specify some non-empty value.

I like this, it's good way to think about it.

The easiest definition that kind of fits everything well and will be easy to remember is: required => true means to use the presence validation except for Boolean types which use a non nil validation.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

For the required stuff: 8e7f108

However, I couldn't just add a required => true on the primary key definition because I cannot easily remove a validator. Which needs to be done when defining another primary key than id. If the id validator stays, it becomes a little sad. So I'll see what I can do later. If it involves heavy monkey patching on the active model validators, I'm not going to happen.

Owner

nviennot commented Jan 14, 2015

For the required stuff: 8e7f108

However, I couldn't just add a required => true on the primary key definition because I cannot easily remove a validator. Which needs to be done when defining another primary key than id. If the id validator stays, it becomes a little sad. So I'll see what I can do later. If it involves heavy monkey patching on the active model validators, I'm not going to happen.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

Okay so I took a closer look, and validators add validation callbacks. So I'd have to remove callbacks when removing the default definition of the primary key. Here's the code that clear all the callbacks: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/callbacks.rb#L715 -- doesn't look so fun.

So I'm not going to add the validation on the primary key. However, if you'd like to add such validation on all models in your own project, you can stick that in an initializer in your app:

module ValidatePrimaryKey
  extend ActiveSupport::Concern

  included do
    field :id, :required => true
  end

  NoBrainer::Document.__send__(:include, self)
end
Owner

nviennot commented Jan 14, 2015

Okay so I took a closer look, and validators add validation callbacks. So I'd have to remove callbacks when removing the default definition of the primary key. Here's the code that clear all the callbacks: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/callbacks.rb#L715 -- doesn't look so fun.

So I'm not going to add the validation on the primary key. However, if you'd like to add such validation on all models in your own project, you can stick that in an initializer in your app:

module ValidatePrimaryKey
  extend ActiveSupport::Concern

  included do
    field :id, :required => true
  end

  NoBrainer::Document.__send__(:include, self)
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment