Skip to content
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

Hanami 0.9.x compatibility #1

Closed
katafrakt opened this issue Nov 20, 2016 · 12 comments
Closed

Hanami 0.9.x compatibility #1

katafrakt opened this issue Nov 20, 2016 · 12 comments

Comments

@katafrakt
Copy link
Owner

Latest Hanami release, specifically new model engine, broke hanami-shrine heavily. This was expected, but not to that extent. Right now I have run out of ideas on how to proceed.

Problem:

  • Entities in Hanami 0.9 are frozen, which means that I cannot set any instance variables. This is done right after initialization.
  • Shrine relies on instance variables to cache its Attacher class, see here.
  • The attacher is created (usually) on first call of attachment=, but new Hanami::Entity does not call it on initialization. As a result, I got can't modify frozen Entity.

Solution I can think of:

Add some kind of after_initialization callback before freezing entity. This would allow hanami-shrine (and possibly other gems integrating with hanami-model) to do their job before freezing. I know it sounds a bit Railsy.


I create the issue in this repo because I think this is not exactly Hanami problem, but it may happen that it can only be resolved by Hanami team. Of course, any suggestions on how to approach the problem differently are welcome.

@janko
Copy link
Contributor

janko commented Nov 21, 2016

It might be worthwhile to take a look at shrine-rom-example, which includes a plugin for ROM (which I imagine is similar). However, it currently requires the model instance to have an <attachment>_data= writer, and I haven't managed to figure out ways around that, which is likely because I don't have the "immutable functional" mindset (yet).

Anyway, just putting it out there, as I think whichever ideas come up with hanami-shrine, it could also be ported to shrine-rom (which should at some point be released as a gem), and vice versa.

@katafrakt
Copy link
Owner Author

katafrakt commented Nov 21, 2016

@janko-m Thanks for the link! I looked through it, but my problem is more related to the fact that the entity is frozen and does not define setter methods, i.e. you have to pass everything in the constructor and it cannot change. For example, it does not have attachment_data= method, which would allow to change image data on promote.

I played around with prepend idea suggested by @beauby on Gitter. I was able to get a little further, but because of the issue mentioned above (no setter methods) I got stuck again.

That being said, I'm even closer to be certain that "usual" file upload is now impossible to achieve in Hanami. Which is definitely not good.

@jodosha, perhaps you could help us here a bit?

@beauby
Copy link

beauby commented Nov 21, 2016

@katafrakt Can't you have the method on an other object (say ImageAttacher or whatever) and have it call UserRepository.new.update(...)?

@janko
Copy link
Contributor

janko commented Nov 21, 2016

but my problem is more related to the fact that the entity is frozen and does not define setter methods, i.e. you have to pass everything in the constructor and it cannot change. For example, it does not have attachment_data= method, which would allow to change image data on promote.

Yes, neither do the dry-rb models that are used in shrine-rom-example; I added the <attachment>_data writer method manually. That's of course not ideal, but it works, at least for the shrine-rom-example app.

@myabc
Copy link

myabc commented Nov 21, 2016

@katafrakt Your plugin could override Attacher#set, #update and a few other methods, e.g.:

def set(uploaded_file)
  @old = get
  data = convert_to_data(uploaded_file) if uploaded_file
  data = data ? convert_before_write(data) : nil)
  record.class.new(record.to_h.merge(data_attribute => data))
  validate
end

(it would actually be better to override #write - but I'm not sure if private methods equate to private API here)

  def write(value)
    record.class.new(record.to_h.merge(data_attribute => value))
  end

@janko
Copy link
Contributor

janko commented Nov 21, 2016

@myabc That sounds like a great idea!

And good question – in Shrine private methods don't equate to private API. Actually, there aren't really methods that are part of private API, almost every method from base is used or extended in plugins, so they have enough "weight" that it's very unlikely they will be removed. Attacher#write is definitely safe to rely on.

@jodosha
Copy link

jodosha commented Nov 22, 2016

Hi all!

I'm not familiar on how shrine work, but is it fine with you to write the value at the initialization time of the entity?

This should be injected by shrine in the entity that has the attachment

def initialize(attributes = nil)
  # prepare attributes for shrine
  super(attributes)
end

or alternatively to have this in native Hanami::Entity

def update(new_attributes)
  self.class.new(attributes.merge(new_attributes))
end

Is that attribute part of the database columns or is it a virtual attribute?

For instance, given a User entity, backed by a users database table. Are you trying to set avatar attribute, which corresponds to users.avatar in the database? I define this a "concrete attribute".

A "virtual attribute" is an attribute that is not backed by a corresponding database column. Eg. You try to set avatar, but there isn't users.avatar column.

I'm asking because entities now whitelist the attributes received when initialized. This is done by an internal schema. This schema is modeled around the corresponding database table.

If you use concrete attributes, you're all set. If you don't (virtual), we should work together to add an extension point in Hanami::Entity.

/cc @davydovanton

@katafrakt
Copy link
Owner Author

@jodosha Thanks for your comments!

avatar and avatar= methods are virtual, they wrap calls to avatar_attacher, which is an object responsible for persisting, promoting the image etc. The actual database column is avatar_data. Note that avatar_data is actually not passed as argument to entity constructor. This avatar_data is going to be created or updated on save, so it's very different from the idea of having all the data upfront as arguments to entity's constructor.

I like the approach with creating a new entity before save (self.class.new(attributes.merge(new_attributes))). But is it safe thing to do? If user or some other library "hacks" the entity and sets some instance variables before freezing, it will be gone after cloning. So it may not be OK to do it like this.

@nessur
Copy link

nessur commented Dec 6, 2016

upgrading my project to hanami-0.9 this week, I took a 'bad' or 'bull in a china shop' approach:

# hanami entities are frozen. This this makes shrine not work.
class Hanami::Entity
  def freeze
    #dont
  end
end

and

class Image < Hanami::Entity
  include ImageUploader[:image]

  # attributes :name, :model_id, :image_data, :pic_original_url

  def image_data=(image_data)
    @attributes[:image_data] = image_data
  end
end

But I would very much like a better solution. I'm guessing/worried that 'disabling' entity freezing might have some downstream effects, other than snubbing dry-rb software design patterns.

@katafrakt
Copy link
Owner Author

@nessur Thanks for sharing and letting me know that you are using this project!

I'm still working on a proper solution and I think I'm getting closer and closer to the solution. This will require some changes in the API for sure, but (I hope) won't include any hacks like overwriting freeze 😉

@jodosha
Copy link

jodosha commented Dec 6, 2016

Adding @davydovanton and @cllns to this conversation.

@katafrakt
Copy link
Owner Author

I created a pull request for adding support for Hanami 0.9: see #2

I know the code is kind of unclear, but I'd appreciate any kind of code review.

@katafrakt katafrakt changed the title Hanami 9.0.x compatibility Hanami 0.9.x compatibility Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants