This repository has been archived by the owner. It is now read-only.

Problem with response.to_json #18

Merged
merged 4 commits into from Aug 30, 2013

Conversation

Projects
None yet
4 participants
@kamui
Collaborator

kamui commented Aug 27, 2013

Hi first of all nice project.

I start using it a couple of days ago and run into an issue with the documentation generation in wd-sinatra I didn't raise the issue there cause it seems to be in the response object of Weasel-Dieasel

Here is the response definition:

  service.response do |response|
    response.object do |obj|
      obj.string :status, :doc => "Response status either OK or ERROR"
      obj.string :message, :doc => "The error message if an error occur", :null => true
      obj.element :name => :payload, :type => 'Resource' do |payload|
        payload.string :id, :doc => "Resource id"
        payload.string :name, :doc => "Resource name"
        payload.string :path, :doc => "Resource full path"
        payload.object :metadata, :doc => "Resource metadata"
        payload.array :tags, :doc => "Resource list of tags"
        payload.datetime :created_at, :doc => "Created at timestamp"
        payload.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

And this is the JSON result of api.response.to_json

{
  "status": "string",
  "message": "string",
  "payload": {
    "payload": {
      "id": "string",
      "name": "string",
      "path": "string",
      "created_at": "datetime",
      "updated_at": "datetime",
      "metadata": {
        "metadata": {

        }
      }
    }
  }
}

If you look at the payload attribute it has a nested payload attribute with the actual payload attributes, thats not what I intended and maybe my declaration is wrong.

This is what I want the payload attribute to look like:

{
  "status": "string",
  "message": "string",
  "payload": {
      "id": "string",
      "name": "string",
      "path": "string",
      "created_at": "datetime",
      "updated_at": "datetime",
      "metadata": {
        "metadata": {

        }
    }
  }
}
@mattetti

This comment has been minimized.

Show comment Hide comment
@mattetti

mattetti Nov 20, 2012

Owner

That looks like a bug due to the XML/JSON semantic difference, try:

service.response do |response|
    response.object do |obj|
      obj.string :status, :doc => "Response status either OK or ERROR"
      obj.string :message, :doc => "The error message if an error occur", :null => true
      obj.object :payload do |payload|
        payload.string :id, :doc => "Resource id"
        payload.string :name, :doc => "Resource name"
        payload.string :path, :doc => "Resource full path"
        payload.object :metadata, :doc => "Resource metadata"
        payload.array :tags, :doc => "Resource list of tags"
        payload.datetime :created_at, :doc => "Created at timestamp"
        payload.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

I replaced your use of element with name by object I'm not quite sure why metadata is doubled tho.
Let me know if that fixes your problem and we can look in the problem.

Owner

mattetti commented Nov 20, 2012

That looks like a bug due to the XML/JSON semantic difference, try:

service.response do |response|
    response.object do |obj|
      obj.string :status, :doc => "Response status either OK or ERROR"
      obj.string :message, :doc => "The error message if an error occur", :null => true
      obj.object :payload do |payload|
        payload.string :id, :doc => "Resource id"
        payload.string :name, :doc => "Resource name"
        payload.string :path, :doc => "Resource full path"
        payload.object :metadata, :doc => "Resource metadata"
        payload.array :tags, :doc => "Resource list of tags"
        payload.datetime :created_at, :doc => "Created at timestamp"
        payload.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

I replaced your use of element with name by object I'm not quite sure why metadata is doubled tho.
Let me know if that fixes your problem and we can look in the problem.

@jarias

This comment has been minimized.

Show comment Hide comment
@jarias

jarias Nov 20, 2012

Actually I paste the response declaration I was just trying to see if that error would still show up.

This is the correct one:

  service.response do |response|
    response.object do |obj|
      obj.object :payload do |payload|
        payload.string :id, :doc => "Resource id"
        payload.string :name, :doc => "Resource name"
        payload.string :path, :doc => "Resource full path"
        payload.object :metadata, :doc => "Resource metadata"
        payload.array :tags, :doc => "Resource list of tags"
        payload.datetime :created_at, :doc => "Created at timestamp"
        payload.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

And payload and metadata are still duplicated.

jarias commented Nov 20, 2012

Actually I paste the response declaration I was just trying to see if that error would still show up.

This is the correct one:

  service.response do |response|
    response.object do |obj|
      obj.object :payload do |payload|
        payload.string :id, :doc => "Resource id"
        payload.string :name, :doc => "Resource name"
        payload.string :path, :doc => "Resource full path"
        payload.object :metadata, :doc => "Resource metadata"
        payload.array :tags, :doc => "Resource list of tags"
        payload.datetime :created_at, :doc => "Created at timestamp"
        payload.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

And payload and metadata are still duplicated.

@mattetti

This comment has been minimized.

Show comment Hide comment
@mattetti

mattetti Nov 20, 2012

Owner

There is something definitely wrong in the JSON representation, probably in https://github.com/mattetti/Weasel-Diesel/blob/master/lib/response.rb#L340-L356
The API usage seems fine so it's probably an issue in the representation. I'll try to reproduce the issue and fix it locally. I know that this part of the code is probably the weakest and it's a shame. Do you use the rake task to generate the doc and that's how you see the json structure?

Owner

mattetti commented Nov 20, 2012

