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

Refactoring: extract exposures. #151

Merged
merged 1 commit into from
Aug 2, 2015

Conversation

marshall-lee
Copy link
Member

This one reaches many goals at once:

@marshall-lee
Copy link
Member Author

At first glance this change may seem too big. But every move here is reasonable.
For instance It's impossible to solve #56 without removing nested_exposures. Removing nested_exposures requires different way to represent nesting trees so I cannot find something better than extract exposure logic from Entity at all.

@marshall-lee
Copy link
Member Author

But some problem is introduced with changing exposures from hash table to an array so grape-swagger will not work with this patch. And it's even unclear how to generate documentation now. Imagine this case:

expose :x, using: E1, if: :condition1
expose :x, using: E2, if: :condition2

Which one should the swagger pick: E1 or E2 ?

I can help with implementing a workaround for swagger but for now I don't know exactly how to fix it.

Maybe bring back construction of exposures as a hash table and deprecate it?
Or maybe fix grape-swagger and bump version requirement for grape-entity?

@dblock
Copy link
Member

dblock commented Jul 22, 2015

I'd like to merge this, it needs a detailed CHANGELOG entry and we have to discuss what to do about swagger. The conflicting cases should just have some predictable behavior.

@marshall-lee
Copy link
Member Author

I just read through swagger specification and didn't find anything that might help gracefully solve this issue.

But:

  1. It tries to analyze only those exposures that has a :documentation option.
  2. It fetches exposure_options by name (which now is not unique) to get its :using option only if there is no :type field in :documentation of exposure.
  3. Grape::Entity.documentation (which is used by grape-swagger) still has an old rewriting nature. But I think that in this case it's good. Ones who use conditional exposures with different using values are forced to specify one :type for exposures with the same key (:as value or attribute name). This :type value could be simply 'object' or some 'AbstractEntity'.
  4. According to previous point for any keygrape-swagger should pick :type from documentation or :using (if there's no :type) from the last exposure with this key and it's already implemented in documentation.
  5. Grape::Entity.documentation still has an issue described in Nested Exposure attributes are flatten in documents #112. I propose to solve it traversing only root_exposures, not exposures.
  6. Remove flat array of exposures. There's no need in this now.

I'd like to propose a workaround to grape-swagger based on a fact that new entities do not respond to exposures method. It would also require adding different versions of grape-entity to grape-swagger's travis matrix.

Any complains?

@dblock
Copy link
Member

dblock commented Jul 22, 2015

No complaints. Grape-swagger needs a lot of work too and as long as we offer people a path forward I'm cool with it. Please at the very least open issues with grape-swagger for anything introduced here.

@dblock
Copy link
Member

dblock commented Jul 24, 2015

@marshall-lee Lets get this merged? Amend CHANGELOG? We also probably need an UPGRADING document.

@marshall-lee
Copy link
Member Author

@dblock I'm working on CHANGELOG right now.
I'd like to write UPGRADING.md too. To what version we're upgrading? 0.5.0? Maybe change lib/grape_entity/version.rb in this patch too?

And what format should I use in UPGRADING?

@dblock
Copy link
Member

dblock commented Jul 24, 2015

0.5.0 makes sense

@marshall-lee
Copy link
Member Author

Oops, JRuby build failed. https://travis-ci.org/intridea/grape-entity/builds/72464413

Latest changes revealed some misbehavior in JRuby, I reported it here: jruby/jruby#3178

@marshall-lee
Copy link
Member Author

LOL, Now Rubocop suggests me in https://travis-ci.org/intridea/grape-entity/jobs/72467983:

Pass &:nesting? as an argument to take_while instead of a block.

But take_while with &:method is buggy in JRuby as I mentioned above.

@marshall-lee
Copy link
Member Author

But it's okay on jruby-head, lol.

@marshall-lee
Copy link
Member Author

@dblock I'm almost ready. Maybe you release 0.4.6 without these changes? Or just put all pending changes to 0.5.0?

@dblock
Copy link
Member

dblock commented Jul 24, 2015

@marshall-lee You've done some great work here. I emailed you about becoming a maintainer on grape-entity. Your first order of duty could be to release 0.4.6?

@marshall-lee marshall-lee force-pushed the exposures branch 3 times, most recently from 0983a92 to 050e965 Compare July 27, 2015 17:27
This one reaches many goals at once:

 - Fixes ruby-grape#56.
 - `exposures` hash table is removed and substituted with
   `root_exposures` array.
 - Due to previous point, tree structure `nested_exposures` based on
   flat hash table with keys like `root_node__node__node` is removed
   because now double exposures don't rewrite previously defined
   so such tree structure is simply incorrect.
   `NestingExposure` with an array of children is introduced instead.
 - Ones who want an old rewriting behavior should manually `unexpose`.
 - Fixes ruby-grape#112.
 - Fixes ruby-grape#149: runtime `options` are now wrapped in an `Options` object.
 - Fixes ruby-grape#150: see new `spec/grape_entity/exposure_spec.rb`.
 - Fixes ruby-grape#152.
 - Fixes ruby-grape#153.
 - Fixes ruby-grape#155.
 - Fixes ruby-grape#156.
 - All exposure configuration and exposing strategies are extracted to
   the separate classes.
 - `key_for`, `name_for` internal methods are removed.
 - Much of the overhead is gone so performance is increased by 15-30%.
@u2
Copy link
Contributor

u2 commented Aug 3, 2015

👍

@rngtng
Copy link
Contributor

rngtng commented Dec 10, 2015

great stuff, too bad it breaks https://github.com/Gild/ruby-swagger too. How does the UPGRADE look like? next time pls deprecate the method first..

@rngtng
Copy link
Contributor

rngtng commented Dec 10, 2015

@rngtng
Copy link
Contributor

rngtng commented Dec 10, 2015

ok this did the trick for me, super hacky, dirty and for sure not complete...

class BaseDecorator < Grape::Entity
    def self.exposures
      extract_exposures(root_exposures, {})
    end

    private

    def self.extract_exposures(root, hash, prefix = nil)
      root.each do |entity|
        key = "#{prefix}#{entity.attribute}"
        nested = prefix.nil? ? {} : { nested: true }
        if entity.is_a?(Grape::Entity::Exposure::NestingExposure)
          hash[key.to_sym] = {}
          extract_exposures(entity.nested_exposures, hash, key + '__')
        else
          hash[key.to_sym] = entity.send(:options).merge(nested)
        end
      end
      hash
    end
  end

@dblock
Copy link
Member

dblock commented Dec 11, 2015

@rngtng Want to give fixing ruby-grape/grape-swagger#314 a try?

@rngtng
Copy link
Contributor

rngtng commented Dec 15, 2015

@dblock I'd love to, if I find the time...let me see..

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.

None yet

5 participants