ActiveResource 3.2.1 : ActiveResource::Formats::JsonFormat.decode and ActiveResource::Base.include_root_in_json

[Mark Peterson]

So ActiveResource::Base.include_root_in_json is no longer supported, and then I encounter the following within Json Formats:

module ActiveResource
  module Formats
    module JsonFormat
      def decode(json)
        Formats.remove_root(ActiveSupport::JSON.decode(json))
      end
    end
  end
end

If ActiveResource::Base.include_root_in_json felt hacky enough to remove, didn't that change above feel even more hacky?

[Aaron Patterson]

Yes?

Sorry, I don't understand the question. Is there a bug?

--
Aaron Patterson
http://tenderlovemaking.com/

[Mark Peterson]

Yes, it violates the principal that the function should do only what it claims to do.

[Aaron Patterson]

On Fri, Mar 02, 2012 at 04:38:06PM -0800, Mark Peterson wrote:
> On Friday, March 2, 2012 7:25:34 PM UTC-5, Aaron Patterson wrote:
> >
> > On Fri, Mar 02, 2012 at 01:26:18PM -0800, Mark Peterson wrote:
> > > So ActiveResource::Base.include_root_in_json is no longer supported, and
> > > then I encounter the following within Json Formats:
> > >
> > > module ActiveResource
> > > module Formats
> > > module JsonFormat
> > > def decode(json)
> > > Formats.remove_root(ActiveSupport::JSON.decode(json))
> > > end
> > > end
> > > end
> > > end
> > >
> > > If ActiveResource::Base.include_root_in_json felt hacky enough to
> > remove,
> > > didn't that change above feel even more hacky?
> >
> > Yes?
> >
> > Sorry, I don't understand the question. Is there a bug?
>

> Yes, it violates the principal that the function should do only what it
> claims to do.

Can you show a code example?

[Mark Peterson]

On Friday, March 2, 2012 7:43:35 PM UTC-5, Aaron Patterson wrote:

Can you show a code example?

--
Aaron Patterson
http://tenderlovemaking.com/

Sure, I'll create a fresh project to juxtapose the problem this causes with GET and POST on the same model.

[Aaron Patterson]

On Fri, Mar 02, 2012 at 04:47:05PM -0800, Mark Peterson wrote:
> On Friday, March 2, 2012 7:43:35 PM UTC-5, Aaron Patterson wrote:
> >
> >
> > Can you show a code example?
>

> Sure, I'll create a fresh project to juxtapose the problem this causes with
> GET and POST on the same model.

Cool, thanks!

[Mark Peterson]

Took longer than I thought to get a basic use case that fails. Scenario 2 below is the failure:

My Code:

class User < ActiveResource::Base
  self.site = "http://localhost:9000"
end

class Image < ActiveResource::Base
end

class ImagePage < ActiveResource::Base
  self.element_name = "image_page"
  self.collection_name = "irrelevant_never_used_in_this_manner"
end

class ApplicationController < ActionController::Base
  def index
      Dir.glob("#{Rails.root}/app/models/*.rb").sort.each { |file| require_dependency file }
    user = User.find(123)
    p user
    render :text => "hello world"
  end
end

### Scenario 1 using remove_root in decode, user.json has "id" ###

def decode(json)
  Formats.remove_root(ActiveSupport::JSON.decode(json))
end

GET /users/123.json

"{\"id\":\"123\",\"image_page\":{\"images\":[{\"id\":123},{\"id\":456}],\"total\":2000,\"count\":2,\"start_index\":0}}"

p user = #<User:0x00000103ab6c98 @attributes={"id"=>"123", "image_page"=>#<ImagePage:0x00000103ab5cd0 @attributes={"images"=>[#<Image:0x00000103aac158 @attributes={"id"=>123}, @prefix_options={}, @persisted=false>, #<Image:0x00000103aabed8 @attributes={"id"=>456}, @prefix_options={}, @persisted=false>], "total"=>2000, "count"=>2, "start_index"=>0}, @prefix_options={}, @persisted=false>}, @prefix_options={}, @persisted=true>


### Scenario 2 using remove_root in decode, user.json does not have "id" ###

def decode(json)
  Formats.remove_root(ActiveSupport::JSON.decode(json))
end

GET /users/123.json

