Handle false and nil values correctly.

[bigjason]

The use of the #one? method was causing false and nil values to be
skipped. This is unexpected and problematic in most situations (see the unit test).

[lagartoflojo]

I'm using @bigjason's fork in my app and it's working as intended.

[justincampbell]

👍 Although ideally this would be configurable

[ismasan]

+1 for this pull request please. False values are still values (same thing happens with nil values, which should translate into JSON nulls). Using the fork for now.

[maletor]

👍

[maletor]

Using fork for now.

[caius]

👍 just ran across this and it confused the hell out of me.

[kayakyakr]

👍

[muxcmux]

👍

[derek-watson]

Switching to the patched fork

[tal]

Agreed, this should be merged in. Should have found this discussion before i made: https://github.com/Talby/jbuilder/compare/nil_values

[rbudiharso]

👍

[gicappa]

any plan about including this pull request?

[bigjason]

I think we just have to wait for @dhh to review it and (hopefully) merge it.

[justincampbell]

I don't think this change will be merged. It changes the behavior of the builder, and could cause unexpected results for existing implementations. A better solution might be a configuration variable available globally and/or scoped to blocks.

# Enables jbuilder to render nil and false values
Jbuilder.render_all = true

or something like

Jbuilder.encode(all: true) do |json|
  json.nil nil # yup
  json.false false # yup

  json.stuff(all: false) do |json|
    json.nil nil # nope
  end
end

The block options could be nil, false, and all(both), or maybe prefixed with render_ (render_nil).

Regardless, I think it makes sense to change the default in a future major version number.

[bigjason]

I am curious about where having the builder output all values passed in would produce unexpected results. Do you have an example?

[gicappa]

I think what Justin is saying is that the builder after the patch would display {key: false} when currently it would have skipped the entry. This change can eventually break the existing code relying on the current (while wrong) behavior.
So it should be preferred to switch to the correct behavior in a 'softer' way.

[justincampbell]

@bigjason If someone already has a client that is checking for the absence of a key to determine null or false. That's of course not good practice, but the bottom line is that the output would be different, and things that "work" now might not work anymore.

However, the current version is 0.3, so if we follow semantic versioning, you could argue that it's ok to break things pre-1.0.

[bigjason]

Ok I see what you mean. Yeah being pre-1.0 I think a note in the release notes for fixing this in release 0.4 would be sufficient. Either way its not up to me so I guess we will have to wait and see.

[kayakyakr]

agree with @bigjason

this should be the default behavior ASAP (ie before it becomes the rails standard). A note and the ability to turn it off if it breaks things should be all that is needed.

[maletor]

On second thought maybe one? isn't such a bad idea. Objective-C doesn't even know how to make a hash (dictionary) without a value.

Additionally, we can save a few bytes that the client has to download.

Depends on how you want your stuff documented: In the response or in the documentation. Given that a lot of the time you will have array: [] anyways it doesn't seem to helpful.

I reneg on my initial thoughts with this feature, but think it may be worthwhile as an option. Perhaps in config/application.rb?

[bigjason]

Heaven help us if we start making our design decisions based on what Objective-C does ;-)

Seriously though the issue isn't about saving bytes. The issue is that when you set a value to false or nil, you are simply ignored. Personally I favor the principle of least astonishment when it comes to API design and I believe most people are astonished when they set a value and it is ignored. This bug also limits how the builder can be used. For instance:

Jbuilder.encode do |json|
  json.name "Bob"
  json.name nil
end

# Output is {name: "Bob"}

Jbuilder.encode do |json|
  json.name "Bob"
  json.name "Phil"
end

# Output is {name: "Phil"}

While this is not how I use Jbuilder, it is a pretty serious flaw for the builder to silently ignore the new value simply because it is false or nil. It is also inconsistant with how the builder works in other places. For instance arrays with nulls, empty arrays and empty objects are all included:

Jbuilder.encode do |json|
  json.foo [nil]
  json.bar []
  json.lou {}
end

# Output is {"foo":[null],"bar":[],"lou":{}}

So I see no reason to treat other value types differently.

[maletor]

So you are against offering as an option?

[maletor]

At the very least there should be consistency.

[bigjason]

No I think an optional would be ok, I just am unsure where you would draw the line. Which of the types get ignored when? Is it just nil and false? What about 0, {}, [] etc. Some languages call those false values too.

Even with it as an option I think my examples above show that it is currently broken, and comments from other people seem to show that it is a general issue. Personally I think that "feature" would fall outside of this bug fix anyways so perhaps its moot in this pull request and needs its own?

[raycohen]

+1 @bigjason

[gicappa]

+1 @bigjason for me as well.

I think that the usage of #one? in that context is wrong and can't be considered a feature. JSON is supposed to allow the usage of native true/false/null values and with this bug it would simply be impossible. So IMO that is a bug.

Said that I think it's correct to get to the right behavior respecting the users and with a smooth transition based on options and defaults.

[NilsHaldenwang]

+1 @bigjason

Just stepped over this because I need to serialize false values to JSON in a current project. In my opinion this behavior should be considered a bug. I will using the fork now,...

Making it configureable would be the best solution, but according to the principle of least astonishment the current behavior should not be the default.

[maletor]

I have been using this for a couple weeks now with great success. Principle of least astonishment is the way to go, not to mention some consistency about how to handle false and nil.

[tagrudev]

so is this going to be merged or no?

[maletor]

/cc @dhh @rails

[aganov]

+1

[xxx]

+1. on two separate occasions we have wasted time because of this.

[robwilliams]

+1

[pigoz]

+1

[Sephi-Chan]

+1

[muxcmux]

Finally, I can switch back to simply gem "jbuilder" in my Gemfile. Thanks @dhh

[justincampbell]

👏