Refactoring Operations, SaveOperation, and Boxes

[jwoertink]

Right now Boxes are created from SaveOperation, and those are inherited from Operation. For me, I use the base Operation a lot, but they require a ton of work when not associated to a model. For this reason, I started a PR to refactor how they work. I had to put this on hold due to a Crystal bug which was solved in 0.35.0. This means that after this next Lucky release, I can get back on to this.

Before doing that, I've noticed a lot of new issues and PRs come up surrounding these concepts to try and update them. To me, it sounds like maybe we need to take a step back and rethink how we want this whole system to work, rip it out to the studs, and then renovate it to be amazing before we hit 1.0.

luckyframework/avram#403
luckyframework/avram#407
luckyframework/avram#404
luckyframework/avram#334

luckyframework/avram#385
luckyframework/avram#377
luckyframework/avram#376
luckyframework/avram#374
luckyframework/avram#371
luckyframework/avram#331
luckyframework/avram#313
luckyframework/avram#298

These aren't even all of them, just some of the PRs and issues I came across.

Discussion

What I'd like to discuss here is, what are your pain points with SaveOperation / What does a "perfect world scenario" API for saving / updating records look like to you? What thing should this do, and what things shouldn't this do? Is the current API pretty close with a few tweaks for you, or do you just dread when you get to this section of your code?

[wout]

So far, I'm very happy with how operations work. But I'm probably not the right person to ask because I have not yet used operations heavily (but I will in July, though). My main issue was with file uploads, which are mostly related to attributes and params, and we need those improvements before the real work can start. So I planned this week to implement them.

[matthewmcgarvey]

Particularly with save operations, I find associations to be a rough spot right now. Say I am making an app that has strong roles for its users. Just because a user can make, say, a new project, it doesn't mean that the project belongs to them, but they should be added as an admin of the project. To do this process with SaveOperations currently, I have to do something like:

SaveProject.create(params) do |operation, project|
  if project
    SaveAdmin.create(project, current_user) do |operation, admin|
      # success
    end
  end
end

You can put the admin creation inside SaveProject but I generally don't want to put a decision like that at the lowest level of the app and you still have to handle the failure conditions or use the bang methods.

I don't have a good solution for this, or if others would even consider it a problem.

[paulcsmith]

@matthewmcgarvey 100% agree. We do have support for many of these-cases with has_one in operations: https://github.com/luckyframework/avram/blob/daaf55955c8131dea8533584720257ca444f23a7/src/avram/nested_save_operation.cr#L1 https://github.com/luckyframework/avram/blob/daaf55955c8131dea8533584720257ca444f23a7/spec/nested_save_operation_spec.cr

This may actually work in your case. Since you are saving just one admin for the project.

I want to add has_many as well as multi-layer deep operation associations that handle all this stuff for you. It's on the roadmap to Lucky 1.0 so this is definitely a big one we want to get right!

[akadusei]

@paulcsmith Oh, I see. I never really knew about nested operations. It should help me with a few things. Thanks.

[jwoertink]

Just throwing some more info out there. I know @fernandes has mentioned we could take some inspiration from http://trailblazer.to/. For me, I see the Operations similar to the ruby mutations.

I know everyone will have their different opinions on how the code should look and function, but I think as long as the end result doesn't hinder or block you from being able to accomplish your end goal, then we're fine. We're starting to take this approach of "Here's how we think you should do it..." but then "here's how you can escape out if you want to do it different".

If you have an example of how you'd do this in another framework that you really enjoyed, feel free to pop it in here. Maybe it'll spark some ideas on an interface that would be great for Lucky!

[matthewmcgarvey]

With Boxes, associations are an issue as well. I believe that all associations should be preloaded/included by default.

Beyond that, though, I think they need to be more opinionated. Right now they are just SaveOperations and there are ways to do trait style creation and to pass in associations but it is all with standard Crystal. This is nice, but it also means that it's not apparent the best way to use them. I'm thinking of ruby's factory_bot and the clear DSL it has for creating and using factories for generating data.

[oneiros]

If I think hard about it, I am sure I could come up with more, but here a two things I encountered this week:

1. I just left an issue over at the avram repo: Support for (virtual) array attributes avram#408 . It is about support for virtual array attributes. But the broader topic is feature parity between Operation and SaveOperation. In the past, I have worked around this issue with needs, which is something SaveOperation supports, but Operation does not. The promise of "operations" is that you can implement transations that do not map 1:1 to a single row in the database. In practice these little disparities make it hard to actually do so.
2. I am not convinced that validations belong in before_save. You may want (or need) to put other things in before_save as well. In that case a simple call to #valid? could have unintended side effects, which can be surprising and possibly hard to debug.

[paulcsmith]

@oneiros Agreed on both fronts. @jwoertink was working on brining parity between Operation and SaveOperation.

I thought there was an issue for this, but can't find it now, but I agree we probably need a validate that runs before before_save and valid? should just call validate.

Maybe

• before_validate Add this
• validate Add this
• before_save

