Fix or workaround for duplicate exposure caveat? about grape-entity HOT 13 CLOSED

That would be a bug. Start by writing a test that demonstrates the problem.

from grape-entity.

Added proof of concept here:
https://gist.github.com/nickls/9560205

Please let me know if you have any questions.

from grape-entity.

@nickls Can you please confirm whether this is fixed on HEAD?

from grape-entity.

This issue documented in caveats.
I wrote a failing test here donbobka/grape-entity@b73d3d9

Result

Failures:

  1) Grape::Entity class methods .expose when same exposures with different options using and if should generate right result
     Failure/Error: ]
       expected: [{:media=>{:sound_file=>"sound_file"}}, {:media=>{:photo_file=>"photo_file"}}]
            got: [{}, {:media=>{:photo_file=>"photo_file"}}] (using ==)
       Diff:
       @@ -1,2 +1,2 @@
       -[{:media=>{:sound_file=>"sound_file"}}, {:media=>{:photo_file=>"photo_file"}}]
       +[{}, {:media=>{:photo_file=>"photo_file"}}]

     # ./spec/grape_entity/entity_spec.rb:325:in `block (5 levels) in <top (required)>'

Workaround:
Use different names for exposures with option as and pass a representable object in block

class MediaEntity < Grape::Entity
  expose :media_sound, as: :media, using: SoundEntity, if: lambda { |object, _| object.sound? } { |object, _| object.media }
  expose :media_photo, as: :media, using: PhotoEntity, if: lambda { |object, _| object.photo? } { |object, _| object.media }
end

from grape-entity.

@nickls @donbobka @dblock Is this still being worked on, or can I help in any way? I ran into exactly the same problem today and would love to see this working (unless there's a specific reason for the exposures overwriting each other).

from grape-entity.

Would love a pull request that addresses the spec failure mentioned above.

from grape-entity.

Cool, I'll see what I can do.

from grape-entity.

I took @donbobka's test and adapted it a bit: https://github.com/Morred/grape-entity/tree/double-exposure

The test fails, and what happens is technically exactly what you're describing in the caveat section of the Readme, the second exposure overwrites the first exposure. Don't know if this is considered a bug (because you do document it in the Readme), but it would certainly be a lot more convenient if this did not happen.

from grape-entity.

Completely forgot the pull request >.< It's there now.

from grape-entity.

I don't see a bug here. Both https://gist.github.com/nickls/9560205 and #124 demonstrate documented behavior: exposure with the same name overwrite previously defined.

However, It would be good to not overwrite them. I think Grape::Entity's DSL for exposures is very similar to a regular ruby code so principle of least surprise says us that DSL is expected to behave as a regular Ruby code. For example:

# Grape::Entity:
expose :a
expose :a, if: -> (object, options) { object.some_condition? }

# Regular ruby code:
expose object.a
expose object.a if object.some_condition?

Looks similar, right?

I definitely want this feature to be implemented and I think I could do it. But it will break backward compatibility (i.e. that caveat in README about double exposures will gone) and it requires big changes in code.

So what should be done in examples:

expose :x, using: E1
expose :x, using: E2

Here x should be exposed using E2.

expose :x, using: E1, if: -> { some_condition? }
expose :x, using: E2

Here x should be exposed using E2 — no matter what the value of some_condition? is.

expose :x, using: E1
expose :x, using: E2, if: -> { some_condition? }

Here x should be exposed using E2 iff some_condition? is true, otherwise it should be exposed using E1.

expose :x, using: E1, if: -> { first_condition? }
expose :x, using: E2, if: -> { second_condition? }

Here:

• x should be exposed using E1 iff first_condition? is true and second_condition? is false.
• x should be exposed using E2 iff second_condition? is true — no matter what the value of first_condition? is.
• x should not be exposed at all iff first_condition? and second_condition? are both false.

I could write detailed failing specs for this and try to construct an implementation.

from grape-entity.

@marshall-lee, I think of expose as a method. If I make two method calls I expect it to be called twice, so I think this is the bug, especially if we think of it behaving like Ruby code. No?

from grape-entity.

@dblock

Yes and I like it.

I just said it's not a bug but a documented caveat. Double exposure overwrites previously defined — not matter they're conditional or not. And since it's documented, implementing it the right way is not backward compatible. Though I think that it would be better not being compatible with quirky behavior :)

from grape-entity.

I am ok with breaking backward compatibility here as long as we have an upgrade path for someone who wants that behavior.

from grape-entity.