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

Fix nested serializer lookup #34

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

stokarenko
Copy link
Collaborator

It fixes #30

undefined method 'load_all_lazy_relationships' for nil:NilClass exception is raising in specific env.

Lets fix two yet different causes of this:

  1. host application uses customized serializers lookup (ActiveModelSerializers.config.serializer_lookup_chain), so ams_lazy_relationships should use the same lookup in order to get the same result;
  2. in case of missed serializer, ActiveModelSerializers will inline nested object right into relationships as it is, so ams_lazy_relationships should keep rolling as well.

@Bajena Bajena self-requested a review December 27, 2019 16:16
@Bajena Bajena self-assigned this Dec 27, 2019
@Bajena
Copy link
Owner

Bajena commented Dec 27, 2019

Great work @stokarenko! Really impressive 🎩 Thanks for your time and high quality PR (nice descriptions and specs).

Btw. if I may ask - how did you get to know this gem and how are you using it?

@Bajena Bajena merged commit 5b2bf0d into Bajena:master Dec 27, 2019
@Bajena
Copy link
Owner

Bajena commented Dec 27, 2019

Unfortunately the master build has failed after merge (https://travis-ci.com/Bajena/ams_lazy_relationships/builds/142470412).
I suppose the older versions of active model serializers didn't support the serializer lookup chain feature.

If you still have some time (and patience :D) it'd be great if you took at them. If not I'll try fixing them (probably by disabling the new specs depending on the AMS version).

@stokarenko
Copy link
Collaborator Author

Hey @Bajena , nice to meet you! )

thanks a lot for your gem, it works like a charm )

if I may ask - how did you get to know this gem and how are you using it?

well, I'm trying to solve a massive N+1 issues within a large SOA project. We realized that active-model-serializer will fetch every association defined in serializer just for maintaining so-called relationships data. I.e. it fires a massive N+1 even without explicit includes.. o_O

So I started to develop my own solution for this, fixed N+1 at least for relationships data, started to think how to preload the things for explicit includes as well, came to laziness idea, then asked google is there is something like this - bingo )

If you still have some time (and patience :D) it'd be great if you took at them.

definitively I will do that on 6th Jun, right after vacation )

also I would like a push one more PR here, it will be a kind of dig but for nested associations, like

class SomeSerializer

  def some_attr
    lazy_dig(:organization, :logo)&.url
  end
end

it is already implemented, just need to tune a code a bit and write specs ) That is the last thing which turns your gem into a graceful rescue for my project )

@Bajena
Copy link
Owner

Bajena commented Dec 29, 2019

thanks a lot for your gem, it works like a charm )

Great to hear it. I had a very similar motivation for building it - we have a big API with lots of controllers loading nested data. It'd be really hard to specify proper Rails-style includes for all of them and maintain it. Also bullet gem wasn't very useful in our case.

definitively I will do that on 6th Jun, right after vacation )

🙇

also I would like a push one more PR here, it will be a kind of dig but for nested associations, like

Yeah, great idea. I already had a few cases when I needed to specify a lazy relationship only for fetching one attribute. Looking forward to seeing it!

@Bajena
Copy link
Owner

Bajena commented Dec 29, 2019

Btw @stokarenko you seem to be a very proficient developer. I'd be honored to grant you a maintainer privilege if you'd like. Would you be interested in that?

@stokarenko
Copy link
Collaborator Author

@Bajena

If you still have some time (and patience :D) it'd be great if you took at them.

definitively I will do that on 6th Jun, right after vacation )

Here we go! ) #43

lazy_dig is coming as well )

I'd be honored to grant you a maintainer privilege if you'd like. Would you be interested in that?

Sure thing, thank you! Lets do the great things together )

@Bajena
Copy link
Owner

Bajena commented Jan 8, 2020

Sure thing, thank you! Lets do the great things together )

Great! I've sent you an invitation to collaborate 🎉

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

Successfully merging this pull request may close these issues.

undefined method `load_all_lazy_relationships' for nil:NilClass
2 participants