There is something definitely wrong in the JSON representation, probably in https://github.com/mattetti/Weasel-Diesel/blob/master/lib/response.rb#L340-L356
The API usage seems fine so it's probably an issue in the representation. I'll try to reproduce the issue and fix it locally. I know that this part of the code is probably the weakest and it's a shame. Do you use the rake task to generate the doc and that's how you see the json structure?

@jarias

This comment has been minimized.

Show comment Hide comment
@jarias

jarias Nov 20, 2012

That is correct I'm using the rake task

jarias commented Nov 20, 2012

That is correct I'm using the rake task

@jarias

This comment has been minimized.

Show comment Hide comment
@jarias

jarias Nov 20, 2012

I also found another JSON error I was going to log it as a separate ticket.

  service.response do |response|
    response.object do |obj|
      obj.array :payload do |node|
        node.string :id, :doc => "Resource id"
        node.string :name, :doc => "Resource name"
        node.string :path, :doc => "Resource full path"
        node.object :metadata, :doc => "Resource metadata"
        node.array :tags, :doc => "Resource list of tags"
        node.datetime :created_at, :doc => "Created at timestamp"
        node.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

Renders:

{}

😬

jarias commented Nov 20, 2012

I also found another JSON error I was going to log it as a separate ticket.

  service.response do |response|
    response.object do |obj|
      obj.array :payload do |node|
        node.string :id, :doc => "Resource id"
        node.string :name, :doc => "Resource name"
        node.string :path, :doc => "Resource full path"
        node.object :metadata, :doc => "Resource metadata"
        node.array :tags, :doc => "Resource list of tags"
        node.datetime :created_at, :doc => "Created at timestamp"
        node.datetime :updated_at, :doc => "Resource updated at timestamp"
      end
    end
  end

Renders:

{}

😬

@deepj

This comment has been minimized.

Show comment Hide comment
@deepj

deepj Apr 4, 2013

I have the same problem. No attribute in array is converted to json.

deepj commented Apr 4, 2013

I have the same problem. No attribute in array is converted to json.

@deepj

This comment has been minimized.

Show comment Hide comment
@deepj

deepj Apr 4, 2013

For example, this example from README doesn't work properly at all...

      # OUTPUT
      # the response contains a list of player creation ratings each object in the list 
      service.response do |response|
        response.element(:name => "player_creation_ratings") do |e|
          e.attribute  :id          => :integer, :doc => "id doc"
          e.attribute  :is_accepted => :boolean, :doc => "is accepted doc"
          e.attribute  :name        => :string,  :doc => "name doc"

          e.array :name => 'player_creation_rating', :type => 'PlayerCreationRating' do |a|
            a.attribute :comments  => :string,  :doc => "comments doc"
            a.attribute :player_id => :integer, :doc => "player_id doc"
            a.attribute :rating    => :integer, :doc => "rating doc"
            a.attribute :username  => :string,  :doc => "username doc"
          end
        end
      end

deepj commented Apr 4, 2013

For example, this example from README doesn't work properly at all...

      # OUTPUT
      # the response contains a list of player creation ratings each object in the list 
      service.response do |response|
        response.element(:name => "player_creation_ratings") do |e|
          e.attribute  :id          => :integer, :doc => "id doc"
          e.attribute  :is_accepted => :boolean, :doc => "is accepted doc"
          e.attribute  :name        => :string,  :doc => "name doc"

          e.array :name => 'player_creation_rating', :type => 'PlayerCreationRating' do |a|
            a.attribute :comments  => :string,  :doc => "comments doc"
            a.attribute :player_id => :integer, :doc => "player_id doc"
            a.attribute :rating    => :integer, :doc => "rating doc"
            a.attribute :username  => :string,  :doc => "username doc"
          end
        end
      end
@kamui

This comment has been minimized.

Show comment Hide comment
@kamui

kamui Aug 27, 2013

Collaborator

I think I fixed both issues.

  1. When calling to_hash, the vectors are totally ignored, which is why arrays are not generated in the response. This was easy to fix, just concat vectors and elements.
  2. As far as I could tell, when invoking to_hash the current element isn't aware of it's parent node. You only need to wrap the element with a key when the element is named and when it's the root node. So I added an optional arg to to_hash called root_node, by default it's true, the recursive calls to to_hash pass false so it's not treated as the root node. This works, but I'm not sure if you would rather link elements together or perhaps there's someway to do this that I missed.
Collaborator

kamui commented Aug 27, 2013

I think I fixed both issues.

  1. When calling to_hash, the vectors are totally ignored, which is why arrays are not generated in the response. This was easy to fix, just concat vectors and elements.
  2. As far as I could tell, when invoking to_hash the current element isn't aware of it's parent node. You only need to wrap the element with a key when the element is named and when it's the root node. So I added an optional arg to to_hash called root_node, by default it's true, the recursive calls to to_hash pass false so it's not treated as the root node. This works, but I'm not sure if you would rather link elements together or perhaps there's someway to do this that I missed.

@kamui kamui referenced this pull request Aug 27, 2013

Closed

JSON generation problems #22

mattetti added a commit that referenced this pull request Aug 30, 2013

@mattetti mattetti merged commit 9b7f5b9 into mattetti:master Aug 30, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.