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

Initial implemention of { <relationship>: { data: null } } nullification #306

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Samsinite
Copy link
Contributor

This commit updates Graphiti to be able to nullify relations only if
null is specified in the association payload. This will still need
feedback from the library maintainers, and after approval of design
tests will need written to test this feature.

Created as a draft, just looking for feedback on the design before starting to add test coverage around this feature. Let me know what you like/don't like so I can adjust as needed. Thanks!

…ation

This commit updates graphiti to be able to nullify relations only if
`null` is specified in the assocation payload. This will still need
feedback from the library maintainers, and after approval of design
tests will need written to test this feature.
@richmolj
Copy link
Contributor

Thanks so much for the work on this @Samsinite and sorry for the delay.

I think this looks overall good. My main thought is maybe for belongs_to we just modify the payload so it's as if the user passed method: 'disassociate' instead of relationship: { data: null }. That way we can follow the same codepath and nothing special there.

I got a little bit lost with should_yield_method_type? and how the relationship actually gets nullified - which speaks to the fact that I haven't touched this myself in a long while, not anything with this code specifically. I was expecting to see an adapter method to handle the "mass-nullification" but doesn't seem to be there?

I think it might be a good step to A) start with belongs_to as noted above, test, merge B) then has_many, test, merge C) then polymorphism, test, merge. Might make it a bit easier to grok and progresses easy > hard.

Again, thanks so much for this ❤️ Great to see!

@Samsinite
Copy link
Contributor Author

Samsinite commented Dec 28, 2020

@richmolj sorry just got back from vacation for the holidays.

I originally looked at using the disassociate method, the problem with setting disassociate in the deserializer, but it doesn't have the ability to know much about its payload, and since it is null there is no type information inside of the actual payload. Unless more dependencies are injected into it, right now it is kind of "dumb", just knows what the general format is, and is unable to look up type information from the relationship name so it cannot attach relevant meta-data type info for persistence to use.

The persistence code could be updated so that disassociate could handle an unknown relationship type, and resolve it at that point in time. It seemed easier though to add a separate method that meant that it is disassociating by relationship name without knowing the type, instead of adding extra conditional branch logic in payload_for, but I'll take a look into that direction again, see how it looks, and reach out if anything starts looking funky to get your opinion.

The belongs_to and has_many cases definitely work as of right now, as I tested these via integration tests in my own code base, but I still need to test has_one and polymorphic relationships as I'm not confident they work right now.

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.

None yet

2 participants