"{\"image_page\":{\"images\":[{\"id\":123},{\"id\":456}],\"total\":2000,\"count\":2,\"start_index\":0}}"

p user = #<User:0x00000101133cb8 @attributes={"images"=>[#<Image:0x000001011327a0 @attributes={"id"=>123}, @prefix_options={}, @persisted=false>, #<Image:0x00000103a49e40 @attributes={"id"=>456}, @prefix_options={}, @persisted=false>], "total"=>2000, "count"=>2, "start_index"=>0}, @prefix_options={}, @persisted=true>


### Scenario 3 not using remove_root in decode, user.json has "id" ###

def decode(json)
  ActiveSupport::JSON.decode(json)
end

GET /users/123.json

"{\"id\":\"123\",\"image_page\":{\"images\":[{\"id\":123},{\"id\":456}],\"total\":2000,\"count\":2,\"start_index\":0}}"

p user = #<User:0x0000010455a168 @attributes={"id"=>"123", "image_page"=>#<ImagePage:0x00000104559880 @attributes={"images"=>[#<Image:0x00000104557c88 @attributes={"id"=>123}, @prefix_options={}, @persisted=false>, #<Image:0x00000104557a08 @attributes={"id"=>456}, @prefix_options={}, @persisted=false>], "total"=>2000, "count"=>2, "start_index"=>0}, @prefix_options={}, @persisted=false>}, @prefix_options={}, @persisted=true>


### Scenario 4 not using remove_root in decode, user.json does not have "id" ###

def decode(json)
  ActiveSupport::JSON.decode(json)
end

GET /users/123.json

"{\"image_page\":{\"images\":[{\"id\":123},{\"id\":456}],\"total\":2000,\"count\":2,\"start_index\":0}}"

p user = #<User:0x000001044166d0 @attributes={"image_page"=>#<ImagePage:0x00000104415f28 @attributes={"images"=>[#<Image:0x00000104414920 @attributes={"id"=>123}, @prefix_options={}, @persisted=false>, #<Image:0x000001044146a0 @attributes={"id"=>456}, @prefix_options={}, @persisted=false>], "total"=>2000, "count"=>2, "start_index"=>0}, @prefix_options={}, @persisted=false>}, @prefix_options={}, @persisted=true>

[Mark Peterson]

And I should add, though it was difficult to get myself into this fail case, It was amazingly quick to set up 2 codebases and servers to perform this task.

Great work people! :)

[Mark Peterson]

Though it's less than likely a resource would return with no "id" (or only 1 attribute), I think that it needs to be considered. It's rare to find perfect CRUD.

Imagine that User.find(123) was actually CoolViews.find("aggregateOfAllHotTopics"). I guess that the service could supply an "id" of "aggregateOfAllHotTopics", but should that be mandatory?

What about searches? Will searches have an "id"? It's very likely that the structure of the search won't be perfect CRUD / REST.

And just to point out, lack of "id" isn't the problem, the problem is that there's only 1 attribute. Adding "id" fixes the problem, but also adding any attribute fixes the problem.

[Rodrigo Rosenfeld Rosas]

Given the code below, it seems you're just starting using Rails and you
still don't understand Rails basic concepts.

Shouldn't you consider posting in the user's mailing list first? It's
more likely that you'll get better advices there than here...

After you get used to Rails, if you still have questions about how Rails
should do things, then your questions will be better discussed here.

Cheers,
Rodrigo.

[Dheeraj Kumar]

Rodrigo has made a very important point about which list to use.

How do to X with rails -> rubyonrails-talk
How rails does X -> rails-core

> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To post to this group, send email to rubyonra...@googlegroups.com.
> To unsubscribe from this group, send email to
> rubyonrails-co...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/rubyonrails-core?hl=en.
>
>

[Mark Peterson]

On Saturday, March 3, 2012 9:23:16 AM UTC-5, Rodrigo Rosenfeld Rosas wrote:

Given the code below, it seems you're just starting using Rails and you
still don't understand Rails basic concepts.

Shouldn't you consider posting in the user's mailing list first? It's
more likely that you'll get better advices there than here...

After you get used to Rails, if you still have questions about how Rails
should do things, then your questions will be better discussed here.

Cheers,
Rodrigo.

Are you referring to my complete and utter lack of syntactic sugar?
It almost seems as though I deliberately avoid the stuff completely :)

