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

Overriding #create. #349

Closed
beauby opened this Issue Nov 22, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@beauby
Copy link
Contributor

beauby commented Nov 22, 2016

The way Hanami::Repository defines the standard query methods on subclasses seems not to be compatible with overriding those (super fails).

class UserRepository < Hanami::Repository
  def create(user)
    user[:encrypted_password] =
      BCrypt::Password.create(user.delete(:password))
    super(user)
  end
end
     # NoMethodError:
     #   super: no superclass method `create' for #<UserRepository:0x007f9196193f78>
@cllns

This comment has been minimized.

Copy link
Member

cllns commented Nov 22, 2016

Hm, this is very similar to a before_create callback.

Instead, how do you feel about writing an Interactor that creates a User and encrypts their password?

@beauby

This comment has been minimized.

Copy link
Contributor

beauby commented Nov 22, 2016

@cllns I can't say that I've seen any mention of Interactors in the guides – do you have a link?

Regardless, I'm curious why the default commands are implemented the way they are (i.e. by dynamically prepending a module during class inheritance, rather than simply defining them in the parent class – my guess is that it either has to do with the fact that Hanami::Repository was initially intended to be a mixin rather than a parent class, or with something ROM does related to those default commands).

The current setup leads to the following inheritance chain, for which I see no good reason.

> UserRepository.ancestors
 => [Hanami::Repository::Commands, UserRepository, Hanami::Utils::ClassAttribute, Hanami::Repository, ROM::Repository::Root, ROM::Repository, Object, Hanami::Commands::Console::CodeReloading, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject] 
@cllns

This comment has been minimized.

Copy link
Member

cllns commented Nov 22, 2016

There's no guide for Interactors yet but you can see the docs here: http://www.rubydoc.info/gems/hanami-utils/Hanami/Interactor

You don't have to use Hanami::Interactor, any 'service' object will do.

@beauby

This comment has been minimized.

Copy link
Contributor

beauby commented Nov 22, 2016

@cllns Thanks for the reading material 👍.

After a bit of investigation, the commands are created dynamically by the call to commands, so the definition of the Hanami::Repository overrides must happen after that. I understand why the prepend would prevent overriding the overrides that Hanami::Repository does, but it's not clear to me why in the above example super does not refer to the ROM::Repository dynamically created method create.

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Nov 22, 2016

I wouldn't recommend overriding core methods like create and introduce your own APIs that use create. Regardless though, this is a violation of POLS so we gotta fix that in rom-repository (I was actually aware of that, just didn't have time to make it better).

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Nov 30, 2016

Also, Hanami::Repository adds another #create on top of ROM::Repository, by prepending a mixin. Hence your problem.

I agree with @solnic. POLS violation here, better to write an explicit method for it. I see general consensus with @solnic comment, so I'm closing this.

@jodosha jodosha closed this Nov 30, 2016

@jodosha jodosha self-assigned this Nov 30, 2016

@jodosha jodosha added the wontfix label Nov 30, 2016

@beauby

This comment has been minimized.

Copy link
Contributor

beauby commented Nov 30, 2016

@jodosha I don't understand your sentence "POLS violation here, better to write an explicit method for it.". Do you mean "The behavior of the framework is violating the principle of least surprise, so use a different name."? I would see how using a different name would be a temporary fix, but expecting people never to override inherited methods does not seem appropriate.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Dec 23, 2016

@beauby Because #create is a public method exposed by default by a repository. Its role is to persist a single object into the database.

If we override #create to enhance it, this situation may "surprise" other devs in a project.

@beauby

This comment has been minimized.

Copy link
Contributor

beauby commented Dec 24, 2016

@jodosha I understand what you mean, and I agree that overriding #create is probably a bad practice. However, exposing a method that is not overridable is the actual surprising thing here, especially as one of the stated goals of hanami is to be less magical than rails and make use of POROs.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Dec 25, 2016

@beauby I agree on the expectations about #create.

If you can, would you like to solve this problem? Thanks.

@beauby

This comment has been minimized.

Copy link
Contributor

beauby commented Dec 25, 2016

@jodosha Sure, I'll look into it – just wanted to make sure you were on board.

@jc00ke

This comment has been minimized.

Copy link
Contributor

jc00ke commented Aug 15, 2018

I've recently run into this issue and I think that @beauby is correct, in that in an OO language, not being able to override a public method violates POLS.

I'm working on an application where I need to hash and encrypt attributes in several models. Currently, I have to write a CreateX and UpdateX interactor for each model X instead of overriding XRepository#create and XRepository#update. In fact, having XRepository#create exposed "just because" means that I now have 2 ways to create an X, and in my case that's actually dangerous. Under no circumstances should someone be able to #create w/o hashing and encrypting.

@jodosha is there a technical reason because we both subclass and prepend? Is there another place we can talk about supporting this? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment