New convention: add validations that should *always* run to base 'SaveOperation' in models

[paulcsmith]

The problem

Right now we suggest creating as many operation as you need to customize how models are saved. This works really well for making it clear what every operation does, but it does introduce some points of failure.

If you have validations that should always run in every operation you'd have to extract a mixin and remember to include it in every operation: https://luckyframework.org/guides/database/validating-saving#sharing-common-validations,-callbacks,-etc. this can get annoying and means you may forget to add the mixin in new operations.

Here is an example:

# A mixin for common validation that should always run on users
module DefaultUserValidations
  macro included
    before_save { validate_email_address email }
  end
end

class SaveUser < User::SaveOperation
  include DefaultUserValidations
end

# Then we create another way to create users, maybe through a CSV
class SaveUserFromCSV < User::SaveOperation
  # Oops I forgot the validations mixin!
  # So an email like "not an email" can sneak in and we have bad data :(
end

The proposal

• Generators should reopen the base class and suggest that validations should go there if they should always run.
• Document this in the guides

Example:

class User < BaseModel
  table do
    column email : String
  end

  class SaveOperation
    # Add validations that should *always* run to a 'before_save' 
    # block in this class
    # 
    #     before_save do
    #       validate_size_of age, min: 13
    #     end
    #
    # Validations and callbacks that should only run in some cases 
    # should go in 'SaveUser' or other SaveOperation that inherits from this class
    # https://luckyframework.org/link-to-guides-with-more-info
  end
end

Problems with this proposal

• Split logic between the model file and operation files. May be hard to figure out where a validation is coming from
• Junk drawer. Someone could add everything to the model. Then again people could do this with regular operations too. It is a risk though

Other ideas

• Maybe we just document re-opening the class instead of adding it in the generator?
• Maybe we have some kind of model hook? always_run_before_save {}

[paulcsmith]

cc @jwoertink @edwardloveall

[bdtomlin]

My fear on this is that people just start shoving all their validations in there and lose out on the advantages that operations bring. It would be real easy to do, especially coming from Rails.

When we start with individual operations it seems to help facilitate learning of a new and better way to handle validations (through operations).

[jwoertink]

I can see how this could be useful, though, for me personally, I've never really been a fan of defining a class inside of a class. I do see @bdtomlin point though. Does this end up being a "junk drawer" for people? And would you use this for anything other than your default validations. Plus, I guess if you're looking to debug a specific operation, and you open up that code, it might not be apparent right away that it has an extra 3 or 4 validations.

Now, as for the plus side, this way doesn't force you in to any sort of config that locks you in. It's not like an option that forces you to always do this. You would still be able to write your validations in separate operations if you'd like.

[edwardloveall]

Good points here. I think I'm in agreement with @jwoertink that it's hard to find when it's inside the model. It feels like the problem is discovery vs remembering. Discovery, because if your model is giving you an error you didn't expect from your model you have to figure out where it came from. Remembering, because if you meant to ensure something before your model saved and you forget to do it, now you have bad data.