This might be the wrong forum, but I'm sure that ActiveResource team are happy that I proofed the issue.

As an expert core member Rodrigo, do you know the alternative to this line?

[Matt Jones]

On Mar 2, 2012, at 10:17 PM, Mark Peterson wrote:
>
> ### Scenario 2 using remove_root in decode, user.json does not have "id" ###
>
> def decode(json)
> Formats.remove_root(ActiveSupport::JSON.decode(json))
> end
>
> GET /users/123.json
>
> "{\"image_page\":{\"images\":[{\"id\":123},{\"id\":456}],\"total\":2000,\"count\":2,\"start_index\":0}}"
>
> p user = #<User:0x00000101133cb8 @attributes={"images"=>[#<Image:0x000001011327a0 @attributes={"id"=>123}, @prefix_options={}, @persisted=false>, #<Image:0x00000103a49e40 @attributes={"id"=>456}, @prefix_options={}, @persisted=false>], "total"=>2000, "count"=>2, "start_index"=>0}, @prefix_options={}, @persisted=true>

This looks to be a variant of issue #2692:

https://github.com/rails/rails/pull/2692

although the patch in that instance wouldn't fix this bug, as the detection heuristic will guess that image_page is a root element to be removed.

It appears that the problem is that remove_root doesn't have any awareness of the context in which it's operating - really, we should only be removing the appropriately-named root element (if it exists).

--Matt Jones

[Mark Peterson]

On Saturday, March 3, 2012 9:27:58 AM UTC-5, Dheeraj Kumar wrote:

Rodrigo has made a very important point about which list to use.

How do to X with rails -> rubyonrails-talk
How rails does X -> rails-core

I'm pointing out something that changed, and would break my project if I upgrade, in ActiveResource which I believe stems from an ActiveResource fix to a problem caused by something that changed in ActiveModel regarding include_root_in_json.

This might be the wrong forum, but this is my fix:

module ActiveResource
  class Base
    self.include_root_in_json = false
  end 
  module Formats
    module JsonFormat
      def decode(json)
        ActiveSupport::JSON.decode(json)

      end
    end
  end
end

module ActiveModel
  module Serializers
    module JSON
      def as_json(options = nil)
        hash = serializable_hash(options)
        if include_root_in_json
          custom_root = options && options[:root]
          hash = { custom_root || self.class.model_name.element => hash }
        end
        hash
      end

      def from_json(json)
        hash = ActiveSupport::JSON.decode(json)
        hash = hash.values.first if include_root_in_json
        self.attributes = hash
        self
      end
    end
  end
end

 I don't use ActiveRecord, so I don't know the full ramifications of using that patch above. But it does make ActiveResource work again as I explore my upgrade to 3.2.* .

[Mark Peterson]

On Saturday, March 3, 2012 10:47:09 AM UTC-5, Matt jones wrote:

This looks to be a variant of issue #2692:

https://github.com/rails/rails/pull/2692

although the patch in that instance wouldn't fix this bug, as the detection heuristic will guess that image_page is a root element to be removed.

It appears that the problem is that remove_root doesn't have any awareness of the context in which it's operating - really, we should only be removing the appropriately-named root element (if it exists).

--Matt Jones

Thank you Matt, it does appear to be the same issue.

[Rodrigo Rosenfeld Rosas]

Em 03-03-2012 12:41, Mark Peterson escreveu:

On Saturday, March 3, 2012 9:23:16 AM UTC-5, Rodrigo Rosenfeld Rosas wrote:

Given the code below, it seems you're just starting using Rails and you
still don't understand Rails basic concepts.

Shouldn't you consider posting in the user's mailing list first? It's
more likely that you'll get better advices there than here...

After you get used to Rails, if you still have questions about how Rails
should do things, then your questions will be better discussed here.

Cheers,
Rodrigo.

Are you referring to my complete and utter lack of syntactic sugar?
It almost seems as though I deliberately avoid the stuff completely :)

Please don't remove text from the threads. Not everyone reads them in Gmail and it makes it really hard to read the messages in an e-mail client, like Thunderbird.

---

class User < ActiveResource::Base
  self.site = "http://localhost:9000"
end"

---

I don't call this "lack of syntactic sugar". This is completely wrong Ruby code and makes me feel that you don't know Ruby enough. Please, post the full code as it is actually easier to read the code with all its "syntactic sugar".

