Add: bang setters for associations

[jorroll]

This PR adds bang setters for simple (non-ActiveRel) associations which raise an error if the association fails to persist (without an extra DB call). In my testing, creating an association with ActiveRel objects using either .create! or .save! already raises an error if it fails to persist.

person = Person.create
owner = Owner.create

Neo4j::ActiveBase.query("
  MATCH (n:`Person` {uuid: '#{person.id}'})
  DELETE n
")

owner.set_person! person
#=> Neo4j::ActiveNode::HasN::Association::PersistanceError: It appears the relation failed to persist in the database

All the tests are passing, but I haven't added any new tests yet. Implementing this ended up being pretty confusing and touching a lot of stuff, so I wanted to run it by you first. The implementation, while working, feels pretty ugly. Though it could be, in part, a result of how complicated the association logic is.

has_one associations now have:

• object.set_#{association}!

has_many associations now have:

• object.set_#{associations}!
• object.add_#{association}!

[cheerfulstoic]

Are these analogous to association = node and association << node? If so, do those methods not raise errors? Or are these new methods filling out compatibility with methods which ActiveRecord adds?

[jorroll]

1. Yes, they are analogous to association = node and association << node
2. association = node and association << node do not raise errors.

I think it's fine that association = node and association << node don't raise errors. But, I would argue, that they should have a bang equivalent which does raise an error. Hence this PR.

Currently, if you want to ensure that the association was set, you need to make an extra DB query. Usually if the association wasn't set, then nil is returned. However I found this isn't always the case. So if you want to be sure, you need to do an extra check. This PR performs the check as part of the create query and then returns true or false (false triggers an exception) so much more efficient.

[cheerfulstoic]

I'm fine with the methods, though I think I'm not sure in which case errors are / aren't raised. If the database gives an error we'll get an exception. Is this if the associated model doesn't validate? If so, I think this is a great addition

[jorroll]

Currently (sans-PR), calling association << node makes a CREATE db call to persist the relation and that db call has no RETURN.

The bang version of association << node ("add_#{association}!") returns true or false depending on whether the newly created pattern exists().

exists_string = "(#{graph_object.from_node_identifier})-[#{identifier}]->(#{graph_object.to_node_identifier})"
create_query.return("exists(#{exists_string}) as result")

If it returns false, an exception is raised.

def _create_relationship_with_factory(verify=false)
  Array(other_node_or_nodes).each do |other_node|
    wrapper = _rel_wrapper(properties)
    base = _match_query(other_node, wrapper)
    factory = Neo4j::Shared::RelQueryFactory.new(wrapper, wrapper.rel_identifier)
    factory.base_query = base

    if verify
      if factory.query!.response.pluck(:result).first
        true
      else
        raise PersistanceError, "It appears the relation failed to persist in the database"
      end
    else
      factory.query.exec
    end
  end
end

[cheerfulstoic]

Cool, I'm down with this, then :+1:

[jorroll]

@cheerfulstoic Awesome! Just coming back to this. Do you have any opinion on my implementation strategy? I've taken an additive approach with this patch. Rather than refactoring the underlying code, I simply added new args to various methods to make things work. This approach makes backwards compatibility easy (and especially backwards compatibility with the specs), but it feels suboptimal. The underlying logic was already pretty complex, and this definitely increases it.

It kinda feels like a refactoring of the underlying persistence code would be appropriate at some point (though maybe you disagree). Should I flesh out this patch with specs as is (and worry about any possible refactorization as some future project), or should any patch like this attempt to "get things right" the first time?

The large code comment I wrote above (beginning with "I spent a few hours..."), mentions one bit of the implementation I don't like.

No rush on replying if you're busy

[cheerfulstoic]

I'm all for a refactoring for sure if it doesn't change the API and if feel up to tackling it. I definitely think there's some weirdness around how the cache works for associations in how it goes to the DB when it doesn't need to.

If it's not backwards compatible I'm still open to it, I would just lay it out. From what I see so far I don't suspect that there would be a problem.