If the problem is that simple (it probably isn't) then I'd take something being hard to find vs accidentally having bad data.

[edwardloveall]

It may also be helpful to have a few examples to talk around when trying to solution here. Did this request come from something specific?

[paulcsmith]

@jwoertink @bdtomlin I'm totally with you on both monkey patching a class inside another class and on it potentially being a junk drawer. That's one of the main reasons operations like this were added to Lucky, was to avoid a junk drawer and spaghetti code of conditional logic.

On the other hand I think we need to have some kind of convention for validations that should always run. I'm not 100% sold on the proposed approach so I am open to other ideas! Maybe it just means documenting this idea, or maybe we have some kind of "required_validations" hook or something?? I'm not sure. I'll add some ideas if I have them and am open to more ideas from y'all if you have some.

@edwardloveall I think you nailed it

Discovery, because if your model is giving you an error you didn't expect from your model you have to figure out where it came from. Remembering, because if you meant to ensure something before your model saved and you forget to do it, now you have bad data.

The idea came from validations that always need to run and my anxiety around accidentally letting bad data in because I forget to include a validation somewhere. Here is an example:

# A mixing for common validation that should always run
module EmailValidation
  macro included
    before_save { validate_email_address email }
  end
end

class SaveUser < User::SaveOperation
  include EmailValidation
end

# Then we create another way to create users, maybe through a CSV
class SaveUserFromCSV < User::SaveOperation
  # Oops I forgot email validation so an email like "not an email" can sneak in and we have bad data
end

This also applies to stuff like validating emails, usernames, etc. You don't want to accidentally forget a username validation somewhere and let someone use "Paul @ Smith" and have bad data. So you either have to be super vigilant about remembering to add these validation mixins, or my preference) have some kind of convention for putting validations that should always run.

[jwoertink]

I think that example really helps to clarify. I've done that same thing too. You expose multiple interfaces because sometimes you just can't avoid that. Then you forget, and your db is full of junk.

I wonder if there's a way to hook in to Crystal's abstract for this... 🤔 Like, it would be kind of cool if there was a thing you added in the model as saying "The business logic for this thing always requires email". Then when you create your custom operation and inherit from the SaveOperation, there's an abstract type call that forces you to implement that call.

I guess in this case ,you'd still end up with a class in your class so you can have class 🎩

This might be a tough one. It might be that to gain the benefits of safety, you sacrifice a little code "prettiness" (or whatever you'd call it).

[bdtomlin]

Just an idea, what if we had some sort of BaseUserOperation?

class BaseUserOperation < User::SaveOperation
  before_save do
    # Validations
  end
end

class SaveUser < BaseUserOperation
end

[paulcsmith]

@bdtomlin I think that is similar to what we're doing in the proposal by extending User::SaveOperation. However I am probably missing some advantages of your proposal. Could you explain what you like better about that the BaseUserOperation approach?

[paulcsmith]

@jwoertink Interesting idea! Basically some way of saying "Hey you always need to include some module/method in every operation" So it is discoverable, but you also get an error if you forget. Did I understand that correctly?

[jwoertink]

Yeah, exactly. So if you inherit from that SaveOperation, and don't include your validation, you get a compile error saying "you forgot this validation you told me you always need". Then the validations are always in your save ops inside the operations folder, but you're less likely to forget one.

I have no clue how that would be implemented, but to me, it feels very Lucky-ish to yell at me when I forget stuff 😂

[bdtomlin]

@paulcsmith The minor advantages were not embedding it in the model and keeping the idea that operations are something separate. It could be further enforced by making the BaseUserOperation class abstract.

I had not seen @jwoertink's idea while typing mine. That seems more interesting. What if had something like this:

module UserBaseOperation
end

class SaveUser <  User::SaveOperation
  include UserBaseOperation
end

And then somehow raise an error if UserBaseOperation isn't included? That would give discoverability as well as a clean model.

[paulcsmith]

@bdtomlin Yeah I like that idea too, and I think it is possible to do. Just need to figure it out how it might work and be understandable/intuitive. Also don't want to make it too complex code-wise. But I like the idea a lot. We can also show people the monkey-patching way because sometimes people just want to do things the easy way, but we can recommend this cool "required mixin" approach since you get much better discoverability!

[edwardloveall]

Here's a totally different idea that sounds hard to do: attribute validations instead of model validations.

Instead of validating the model each time in different ways, what if instead we validated a group of attributes? The UserEmailValidator ensures that emails always have a string, an @ character and something with a dot after the @, for a simple example. Or the InvitationUserValidator ensures that when inviting a user to a group, that user exists. An InvitationGroupValidator would ensure that the group exists.

[edwardloveall]

This is a great discussion BTW. Thanks @bdtomlin @jwoertink and @paulcsmith

[paulcsmith]

@edwardloveall I think I get your idea. I think Rails has something similar that works well. My only concern there is that it adds another concept people would have to learn, but I think you're liking this idea of compile errors so maybe that's the way forward

Update: Also another approach to use is custom types where the validation lies in the parsing. So you could have an Email type for example. But that can be overkill for a lot of things. Hopefully we can make that easier

[jwoertink]

I can get behind the User::DefaultValidations. We just need to make sure that we have a way to specify if a validation is default or not. Like, I may have an attribute that is nil when the record is created, but when I update the record, it has to have a value at that point. Like when upgrading a user account from a "Guest" to a "Full" or something.

[paulcsmith]

I think in those cases it would not fall under a "default validations". Those should be run every time no matter what. If you have one for update and one for create then I think those would go in the operations that handle that. Does that make sense?

[paulcsmith]

I should include what I think the module would like and how it could be used:

module User::DefaultValidations
  macro included
    before_save do
      validate_size_of age, min: 13
      validate_phone_format phone
      validate_email_format email
    end
  end
end

class CreateUser < User::SaveOperation
  include User::DefaultValidations
  # Don't validate account type. Just run default validations only
end

class UpdateUser < User::SaveOperation
  include User::DefaultValidations

  before_save do
    validate_inclusion_of account_type, in: [:guest, :full]
  end
end

[bdtomlin]

I always use generators for models since it generates the migration as well.

This also brings up something I've been wondering about code organization. Should operations be grouped in folders like the following by default?

src/operations/user/default_validations.cr
src/operations/user/save_user.cr
src/operations/user/sign_up_user.cr

I think I'm probably going to start organizing this way. With the current structure I find it difficult to see at a glance all of the operations for a model. I think this grouping would make it easier.

I think it would also be nice to have your default validations right there by your operations.

There would be additional benefit if you are using a fuzzy file finder.

[edwardloveall]

Good question @bdtomlin. I think it's fine to organize that way if you're feeling pain from having them all loose in the same folder. I think though that I'd want the current way as the default still. Most apps have few models with few operations to start so I think it be over-designed to have them all separated out.

[bdtomlin]

@edwardloveall that is a fair point. I think it is still worth considering in the context of this issue since it would group default validations with the operations that use them. I'm currently unclear where these files would end up going, which is the reason for my comment.

It seems like these are the 4 most obvious options:

1. Top level in the operations folder mixed in with all the operations.
2. Top level in the model folder mixed in with all the models.
3. Nested in the model folder along side the model file src/models/user/default_validations and src/models/user/user.cr (yuck)
4. Nested in the operations folder along side operations:

src/operations/user/default_validations.cr
src/operations/user/save_user.cr
src/operations/user/sign_up_user.cr

Since the operations are the most likely to have multiple files already it seems like the best way. Maybe there are other better ideas that I haven't thought of though, or maybe there's a preference for the flatter approach?

[paulcsmith]

@bdtomlin I agree that this is tricky and is worth figuring out. My initial thought was this src/operations/mixins/user/default_validations. I think this is a good first pass, but I also think you've got some good points. I just wonder if this should be a suggestion or if we can figure out a way to simplify the folder structure somehow. Basically I just don't want to type User::SaveUser all the time. And it reads a bit strangely. But at the same time...I like that you can group them all under User....naming things is hard!

For now

I think we could move forward with this in src/operations/mixins/user/default_validations

Later

Figure out a way to address the issues you mentioned around discoverability

We can keep discussing this here, but don't want anyone to think this has to block the work.

Here are some thoughts I have:

• Change SaveUser -> User::Save
  • Other examples: User::Create, User::Update, User::SaveFromCSV, User::SignUp
• Keep mixins in src/operations/mixins/user/* so you can also have shared mixins and so they are required before requiring operations
• This would however require changing action naming since they are now way too similar. I've been thinking about this for awhile though and maybe it'd be a good idea...
  • Users::CreateAction, Users::CreateRoute, or Users::CreateEndpoint?
  • Or namespace them by type: Actions::Users::Create and same with pages: Pages::Users::IndexPage but I don't know. The tricky part is these all affect the other naming conventions so it is tricky to make name changes since it requires rethinking a bunch of other stuff

Honestly this is probably much bigger discussion, but wanted to get it out there! Thanks for starting it @bdtomlin

[bdtomlin]

@paulcsmith I wasn't planning on putting this here, but I've been thinking about organization for a while and I think there are a few problems currently with lucky's organization that lead to collisions and issues with discoverability.

Obviously you can choose to organize this way on your own if you want, but I think standardizing it could be beneficial for new devs as well as larger apps down the road.

Operations:
Operations::User::Create, Operations::User::Update, Operations::User::SaveFromCSV, Operations::User::SignUp

Actions:
Actions::User::Create, Actions::User::Update

Pages:
Pages::User::Index, Pages::User::Show

Queries:
Queries::User, Queries::Customer

Serializers:
Serializers::User, Serializers::Customer

Here are some benefits:

• Really easy discoverability. New users can generate scaffolding and see Operations::User::Save in the code and know exactly where to go. Consider currently when a new dev sees SaveUser.new, where do they go, how do they even know it's an operation? I definitely struggled with some of this initially.
• Grouping becomes easy. User operation mixins go in operations/user/mixins or something like operations/user/default_validation_mixin.cr. Mixins for all operations go in the root operations folder or operations/mixins
• Fixes name collision problems.

And the drawbacks:

• Changes to the current structure and generators
• UserQuery reads better than Queries::User, but I bet new developers can always find the latter.
• A little more typing.

[paulcsmith]

@bdtomlin First off, thank you! This is the kind of thing I definitely want to nail before 1.0! I agree on a lot of the "pros" you mentioned here, but I think there are a few more drawbacks:

• You can no longer use Crystal's constant lookup for concise page/link rendering

class Actions::User::Index < BrowserAction
  post "/users" do
    # Can't do this anymore
    render IndexPage
    # need to do this
    render Pages::User::Index
  end
end

This falls in to your "more typing" but I thought I'd give an example. Though after looking at it, it's not that bad lol

• Harder to work with in some editors.
  • index.cr could be for an action or a page. user.cr could be for a model or a query
  • Maybe not a big deal since a lot of editors show the folder it is in index.cr - user but that still isn't clear it is a page unless you show the full relative path in the tab :S

Overall

I like the approach and I like using singular for everything (except for the actual paths in actions). Discoverability is awesome.

Tiny bit more typing, but not a lot. Biggest problem I see is the tab/file issue that will make it hard to jump around to files since there are a lot of similar named files. This is the major reason I decided to add the type of the object at the end of the file.

We could do something like:

• Pages::User::IndexPage and Actions::User::IndexAction but then you've got even more typing and it is repetitive in the code :S

I'd really love to come up with something where you can figure out what the files are without showing the whole relative path in the editor, but keeping the discoverability. I'm sure we can come up with something, but it is tricky to really nail it!

Also Operations is super long. Almost makes me want to rename those lol

[bdtomlin]

@paulcsmith, thanks for pointing out the editor issue, that is not something I had considered.

Also, I think the explicit Pages::User::Index instead of just Index is actually an improvement for discoverability that is worth the tradeoff of typing.

[paulcsmith]

@bdtomlin Agree on the pages part. But not so sure if it is Pages::User::IndexPage. Maybe that is ok, but my main worry is people looking at these cod samples and thinking it is all boilerplate. It is mostly aesthetic but that matters to some people. Might be fine, but worth keeping in mind

[paulcsmith]

Here's another quick thought I had:

• User::SaveOperation -> src/operations/user/save_operation.cr
  • Note we would have to rename the base operation since the default one right now is User::SaveOperation. Maybe User::BaseSaveOperation to match User::BaseQuery?)
• User::IndexAction -> src/actions/user/index_action.cr
• User::IndexPage -> src/pages/user/index_page.cr
• User::Query -> src/queries/user/query.cr
  • Opens up adding other queries for a model. Like User::WithoutDefaultsQuery so User::Query can have some ordering or filtering out soft deleted records and User::WithoutDefaultsQuery includes them all

Doesn't start with the type of object, but it is consistent, singular, and you'd have a better idea of what you're looking at tabs in the editor. I kind of like it!

Other examples:

• Admin::User::CreateAction
• User::SaveFromCsvOperation
• link "Users", to: User::IndexAction
• render User::IndexPage
• form_for User::CreateAction
• User::SaveOperation.create!(params)

I like it and I think it addresses most of the concerns. Thoughts? cc @jwoertink @edwardloveall @bdtomlin

[bdtomlin]

@paulcsmith I definitely don't like Pages::User::IndexPage either.

I think your most recent idea is an improvement. One other option is to name the file but keep the name spacing simpler:

operations/user/save_operation.cr -> Operations::User::Save
pages/user/show_page.cr -> Pages::User::Show
actions/users/show_action.cr -> Actions::Users::Show

That way your editor shows a more descriptive file name, but your code is still cleanly name spaced and discoverable.

[stephendolan]

@jwoertink It seems like this is going to be handled in the Operations refactor you're working on (luckyframework/avram#369), right?

Perhaps you can provide this discussion with a short summary of the direction we're taking and mark that as an answer? Or, if you still need feedback, maybe we close this one and open new threads with some specific items now that we've had a broader discussion?

[jwoertink]

I'll have to read back through the thread when I have a little more time, but yeah, the refactor should fix this.

So basically, we will be able to do this:

module User::DefaultValidations
  macro included
    before_save do
      validate_size_of age, min: 13
      validate_phone_format phone
      validate_email_format email
    end
  end

  private def guest?
    record.try &.guest?
  end
end

class CreateUser < User::SaveOperation
  include User::DefaultValidations
  # Don't validate account type. Just run default validations only
end

class UpdateUser < User::SaveOperation
  include User::DefaultValidations

  before_save do
    # only validate the account_type when the user is a guest
    validate_inclusion_of account_type, if: :guest?
  end
end

[paulcsmith]

I'm not totally sure this is handled by the refactor. Part of the proposal here was to make it so CreateUser and UpdateUser will fail to compile if you forget to include User::DefaultValidations. I think that's a key part since the goal of the defaults is that they are always applied in all operations.

I'm still thinking we might want to do something like I proposed in this gist https://gist.github.com/paulcsmith/3ce36672c5122b7cc9ca437b74b66e82#file-new_operation-cr-L3. I think maybe we overthought this problem and putting it in the model is fine. Once you know it is there you know it forever and will remember it.

I'm not sure though :S What do y'all think?