[jwoertink]

Just dropping this bit luckyframework/avram#413

[oneiros]

Another nice-to-have, not essential at all, but since we are at it 😁

I like being able to have seperate operations for creating and updating records. Sometimes I need this, e.g. when there are attributes that should not be changed once the record has been created. So currently I would have my CreateRecord and UpdateRecord operations which is really nice.

But technically both would still allow to use them for creating and updating. It might be nice to be able to choose if you want only one of those capabilities or both.

[paulcsmith]

I agree! I'd love to be able to do only_update or only_create

[akadusei]

Left a comment on luckyframework/avram#413 before my attention was drawn to this discussion: luckyframework/avram#413 (comment)

Maybe SaveOperation should be split into CreateOperation, UpdateOperation and DeleteOperation, with their respective before_* and after_* callbacks.

They could have a common interface for calling/running them, like SomeOperation.run.

In it's current form, nothing stops anyone from calling UpdateUser.create, instead of UpdateUser.update (or maybe I don't fully understand Operations yet?).

[akadusei]

Just a thought with regards to luckyframework/avram#410:

In save operations, there could be a macro skip_validate_required_for(*attributes), which skips default validate_required for attributes done in #valid?. I assume #required_attributes can be updated to filter out any attribute marked for skipping.

In future, if there are other default validations, such as validating uniqueness of columns with "UNIQUE" constraint, there could be a macro skip_validate_unique_for(*attributes), etc.

This should allow for fine-grained skipping of default validations, without needing to turn off the entire feature.

I find the existing feature useful, as it may prevent inadvertent database errors at runtime. At the same time, I have run into situations where I needed to turn it off for a specific attribute.

For instance, one may not want to tell a user that "password_hash is required" when that attribute is not in the UI. One may, rather, validate password and skip validation for password_hash.

[jwoertink]

Thanks for the input! This makes sense.

[stephendolan]

@jwoertink I know we've discussed this in the past somewhere, but I couldn't seem to track it down...

I think it'd be worthwhile in this change to consider namespacing operations in the generated app and operation generators similarly to how we namespace pages/actions, since they are used quite closely with both. I've found that my top-level operations folder can get a bit cluttered, but I feel a bit like I'm going against the "Lucky way" if I manually refactor the operations directory after lucky init to be namespaced.

[jwoertink]

How would this look? For example, if you had 1 SaveOperation for each model, and then a few base operations, would it look like this?

src/
  operations/
    contacts/
       send_contact_email.cr
    comments/
      save_comment.cr
   mixins/
   passwords/
     request_password_reset.cr
     reset_password.cr
   posts/
      save_post.cr
   users/
      save_user.cr
      sign_in_user.cr
      sign_up_user.cr

Then would you have to call them like Users::SaveUser and Comments::SaveComment? 🤔

[stephendolan]

I'm not sure... I think it might make sense to have something like Users::SaveOperation going to a file like users/save_operation.cr and Users::SignInOperation going to users/sign_in_operation.cr, but that might get overly verbose quickly.

I mainly just like the idea of knowing what I'm writing based on the class name with very little other information. When I go to Users::IndexPage, for example, there's no question that I'm asking for a page object. We can definitely do that by modifying the existing structure on an app-by-app basis, but there could be value to new folks in having it be more explicit.

[jwoertink]

The idea of the operation naming is to be the verb of what it's doing so that way when you look at the filename, you should have a general idea of what it's purpose is. Before we renamed them and called them "forms", we used to always add _form.cr to it, but we decided user_form.cr doesn't really describe it's purpose because is it updating a user, or like in the future we may add delete operations... Also, it got confusing talking about forms and html forms.

Also, keep in mind that SaveUser < User::SaveOperation currently. Then that would become Users::SaveOperation < User::SaveOperation 😬 That might be a lot more confusing to newcomers.

[paulcsmith]

Yeah this is tricky. I do think namespacing is a good idea though, but there I remember there being some edge-cases we had to think about. One of them was what @jwoertink said.

I think something like this would be cool though:

• Users::ResetPassword
• Users::Create
• Users::SignUp

And you'd be able mark them as only_update or only_create and call them with run

Here is the last gist I did on operations https://gist.github.com/paulcsmith/3ce36672c5122b7cc9ca437b74b66e82

[matthewmcgarvey]

I think there's an opportunity to make the Avram::Callbacks module more re-usable. I made an example repo here with a working example implementaion https://github.com/matthewmcgarvey/crystal-callbacks

It would allow us to register callbacks that subclasses can plug in to.

abstract class BaseClassWithCallbacks
  include Avram::Callbacks

  register_callback :before_it
  register_callback :after_it, status : Int32

  def task
    call_before_it # adding the `call_` is the best I could do to get it running
    status = do_it
    call_after_it status
  end
end

class MyClass < BaseClassWithCallbacks
  before_it :do_something
  before_it :do_something_else
  after_it :finish_up

  ...
end

This would immediately be applicable to adding more callbacks than before_save and after_save