Handle updates for nested .has_one save operations

[akadusei]

Resolves #7

This PR seeks to support updating for .has_ones. The previous implementation worked for creates, not for updates. So if you called SomeOperation.update for an operation that called .has_one, you would get an error.

[matthewmcgarvey]

Sorry for the slow reply, I've been thinking about this though. While the operation method to call for these nested operations is "has_one" I'm not sure if it is right to assume that it is necessarily connected to a model's SaveOperation and set foreign keys on it. I view that as a concern of the nested operation to manage that.

This would also allow you to avoid digging into the type of the operation to pull out the model. Though, admittedly, I'm not really clear on what this change is doing

(I just noticed that the old code was setting the foreign key as well, I'd be open to that changing)

[akadusei]

Though, admittedly, I'm not really clear on what this change is doing

This PR seeks to support updating for .has_ones. The previous implementation worked for creates, not for updates. So if you called SomeOperation.update for an operation that calls .has_one, you would get an error.

Sorry for the slow reply, I've been thinking about this though. While the operation method to call for these nested operations is "has_one" I'm not sure if it is right to assume that it is necessarily connected to a model's SaveOperation and set foreign keys on it. I view that as a concern of the nested operation to manage that.

That's OK. My understanding is that .has_one is for automatically saving a model that has an actual has_one association with another model, such that saving one necessitates the saving of the other. (At least, that's how I have always used it).

I'm not sure if this can be expanded to a general use though; It could be worth the try.

[jwoertink]

My understanding is that .has_one is for automatically saving a model that has an actual has_one association

That's correct. The Avram::Operation won't have a has_one method. There's a few things this needs to do to update. Like it should probably raise a compile-time error if the associated SaveOperation is unrelated. But things like that can come later. I had planned on working on it, but work has been crazy lately.

I haven't had a chance to really look over this, but if the idea is to make it work for both creates and updates, then I think that's a good step.

[akadusei]

Like it should probably raise a compile-time error if the associated SaveOperation is unrelated.

A very good point. I'll try to work on that. If it proves cumbersome, I'll just leave for a later PR.

[jwoertink]

I haven't forgot about this, still processing some stuff around it. I just implemented some operations using has_one only for update, and I don't get any errors like how you mention in the description. I do, however, get a different issue. If your operation is successful, but the nested save operation fails during an update, there's nothing that bubbles the errors back up.

class SaveUser < User::SaveOperation
   has_one account : SaveAccount
end

SaveUser.update(user, params) do |op, saved_user|
  # if the `SaveAccount` fails, these are still true
  # op.valid? == true
  # saved_user != nil
end

To get around that, you'd have to also look at op.account.valid? and op.account.errors. Maybe that's fine for 1 association, but if you had let's say 4 of them, then it starts to get super messy.

I'll need to come back to review once I have a free moment to think on this, but my initial thought is that we may not need a whole lot of changes to make all this work better. Thanks!

[akadusei]

# saved_user != nil

This is as expected for .update, which yields a user whether or not operation succeeded. .create on the other hand, may yield a user or nil.

# op.valid? == true

Yup, missed that. I should add that to the specs. Would something like this suffice?:

def save_{{ name }}(saved_record)
  unless {{ name }}.save
    add_error(:nested_{{ name }}, "failed") # <===
    mark_nested_save_operations_as_failed
    database.rollback
  end
end

[matthewmcgarvey]

I think I'm ok with this getting merged if @jwoertink also signs off.

Good job on this @akadusei 👍

[akadusei]

Looking at #276, should we add error to all nested operations if parent operation failed? I'm thinking this:

  #def mark_nested_save_operations_as_failed
    #nested_save_operations.each do |operation|
      #operation.add_error(:parent, "failed") # <<<=== We could use the parent operation name as key
      #operation.as(Avram::MarkAsFailed).mark_as_failed
    #end
  #end

Scratch that. Must be going outta my mind 😄

[akadusei]

Wait! We did not factor in operation needs. If a nested operation has needs, we get a compile error. This is getting out of hand. 🤣

I'll see what I can come up with.

[jwoertink]

This is getting out of hand. 🤣

yup! There's quite a bit to unpack. I think I have some free time tomorrow to take a look at this. I have some ideas on how to do this.

[akadusei]

I think I have some free time tomorrow to take a look at this. I have some ideas on how to do this.

Great! I just pushed a commit, but still interested in your ideas.

[jwoertink]

Thinking about this more, there's a ton of things that need to happen, but I think this PR is already really big. I'd like to start by taking out the addition of accounting for needs, then file an issue about adding that in later. There's a few other things like attribute and until we remove it, file_attribute that also have to be accounted for.

Overall, I think this is doing some good things that will help, but I left a few requests to change. Once those are updated, I'll give it one last look over, and we can start thinking about the next iteration on making this better. Thanks for tackling this!

[jwoertink]

Awesome! Yeah, this update feels a lot better. I know we still have a ton of stuff missing, but doing this in small steps definitely makes it easier to digest. Thanks for handling this and sorry for taking so long. I'll get this merged in, and we can start adding in the issues for what else needs to be done.