This might be the wrong forum, but I'm sure that ActiveResource team are happy that I proofed the issue.

As an expert core member Rodrigo, do you know the alternative to this line?
Dir.glob("#{Rails.root}/app/models/*.rb").sort.each { |file| require_dependency file }

I'm not a Rails core member, nor even an expert in Rails. And I don't know why you need this. Shouldn't Rails be automatically loading those files? Why should they be loaded in any particular order?

Cheers,
Rodrigo.

[Mark Peterson]

This is as full as I need my User.rb model to be, in practice and for the proof on this thread. Lord praise the fat model, which is encapsulated nicely inside of ActiveResource and ActiveModel.

Common sense advice: When determining how fat your model is, also consider the classes/modules from which your model inherits.

You don't have to add unnecessary filler code to your model to make it fatter than the controller from which it is invoked. Though I can imagine that there's some in syntactic sugar land that will pervert the meaning of "fat model / skinny controller" in that way :)

And syntactic sugar is not legible to the next person who is looking at your code.

Thank you for the advice about trimming the quoted text.

 

[kristian]

On Sat, Mar 3, 2012 at 9:44 PM, Rodrigo Rosenfeld Rosas >

> ---
>
> class User < ActiveResource::Base
>   self.site = "http://localhost:9000"
> end"
> ---
>
> I don't call this "lack of syntactic sugar". This is completely wrong Ruby
> code and makes me feel that you don't know Ruby enough. Please, post the
> full code as it is actually easier to read the code with all its "syntactic
> sugar".
>

please have a look at
http://api.rubyonrails.org/classes/ActiveResource/Base.html
and see where this is coming from. but maybe I might just got the
whole thing wrong ;-)

regards, Kristian

[Mark Peterson]

But he had this strong sense that I did something wrong. Shouldn't that be good enough for you?!?!?!?!

He glanced at bitter, unsweetened, functionally sound and readable code. Is it his fault that he just assumed?

:)

[Rodrigo Rosenfeld Rosas]

Yes, it was my fault here as I haven't used ActiveResouce::Base before and I was assuming that "self.site" was a random attribute to be included in JSON.

Sorry for the confusion and bad assumptions. Clearly I wasn't in a good day after fighting the whole week with Groovy:

http://rosenfeld.herokuapp.com/en/articles/ruby-rails/2012-03-04-how-nokogiri-and-jruby-saved-my-week

Sorry again,
Rodrigo.

[Mark Peterson]

Apology conditionally accepted on the grounds that you look at this JRuby/Tomcat HTTP streaming issue that I encountered, and point me in the right direction.

https://groups.google.com/forum/?fromgroups#!searchin/rubyonrails-talk/jruby/rubyonrails-talk/aIS8vlFO1qY/taex37-BveMJ

[Rodrigo Rosenfeld Rosas]

Hehe :) Where did you find the documentation for the "stream" parameter in "render"? I couldn't find the documentation for "render" itself in the Rails API site... :(

The documentation has really changed *a lot* since Rails 1 era... :(

[Mark Peterson]

http://api.rubyonrails.org/classes/ActionController/Streaming.html

[Rodrigo Rosenfeld Rosas]

I'll have to apologize again Mark, but this weekend was a long one (too much parties) and I couldn't set up some time for helping you on this and I have tons of work to do this next week and I won't have energy to help you with this at night.

I don't have any experience deploying JRuby on Rails applications in Java containers.

But I'd just like to make sure that you know what streaming support currently means in Rails.

I'm interested in real streaming support in Rails for several years now, since it was completely removed by Rails 3. The last time I checked, Rails only has partial streaming support.

This means that you won't be able to send small dynamic chunks of HTML and flush them before your action ends. You'll only be able to send your template headers first while you process the action in the controller. This is meant to reduce the client-side code and style latency. If that is what you're looking for, I'd suggest you to open a JIRA in JRuby and I think they'll be willing to help you to identify who is the culprit for this bug.

Good luck,
Rodrigo.

[Mark Peterson]

HTTP streaming, as Rails implemented it, seems to work fine. Sleeps (in the layout, not the view) invoke a flush so far as I've seen, so that gives me some ideas. But even if it's just streaming of 3 chunks (before view, during view, after view), then that's fine too.

That's a good idea of opening a ticket with JRuby.