Setting belongs_to ids to nil when an empty string is set #133

Closed
ajselvig opened this Issue Apr 12, 2015 · 15 comments

Comments

Projects
None yet
2 participants
@ajselvig
Contributor

ajselvig commented Apr 12, 2015

When using url encoded for submissions, there's no real way to represent a "null" value. A common pattern is to have a dropdown for the possible options of a belongs_to relationship, with an empty option for "none", which gets transmitted as an empty string.

The problem is that NoBrainer faithfully stores the foreign key as an empty string. I believe that it would be better to store that field as null. Storing an empty string is technically incorrect, since there could actually be a record with an empty string as an id.

Is this possible?

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

I don't have this problem. Can you write a little test case?

Owner

nviennot commented Apr 12, 2015

I don't have this problem. Can you write a little test case?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 12, 2015

Contributor
require 'nobrainer'

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

class Foo
  include NoBrainer::Document

  has_many :bars
end

class Bar
  include NoBrainer::Document

  belongs_to :foo
end

Foo.delete_all
Bar.delete_all

foo = Foo.create
bar = Bar.create foo_id: foo.id

bar.update_attributes foo_id: ''
bar.update_attributes foo_id: nil

The first update_attributes executes:

r.table("bars").get("1oVxWr6AdJuuGl").update {|var_1| {"foo_id" => ""}}

and the second executes:

r.table("bars").get("1oVxWr6AdJuuGl").update {|var_1| {"foo_id" => nil}}

I am proposing that they both execute the latter query. This is because it's a common pattern to be assigning the foo_id from a form like:

<select name="foo_id">
    <option value="">None Selected</option>
    <option value="1oVzCCd2GE5pEP">Foo 1</option>
    ... other options
</select>
Contributor

ajselvig commented Apr 12, 2015

require 'nobrainer'

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

class Foo
  include NoBrainer::Document

  has_many :bars
end

class Bar
  include NoBrainer::Document

  belongs_to :foo
end

Foo.delete_all
Bar.delete_all

foo = Foo.create
bar = Bar.create foo_id: foo.id

bar.update_attributes foo_id: ''
bar.update_attributes foo_id: nil

The first update_attributes executes:

r.table("bars").get("1oVxWr6AdJuuGl").update {|var_1| {"foo_id" => ""}}

and the second executes:

r.table("bars").get("1oVxWr6AdJuuGl").update {|var_1| {"foo_id" => nil}}

I am proposing that they both execute the latter query. This is because it's a common pattern to be assigning the foo_id from a form like:

<select name="foo_id">
    <option value="">None Selected</option>
    <option value="1oVzCCd2GE5pEP">Foo 1</option>
    ... other options
</select>
@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

add :required => true on the belongs to association

Owner

nviennot commented Apr 12, 2015

add :required => true on the belongs to association

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

oops no, nevermind. let me think about this

Owner

nviennot commented Apr 12, 2015

oops no, nevermind. let me think about this

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 12, 2015

Contributor

But it's not a required association, that's the whole point of having the empty option.

Contributor

ajselvig commented Apr 12, 2015

But it's not a required association, that's the whole point of having the empty option.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

What does your controller code look like?

Owner

nviennot commented Apr 12, 2015

What does your controller code look like?

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

You can add this in your model to have the behavior that you want:

def foo_id=(value)
  super(value.presence)
end

I'm a little uncomfortable to do it by the ORM, because then you'd want this feature for any attributes. Why restrict this feature with foreign keys?

Owner

nviennot commented Apr 12, 2015

You can add this in your model to have the behavior that you want:

def foo_id=(value)
  super(value.presence)
end

I'm a little uncomfortable to do it by the ORM, because then you'd want this feature for any attributes. Why restrict this feature with foreign keys?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 12, 2015

Contributor

The controller is totally normal, just assigning the form parameters to the model object:

def update
  bar = Bar.find params[:id]
  bar.update_attributes params[:bar]
end

because then you'd want this feature for any attributes?

Certainly not. Storing empty strings is reasonable behavior for regular string fields.

Why restrict this feature with foreign keys?

Foreign keys are fundamentally different than string fields. They are supposed to always contain the id of an existing record or "nothing" (nil).

In the same vein, this also works fine:

bar.update_attributes foo_id: 'something'

but is also equally non-sensical.

Contributor

ajselvig commented Apr 12, 2015

The controller is totally normal, just assigning the form parameters to the model object:

def update
  bar = Bar.find params[:id]
  bar.update_attributes params[:bar]
end

because then you'd want this feature for any attributes?

Certainly not. Storing empty strings is reasonable behavior for regular string fields.

Why restrict this feature with foreign keys?

Foreign keys are fundamentally different than string fields. They are supposed to always contain the id of an existing record or "nothing" (nil).

In the same vein, this also works fine:

bar.update_attributes foo_id: 'something'

but is also equally non-sensical.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

I'd solve this in two ways:

  1. Either add this in your model:
def foo_id=(value)
  super(value.presence)
end

