alba icon indicating copy to clipboard operation
alba copied to clipboard

initial proof of concept of a nested_hash method

Open jrochkind opened this issue 2 years ago • 5 comments

A very simple proof of concept of the nested_hash concept we were talking about a bit in #233

The implementation here isn't quite ready for merge, the way I make a special Object.new singleton with a to_sym method, to get around the yield_if_within call at https://github.com/okuramasafumi/alba/blob/bba0d91cfa4ad7e7f47a5874a6bd63dcf7c3cfd6/lib/alba/resource.rb#L209 ... is very hacky.

But kind of amazing this can work with so few changes to code, leaning on the Association class? It is in fact very similar to an inline definition of a one association, just... without an association.

No tests in the PR, but this works:

person = OpenStruct.new(
  membership_number: 12988,
  member_name: "Terry Bradshaw",
  member_address1: "5555 West Test",
  member_city: "Miami",
  member_state: "FL"
)

class TestSerializer
  include Alba::Resource

  attribute :number, &:membership_number

  nested_hash :member do
   attribute :name, &:member_name
  end
end

TestSerializer.new(person).serializable_hash

# => {:number=>12988, :member=>{:name=>"Terry Bradshaw"}}

I know you may or may not be interested in this idea, but I got curious to see if I could make a proof of concept at least this afternoon, after realizing it might be very similar to an inline association resource, to see where I could get, and what you thought.

(and still don't know about the name nested_hash. nested_object? nest_object?)

jrochkind avatar Sep 15 '22 20:09 jrochkind

See also, to do something in some ways resembling json:api...

nested_hash :data do
  nested_hash :attributes do
     attributes :whatever, :etc
  end
end

And separately... I think I'd put support for params back, so you can have different params in a nested_hash block, why not.

jrochkind avatar Sep 15 '22 21:09 jrochkind

You could also DIY your own root_key/meta....

nested_hash :my_root_key do
    attributes :whatever
end
attribute(:meta) { params[:meta] }
# and hey, maybe we want to add another block...
attribute(:links) { params[:links] }

Making it more flexible for making json whatever "shape" you want, for use by different clients or standards, instead of assuming/enforcing a certain standard "Rails" shape.

jrochkind avatar Sep 15 '22 21:09 jrochkind

Wow this is hacky! I personally prefer adding a class for nested_hash DSL, but if this implementation works well it's fine.

With this implementation, we can still set condition proc but it should not work. To make sure this works for many cases, could you add some tests?

okuramasafumi avatar Sep 16 '22 01:09 okuramasafumi

There is no argument in the nested_hash method to set conditional proc, so you can not set it via nested_hash to be passed on to Association, it is not possible (as it should not be). You can set an if option, which is desirable.

Yes, if you are interested in this approach, I can try to carry it further next week!

In addition to adding tests (and some documentation, including in comment docs to Association mentioning it's used for nested_hash too), I wonder if there's a way to clean things up so that to_sym implementation isn't needed. perhaps changing yield_if_within(attribute.name.to_sym) to yield_if_within(attribute.name), letting yield_if_within accept nil and/or special Object token, and do to the to_sym itself internally only if not.

I don't find re-using Association itself to be hacky, if it can be done smootly -- this really is just like an inline association, but with the target remaining the original object instead of an association. It's the presence of in-line associations that made me think "alba an already almost do this, so maybe..."

jrochkind avatar Sep 16 '22 12:09 jrochkind

@jrochkind I implemented my own version at #237 so let's compare these approaches. Could you copy test case from my PR so that we're sure both work the same?

okuramasafumi avatar Sep 16 '22 15:09 okuramasafumi

Thanks @okuramasafumi!

Your implementation does look better than mine.

When running your tests against my implementation, I get one failure... I think it may be my implementation failing though?

  1) Failure:
NestedAttributeTest#test_deeply_nested_attribute [/Users/jrochkind/code/alba/test/usecases/nested_attribute_test.rb:60]:
--- expected
+++ actual
@@ -1 +1 @@
-"{\"foo\":{\"bar\":{\"baz\":{\"deep\":42}}}}"
+"{\"foo\":{\"bar\":null}}"

I'm not sure why my implementation fails to deeply nest, very odd! But it doesn't really matter if we are going to abandon this implementation for your preferable one in #237, which makes sense to me, so i will close this PR!

Thank you for your work!

jrochkind avatar Sep 19 '22 14:09 jrochkind