Class reopening and inheritance.

[marshall-lee]

Maybe it's just a corner case but maybe not.

I also think it could be related to #120 but I'm not sure.

[dblock]

I would expect this to succeed, so I say it's a bug.

[marshall-lee]

@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.

[dblock]

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?

[marshall-lee]

@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).

[marshall-lee]

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.

[marshall-lee]

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.

[dblock]

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?

[marshall-lee]

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.