You enforce the translation at the model level.

  1. Or in your controller:
  bar.update_attributes(:foo_id => params[:foo_id].presence)

To avoid issues with bar.update_attributes foo_id: 'invalid', You can do something like this:

belongs_to :foo, :validates => {:presence => true, :if => ->{ foo_id.present? }}

It's pretty ugly, but I don't want to do this behavior by default. What would be a good shorthand to specify this?

Owner

nviennot commented Apr 12, 2015

I'd solve this in two ways:

  1. Either add this in your model:
def foo_id=(value)
  super(value.presence)
end

You enforce the translation at the model level.

  1. Or in your controller:
  bar.update_attributes(:foo_id => params[:foo_id].presence)

To avoid issues with bar.update_attributes foo_id: 'invalid', You can do something like this:

belongs_to :foo, :validates => {:presence => true, :if => ->{ foo_id.present? }}

It's pretty ugly, but I don't want to do this behavior by default. What would be a good shorthand to specify this?

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

Actually, maybe it's a good default. If you don't want it, it's probably because you are importing data in some ways. You might be better of with doing save(validate: false).
Let me implement that. It should be quick.

Owner

nviennot commented Apr 12, 2015

Actually, maybe it's a good default. If you don't want it, it's probably because you are importing data in some ways. You might be better of with doing save(validate: false).
Let me implement that. It should be quick.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 12, 2015

Contributor

All I had to do was not respond for three minutes!

Contributor

ajselvig commented Apr 12, 2015

All I had to do was not respond for three minutes!

@nviennot nviennot closed this in 5a25892 Apr 12, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 12, 2015

Owner

Foreign keys are always validated now. If you don't want this behavior, you can pass validates: false on the belongs_to declaration.

Owner

nviennot commented Apr 12, 2015

Foreign keys are always validated now. If you don't want this behavior, you can pass validates: false on the belongs_to declaration.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 27, 2015

Contributor

So, this is definitely better but it still doesn't solve the original
problem: ids that get sent to null on the web side get transmitted as an
empty string and assigned on the server. This, of course, causes the
validation to fail.

If you don't want to bake a concept of 'empty string == nil id' into
NoBrainer, can you suggest the simplest way to monkey patch NoBrainer to
add this behavior?

On Sun, Apr 12, 2015 at 1:40 PM, Nicolas Viennot notifications@github.com
wrote:

Foreign keys are always validated now. If you don't want this behavior,
you can pass validates: false on the belongs_to declaration.


Reply to this email directly or view it on GitHub
#133 (comment).

Contributor

ajselvig commented Apr 27, 2015

So, this is definitely better but it still doesn't solve the original
problem: ids that get sent to null on the web side get transmitted as an
empty string and assigned on the server. This, of course, causes the
validation to fail.

If you don't want to bake a concept of 'empty string == nil id' into
NoBrainer, can you suggest the simplest way to monkey patch NoBrainer to
add this behavior?

On Sun, Apr 12, 2015 at 1:40 PM, Nicolas Viennot notifications@github.com
wrote:

Foreign keys are always validated now. If you don't want this behavior,
you can pass validates: false on the belongs_to declaration.


Reply to this email directly or view it on GitHub
#133 (comment).

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Apr 28, 2015

Owner

You can add this in an initializer:

module ForeignKeyPresence
  def hook
    super
    fk_setter = "#{self.foreign_key}="
    owner_model.inject_in_layer :transformations do
      define_method(fk_setter) { |value| super(value.presence) }
    end
  end

  NoBrainer::Document::Association::BelongsTo::Metadata.send(:include, self)
end
Owner

nviennot commented Apr 28, 2015

You can add this in an initializer:

module ForeignKeyPresence
  def hook
    super
    fk_setter = "#{self.foreign_key}="
    owner_model.inject_in_layer :transformations do
      define_method(fk_setter) { |value| super(value.presence) }
    end
  end

  NoBrainer::Document::Association::BelongsTo::Metadata.send(:include, self)
end
@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Apr 28, 2015

Contributor

That worked. thanks!

On Tue, Apr 28, 2015 at 9:39 AM, Nicolas Viennot notifications@github.com
wrote:

You can add this in an initializer:

module ForeignKeyPresence
def hook
super
fk_setter = "#{self.foreign_key}="
owner_model.inject_in_layer :transformations do
define_method(fk_setter) { |value| super(value.presence) }
end
end

NoBrainer::Document::Association::BelongsTo::Metadata.send(:include, self)end


Reply to this email directly or view it on GitHub
#133 (comment).

Contributor

ajselvig commented Apr 28, 2015

That worked. thanks!

On Tue, Apr 28, 2015 at 9:39 AM, Nicolas Viennot notifications@github.com
wrote:

You can add this in an initializer:

module ForeignKeyPresence
def hook
super
fk_setter = "#{self.foreign_key}="
owner_model.inject_in_layer :transformations do
define_method(fk_setter) { |value| super(value.presence) }
end
end

NoBrainer::Document::Association::BelongsTo::Metadata.send(:include, self)end


Reply to this email directly or view it on GitHub
#133 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment