Handle has_* :through associations on rails 5.1+

SchemaPlus / schema_associations

ActiveRecord extension that automatically (DRY) creates associations based on the schema

Other

46 stars 8 forks source link

Handle has_* :through associations on rails 5.1+ #26

Closed urkle closed 5 years ago

urkle commented 5 years ago

In rails 5.1+ has any through associations have to reference an association defined BEFORE them.. Thus w/ Schema_associations we need to allow defining these manual associations after schema+ has defined the automatic ones.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 24eebf5c23975733f0f6577c9f77279b824a2ab4 on feat-rails-5.2 into cd0a3af36ffef352a62742874ca3e9f9ea2c26b0 on master.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 24eebf5c23975733f0f6577c9f77279b824a2ab4 on feat-rails-5.2 into cd0a3af36ffef352a62742874ca3e9f9ea2c26b0 on master.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 24eebf5c23975733f0f6577c9f77279b824a2ab4 on feat-rails-5.2 into cd0a3af36ffef352a62742874ca3e9f9ea2c26b0 on master.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 06ffaee81e03864d15bf30cf592e3bc60cff2584 on feat-rails-5.2 into cd0a3af36ffef352a62742874ca3e9f9ea2c26b0 on master.

urkle commented 5 years ago

@lowjoel can you re-review this? I redid the test to be a slightly more practical example. also I added documentation n the README and bumped the version to 1.3.0

urkle commented 5 years ago

@lowjoel @ronen this is blocking some other work that needs this update.. Can I get one of you to look over this and approve the changes I made?

lowjoel commented 5 years ago

Sorry for the delay, I don't have a good Internet connection these few days. Please send a chaser if you need sooner feedback.

I'm in two minds on this. On one hand this defines a new macro that really shouldn't be needed. I think we should probably override has_many to look for through: and figure out that we have to defer it automatically at the end of the process.

On the other hand that proposal is rather magical. Having said that, the point of schema associations is to be DRY, so... thoughts?

The first idea can be implemented by just recording all has_many through: and running those at the end of defining all automatic schemas, much like how you've defined post_block (call the new variable _deferred_associations or something.) I should've thought about this when doing the initial review, sorry.

urkle commented 5 years ago

@lowjoel hmm.. that actually sounds like a good idea.. Though really I should be able to just do it with ALL associations that are defined (and some magic to prevent looping). I'll make a fixup w/ that approach and throw it to you.

urkle commented 5 years ago

@lowjoel I pushed up fixups that rework the behavior to be deferred for has_many and has_one (the only associations that support :through) I also backed down the version to 1.2.7 (instead of 1.3.0) since this does not change any public contracts now.

urkle commented 5 years ago

@lowjoel thanks for the review.. I've made adjustments as you requested.

urkle commented 5 years ago

@lowjoel this should be good now. everything is passing and there is no longer any contract change

lowjoel commented 5 years ago

Awesome, thanks!

I believe that the differences matter more for smaller classes, Ruby has a specialisation for classes with a small number of ivars, they transition to a hash table beyond a certain number. Not sure if remove_instance_variable will even cause a reverse transition.

Having said that AR objects have a gazillion ivars, so... 🤷‍♂️ but it's good we thought about it.