Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to "flatten" nested entities into parent (e.g. for CSV) #45

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

joelvh
Copy link
Contributor

@joelvh joelvh commented Jan 8, 2014

This feature allows you to specify another entity to copy exposures from. It's useful when flattening a hierarchy for CSV. It also adds the :object option to exposures that can define an alternate object to get values from. (Specifying an alternate object is necessary when copying exposures because the exposures don't refer to the original entity/model anymore.)

sethers and others added 3 commits January 7, 2014 17:29
Models that use single-table inheritance will be cast as specific classes that don't always have the attribute defined as a method. Checking to see if the `object` responds like a Hash allows more flexibility to get raw column data. We raise an `ArgumentError` to indicate that the attribute may be invalid.
else
object.send(attribute)
raise ArgumentError, ":attribute was unable to be found anywhere"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be specialized as an AttributeNotFoundException, with the attribute name as a field, then you don't need an English explanation.

@dblock
Copy link
Member

dblock commented Jan 8, 2014

This would need tests, README and CHANGELOG entries. And of course a passing build. Thanks for contributing!

@joelvh
Copy link
Contributor Author

joelvh commented Jan 9, 2014

@dblock updated with docs and tests.

@joelvh
Copy link
Contributor Author

joelvh commented Jan 9, 2014

@dblock updated the example in README

@dblock
Copy link
Member

dblock commented Jan 9, 2014

I want to play devil's advocate for this feature for a second. Can't this be accomplished outside of presenters? I mean flattening a JSON generically may be easier (like this or this).

@joelvh
Copy link
Contributor Author

joelvh commented Jan 9, 2014

@dblock Interestingly enough, we took that approach initially, and then wanted to try and utilize the Entity paradigm to help document the schema automatically.

Initially, we added an Entity#to_csv method that took the JSON representation and flattened it (similarly to the links you posted). However, we are using grape-swagger to generate schema documentation for the Swagger UI for testing the API. Flattening the data resulted in the documentation not being accurate -- additional properties magically appearing when using CSV.

The solution was to flatten the entities by copying the exposures so that the schema would describe all attributes, regardless of format. However, the only thing that I wasn't able to implement cleanly in this feature (that we had in our initial hacked-together version of this), was appending " (CSV only)" to the end of each description -- which would indicate why some attributes don't always appear.

You're right, we could simply flatten the data, but that didn't seem to be self-documenting enough and defeated the purpose of using Grape Entity for us :-/

@dblock
Copy link
Member

dblock commented Jan 10, 2014

I'd love to hear what others think of this - will leave it open for a bit. In the meantime you might want to squash the commits/rebase this PR.

@gsps
Copy link

gsps commented Feb 9, 2014

I stumbled upon this PR looking for that exact functionality.
From my point of view it's a perfectly legitimate feature. Consider for instance this case where the internal model doesn't match the representation in the API:

class ItemRevision
  (...) # versioned attributes a, b, c
end
class Item
  (...) # unversioned attributes x, y, z
  has_many ItemRevisions
end

class ItemRevisionEntity < Grape::Entity
  expose :a, :b, :c
end
class ItemEntity < Grape::Entity
  expose :x, :y, :z
  merge_with ItemRevisionEntity { object.current_revision }
  expose :revisions, using: ItemRevisionEntity, if: {history: true}
end

I.e., the versioning might be irrelevant for some public API and is therefore hidden by only including the current revision, while the revision entity might be useful on its own in other contexts.
Moreover, I agree with Joel that grape-entity derives most of its usefulness from authoritatively defining the (e.g. ReST) resource. Consequently I think it should reflect the exact format that calls to the API will give you.

If I can help please let me know, I'll be using this one way or the other.

@dblock
Copy link
Member

dblock commented Feb 10, 2014

I'll leave this open for a bit. FWIW, this morning my inclination is to merge this.

gsps added a commit to gsps/grape-entity that referenced this pull request Mar 19, 2014
@donbobka
Copy link

PR allows make entity partials in grape-entity. 👍
Example:

class DuckOptionsEntity < Grape::Entity
  expose :duck_specific_attribute
end
class DogOptionsEntity < Grape::Entity
  expose :dog_specific_attribute
end
class AnimalEntity < Grape::Entity
  expose :nickname, :type
  merge_with DogOptionsEntity, if: lambda { |object,options| object.dog? } 
  merge_with DuckOptionsEntity, if: lambda { |object,options| object.duck? } 
end

Usage:

present Animal.all, with: AnimalEntity

Result

[
  {
    "nickname": "puppy",
    "type": "dog",
    "dog_specific_attribute": "value"
  },
  {
    "nickname": "scrooge",
    "type": "duck",
    "duck_specific_attribute": "value"
  }
]

Is there any way to make it with standard grape-enitity?

@dblock
Copy link
Member

dblock commented Apr 25, 2014

I am still sitting on this, I'll take a close look. Would appreciate other people's opinion. Generally, I think I want to weed the "flatten" part out of this, the merge_with part is great.

@brianphillips
Copy link

👍 on getting the merge_with bits pulled in

@dblock
Copy link
Member

dblock commented May 11, 2015

@brianphillips Want to try and extract a merge_with PR out of this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants