Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

panko <3 breaks with Rails 8.0.0.beta1 #166

Open
elthariel opened this issue Oct 16, 2024 · 11 comments
Open

panko <3 breaks with Rails 8.0.0.beta1 #166

elthariel opened this issue Oct 16, 2024 · 11 comments

Comments

@elthariel
Copy link
Contributor

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
Copy link
Contributor Author

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

@elthariel
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@elthariel
Copy link
Contributor Author

elthariel commented Oct 16, 2024

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:

  • Forcing the LazyAttributeSet to materialize the attributes
  • write a routine to acces the data in the new format (i.e. the double array)

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
Copy link
Contributor Author

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
Copy link
Owner

yosiat commented Oct 18, 2024

Hi @elthariel

Thank you for this investigation!
I plan to fix this as part of: #167 .. It will take time to fix it, since I want to wait for stable rails 8 version release.

@pjpires
Copy link

pjpires commented Oct 18, 2024

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
Copy link
Owner

yosiat commented Oct 19, 2024

@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 yosiat added the bug label Oct 20, 2024
@yosiat yosiat self-assigned this Oct 20, 2024
@yosiat yosiat mentioned this issue Oct 20, 2024
@yosiat
Copy link
Owner

yosiat commented Oct 20, 2024

@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
Copy link

pjpires commented Oct 21, 2024

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
Copy link

Uaitt commented Nov 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants