grape-entity
grape-entity copied to clipboard
Class reopening and inheritance.
Maybe it's just a corner case but maybe not.
I also think it could be related to #120 but I'm not sure.
I would expect this to succeed, so I say it's a bug.
@dblock
Lets see, I have a working solution now but I don't like it for some reasons.
I introduced inherited_entities
and call #expose
and #format_with
on them when they're called on parent class. It's simple.
But I don't like the way how inherited_entities
are collected. My first solution used this technique:
def self.inherited_entities
sclass = class << self; self; end
ObjectSpace.each_object(sclass).to_set.delete(self)
end
I like this much more because I don't want to store child classes in array. Because when someone call remove_const
for child class then it's expected for this class to be garbage collected. But since class reference is stored in array it doesn't.
Solution with ObjectSpace
is not portable to JRuby since:
RuntimeError: ObjectSpace is disabled; each_object will only work with Class, pass -X+O to enable
I can detect if running under JRuby and determine child classes using ObjectSpace.each_object(Class)
with filtering but it's probably not so effective. On the other hand we don't need performance in such places because it's being called only in initialization.
There's also a problem with unexposures since we have this spec:
subject.expose :name, :email
child_class = Class.new(subject)
subject.unexpose :email
expect(subject.root_exposures.length).to eq 1
expect(subject.root_exposures[0].attribute).to eq :name
expect(child_class.root_exposures[0].attribute).to eq :name
expect(child_class.root_exposures[1].attribute).to eq :email
They're expected to not affect children but exposures are. So it won't be intuitive if some things are truly inheritable but some other are just half inheritable (unexposures).
I propose to make this things behave identical: remove the spec I mentioned above and make exposures, unexposures and formatters be inheritable. But since it's a breaking change just put it only to 0.5 since it's already breaking. And of course describe this change in UPGRADING.
The code that's here is a lot simpler than what you're describing and inherited_entities
are collected when inherited
. What am I missing?
@dblock
It could be buggy with code reloading feature but I'm not sure. Depends on how it's implemented.
Ideally, class object should be referenced only by a constant to which it's assigned. But this implementation references class also in inherited_entities
so there's no possibility to completely delete a class. One would assume a class to completely disappear when calling remove_const
but inherited_entities
could still store a reference to such class (now unnamed).
Oh, there's another reason not to choose this way at all.
class A < Grape::Entity
expose :x
end
class B < A
expose :z
end
A.expose :y
In current implementation :y
will be exposed after :x
in A
, and after :z
in B
but for B
i would expect to see :y
between :x
and :z
({ x: 1, y: 2, z: 3 }
). We should preserve the order of exposures, especially it matters when there're conditional exposures.
So I need to invent a new solution.
So my intuition is that anything that happens in the parent class should be "prepended" to the child class — exposures, unexposures, formatters, etc. Even if I reopen parent class after inheritance.
Oh my. I feel largely unqualified to make calls here.
I think in your example I would survive the current behavior just fine, of :y
being exposed between :x
and :z
. After all it was exposed after the fact, so why should it somehow be inserted? And otherwise why does it really matter anyway as long as the behavior is predictable?
Lets describe it in terms of RSpec:
it 'puts exposures in the right order' do
subject.expose :x
child_class = Class.new(subject) do
expose :z
end
subject.expose :y
object = {
x: 1,
y: 2,
z: 3
}
expect(child_class.represent(object, serializable: true).keys).to eq([:x, :y, :z])
end
it 'just prepends parent class exposures to the inherited class' do
subject.expose :x
child_class = Class.new(subject) do
expose :y, proc: -> (_obj, _opts) { 'specific' }
end
subject.expose :y
object = {
x: 1,
y: 2
}
expect(child_class.represent(object, serializable: true)).to eq(x: 1, y: 'specific')
end
This two new specs are not satisfied https://travis-ci.org/ruby-grape/grape-entity/jobs/74816055.