Add tests for child relations support #6
Conversation
Fix Attacher#atomic_persist for embeded models. TODO: Fix nested atributes with cascade_callbacks support.
|
Thank you, this will definitely help in figuring out a solution for #5. Do you know why model reloading doesn't work with embedded documents? I thought it worked when I looked into the Mongoid source code, I recall seeing code that specifically handles embedded documents 🤔 |
|
@janko I didn't check their code yet, but that reloading was raising an error on embedded records. The error was not obvious at all and didn't look for it in their code either. However, that error could be "silenced" (with wrong behavior anyway) by telling Mongoid in which collection to look for the record, but that's nonsense, as there are no separate collections ("tables") for embedded records. Anyway, for me, it feels strange to reload an embedded record, because what it requires, in fact, is to reload the "root" parent, with all the other embedded records in it, and it's not even guaranteed that after reloading our initial embedded record is still there. So I guess embedded records are simply not meant to be reloaded on their own. I don't know if it's true though 🙂 |
Art3606
force-pushed
the
shrine-3.0-update
branch
from
526d44e
to
87a1764
Compare
Art3606
force-pushed
the
shrine-3.0-update
branch
from
96966fb
to
27bec0c
Compare
|
Hey @janko, I'm getting back to this one 🙂 I have a question though: can we avoid reloading at all? I'm afraid it might lead to tricky hidden bugs. I guess it's better to deliver less functionality out of the box for Mongoid, leave it for users to take care of some things, and make it clear. |
|
@FunkyloverOne Sure thing, we can just return the same model instance without reloading. I think that's reasonable in this case. |
- Do not perform extra `persist` in after_save callback, so there's no more bugs with saving records with embedded models with shrine attachments. Call `finalize` in before_save callback now. - Support deeply nested records. - Skip reload for non-persisted records. - Add more tests.
|
@FunkyloverOne Thank you, I really appreciate your work on this! Will calling |
|
I believe the background job won't be able to pick it up at all, how could it? And even if it causes any issues with backgrounding (which we do not know yet) - it's better to have no support for it and leave it to users, but have a properly working "base". BTW I plan to test backgrounding in a project I work on tomorrow. So I will let you know what do we have there. I see what you mean here. |
|
@janko how about - we give users a standalone |
|
@FunkyloverOne Just to check, is it not possible for |
|
It depends on what it does. if it changes the record - then we need to save it once again - and that was exactly the problem with embedded records with non-persisted parents. |
|
@janko we could also give an option to have |
|
Yeah, that's an option, too. Ok, I think it's time for me to step away and just let you do any changes you think are right 😉 I already gave you RubyGems access to the gem, so you're free to publish releases with the new changes. I'm very grateful for the work you're putting into improving shrine-mongoid 🙏 |
TODO: add tests, especially for backgrounding.
|
Alright, got it! Hmmm, I've also made this auto-finalization completely disabled by default. That's a breaking change anyway though. And then I believe I should release v2.0.0 (due to that breaking change). |
|
Sounds good to me, thank you 🤘 |
Adds test for #5.
@janko, that bug is not yet fixed, but I believe this might help you figure it out.
And it also fixes a bug with
Attacher#atomic_persistand nested models.