yosiat / panko_serializer

High Performance JSON Serialization for ActiveRecord & Ruby Objects
https://panko.dev
MIT License
592 stars 36 forks source link

panko <3 breaks with Rails 8.0.0.beta1 #166

Open elthariel opened 1 month ago

elthariel commented 1 month ago

Hello,

I'm starting a pet project using the latest rails version 8.0.0.beta1, and it seems the latest ActiveRecord breaks Panko:

Here's a simple Serializer:

class LoopSerializer < Panko::Serializer
  attributes :id
end

I'm calling it from the rails console with LoopSerializer.new.serialize(Loop.first)

and getting the following stacktrace:

panko_serializer (0.8.2) lib/panko/serializer.rb:141:in `serialize_object': undefined method `default' for an instance of ActiveRecord::Result::IndexedRow (NoMethodError)

      Panko.serialize_object(object, writer, @descriptor)
           ^^^^^^^^^^^^^^^^^
    from panko_serializer (0.8.2) lib/panko/serializer.rb:141:in `serialize_with_writer'
    from panko_serializer (0.8.2) lib/panko/serializer.rb:130:in `serialize'
    from (sked):3:in `<main>'
    from <internal:kernel>:187:in `loop'
    from rails (865a3a2ef5cf) railties/lib/rails/commands/console/irb_console.rb:129:in `start'
    from rails (865a3a2ef5cf) railties/lib/rails/commands/console/console_command.rb:59:in `start'
    from rails (865a3a2ef5cf) railties/lib/rails/commands/console/console_command.rb:8:in `start'
    from rails (865a3a2ef5cf) railties/lib/rails/commands/console/console_command.rb:87:in `perform'
    from thor (1.3.2) lib/thor/command.rb:28:in `run'
    from thor (1.3.2) lib/thor/invocation.rb:127:in `invoke_command'
    from rails (865a3a2ef5cf) railties/lib/rails/command/base.rb:178:in `invoke_command'
    from thor (1.3.2) lib/thor.rb:538:in `dispatch'
    from rails (865a3a2ef5cf) railties/lib/rails/command/base.rb:73:in `perform'
    from rails (865a3a2ef5cf) railties/lib/rails/command.rb:65:in `block in invoke'
    from rails (865a3a2ef5cf) railties/lib/rails/command.rb:143:in `with_argv'
    from rails (865a3a2ef5cf) railties/lib/rails/command.rb:63:in `invoke'
    ... 5 levels...

The actual problem is likely in the native extension, but the call to default is likely a side effect and I can't trace it.

Do you have any thougts ?

elthariel commented 1 month ago

Using LoopSerializer.new.serialize(Loop.first.attributes) seems to work

elthariel commented 1 month ago

It seems the type of the @values field of @attributes of ActiveRecord::Result has changed to ActiveRecord::Result::IndexedRow

They're trying to mimick the API of a Hash but it's not one, so that's probably the cause of it

elthariel commented 1 month ago

Calling the private method attributes of the LazyAttributeSet makes the lazy set materialize the data and fixes it

elthariel commented 1 month ago

Finally, it comes down here: https://github.com/yosiat/panko_serializer/blob/master/ext/panko_serializer/attributes_writer/active_record.c#L108

and tries to use the lowlevel hash primitive to access the data, but it's not a real hash anymore, so it fails, and it tries to get the default value calling default.

From there I see two options:

In all the case, panko will incur a performance penalty from this new design in AR.

TBH, this was my first time digging into the Ruby C API, I don't think I'm the right person to implement at fix :-/

elthariel commented 1 month ago

In the meantime, a subpar workaround is to override the method serialize_with_writer in your Serializer classes (ideally in ApplicationSerializer if you have one) with this:

  def serialize_with_writer(object, writer)
    object.attributes # Materializes the LazyAttributeSet
    super
  end
yosiat commented 1 month ago

Hi @elthariel

Thank you for this investigation! I plan to fix this as part of: https://github.com/yosiat/panko_serializer/pull/167 .. It will take time to fix it, since I want to wait for stable rails 8 version release.

pjpires commented 1 month ago

I'm also facing the same issue in my upgrade to try out rails 8, but unfortunately the solution pointed out by @elthariel is not working for me.

What I noticed by opening the activerecord gem and trying some changes is that removing the call to indexed_rows that was added here makes all my specs pass (without changes I'm also facing the same "undefined method `default'" issue).

I don't have enough knowledge of the inner workings of AR to understand if that means the issue should be fixed at rails itself, just thought I'd point it out here in case it helps.

yosiat commented 1 month ago

@pjpires hey, i am working on rails 8 support it will take sometime.

I don't have enough knowledge of the inner workings of AR to understand if that means the issue should be fixed at rails itself, just thought I'd point it out here in case it helps.

This issue should be fixed in Panko, since Panko is peeking into Rails internals..

yosiat commented 1 month ago

@pjpires @elthariel I merged a fix, please test it (via installing from GitHub, from master branch) and report if it works for you 🙏

gem 'panko_serializer', github: 'yosiat/panko_serializer', branch: 'master'
pjpires commented 1 month ago

Hey @yosiat.

I ran our test suite with your master branch both against rails 8.0.0.beta1 and rc1 which is out already, and everything looks good! Thanks for such a fast response. 🚀

Uaitt commented 1 week ago

@yosiat I was facing the same issue while upgrading one of my Rails app to Rails 8.0.0 stable and I solved it with:

gem 'panko_serializer', github: 'yosiat/panko_serializer', branch: 'master'

Can you please release a new version of the gem that contains the aforementioned fix?