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

Feature Request: Custom syncing and loading of data to and from the model #277

Open
tilod opened this issue Jul 25, 2015 · 24 comments
Open

Comments

@tilod
Copy link

tilod commented Jul 25, 2015

I often deal with form objects where the structure of the HTML form and the submitted params hash don't have much in common with the data structure of the models. But instead of rebuilding the structure of the models in the form object and fiddling in the data from the params hash with populator lambdas, I prefer to represent the structure of the params hash in the form object and override either #sync or #save where I do the data mapping manually. This works well for writing but of course not for loading data from the model(s) into the form.

Is there a method I can override to manually define the way data should be read from the model(s) and written to the form's properties?

Awesome would be an option per property, for instance property :email, load: ->() { ... }, sync: ->(value) { ... }. The :load lambda should be called whenever a property is read from the model after initialization of the form and :sync should be called by #sync. If all properties are defined this way, you actually wouldn't even need a dedicated model in the form at all.

What do you think of the idea?

UPDATE:
I just had a better idea for naming the options: How about naming them :read and :write, in correspondence with the :readable and :writeable options?

@apotonick
Copy link
Member

That's basically the idea of the underlying twin. Reform was written to decouple your models from forms, so you don't need to expose your table structure via forms.

Many use the Composition feature to have several models in one form. https://github.com/apotonick/reform#compositions

In my perfect world, you'd have three objects.

  • The dumb model(s).
  • Twins to indirect the models and the public API you want to expose.
  • The form that only knows about the twin's API, not the model.

Currently, twin and form are one and the same object, but you could simply pass a twin into the form to achieve what I think is the correct way. However, that would imply you having to define form and twin, and most people will think this is "too complicated" even though this is basically what happens inside of Reform anyway.

In the "data" twin, you would specify readers and writers similar to what you suggest. We do the same in Representable and Roar, where you have :setter and :getter options. However, I find this wrong. I also find it wrong in Reform itself because this is a form object and not a data mapper. This is why I extracted all data modelling into Disposable.

Gimme a few days and I will present you with some examples. You can then also reuse your twins for other parts of your application, e.g. in business logic. Good points, loving this discussion already! 🍻

@tilod
Copy link
Author

tilod commented Jul 25, 2015

Ah, so we are actually having the same idea of a perfect world. 😄 I don't use the Composition feature (I also don't know how to do this in Trailblazer as there is always one model that is passed to the contract) but define my own "data mapping objects" to translate between the models and the structure of the params hash (which is defined as properties in the form object). I call these objects "Aggregators". The initializer of an aggregator does all the database calls and presents the data structured the way the form needs it. I use a bunch of delegates to map the readers and writers and add custom readers and writers when needed. And there is a #save method that calls save on all the models. When using an aggregator, I can leave #save in the form untouched. The problem is, doing it that way, gives you a "flat" surface, and no nested properties.

So I just thought, maybe I can get rid of the aggregators and do the mapping directly in the form object. The aggregators are another layer of indirection and I understand that people think it is too complicated for that reason. But if you define this as the preferred way of data mapping in Reform/Trailblazer, I am happy too. But it would be very good to have some guidelines how to achieve this using a Twin. So I'm really looking forward to see the examples you proposed. 👍

Thinking about this, doesn't that mean, to put a Twin (the aggregator) into another Twin (defined by the form)? And when you say, you find it wrong to use the :getter and :setter options of Representable (just skimmed the Readme), how I am supposed to do the actual mapping? Well, I will wait patiently for your examples. It's weekend anyways. 🍻

@JakeBecker
Copy link

+1. My forms don't map cleanly to my models at all, and I could use some examples of how to handle that.

@apotonick
Copy link
Member

Give me some examples of what you want and I'll tell you how to do it, how does that sound?

@JakeBecker
Copy link

Okay, thanks! I have an API that contains a resource for a "spreadsheet" where one of the fields is for the spreadsheet's schema. That field looks something like this:

{ 
    schema: [{
        name: String,
        type: String,
        description: String
    }]
}

Under the hood, however, this field is split up and stored in two separate fields in the model: A serialized JSON array of arrays: [ [name, type] ] and a serialized JSON hash that maps the name to the description: {name: String}

This is probably one of the worst examples in the project I'm working on, but there are lots of places where the API maps only roughly to the underlying model. It's very hard to do database migrations, unfortunately, which makes it very appealing to have a twin that corresponds to the way we present the resource in the API (as opposed to the frequently stupid way things are actually stored in the model).

I am having some trouble figuring out the best way to handle this sort of data mapping, though. The easiest way would just be to have a getter and setter on the model to do the conversion, but I was hoping to be able to abstract that away somehow.

@apotonick
Copy link
Member

Cool example, @JakeBecker!

@JakeBecker
Copy link

My latest attempt looks something like this:

class MyForm < Reform::Form
  property :schema, writeable: false

  def initialize(model)
    super(model)
    self.schema = ...  # Convert from the model's schema fields to the form's unified schema field
  end

  def sync(options={})
    super(options)
    self.model.schema_field_1 = ...
    self.model.schema_field_2 = ...
    return self.model
  end
end

This seems to work, but I'm certain there must be a better way. I've done some more reading about Trailblazer (I bought your book!) and you've mentioned in the docs for Representable that the :reader and :writer option on properties can be used to handle input fields that should be "split" before validation, but I think what I really want is something like a representer to convert between the form and the model, instead of between the input/output and the form.

Looking forward to any advice!

@apotonick
Copy link
Member

I have a cool idea. So my main idea was to push people towards using their model wrappers manually, and Reform shouldn't know anything about how the internals of your models work.

But what if you could specify your twin on the form (and if you want to reuse it elsewhere, you simply extract it to a separate class)?

class AlbumForm < Reform::Form
  twin: YourPoro # that will use YourPoro as a model wrapper 
  # or..
  twin do
    property :title, getter: -> {..} # specify how to read in a lambda.
    property :title, getter: :compute_title # from a method.

    def compute_title
      model.title + model.id
    end
  end
end

This will separate form logic (Reform) and the way you model your domain (Disposable::Twin / your own stuff). Like?

@tilod
Copy link
Author

tilod commented Aug 11, 2015

The idea with the decorating twin looks convenient at first sight, but it limits you to one model per twin. I don't know about

@apotonick
Copy link
Member

No, it does not. The twin can be a Composition or whatever you want, we just have to provide a way to specify how it's instantiated, then you can use the whole power of twins without messing around in the form.

[Update] Example:

class AlbumForm < Reform::Form
  twin do
    include Composition
    property :title, on: :cd

@apotonick
Copy link
Member

I am thinking about providing that in all other gems, too, Operation, Cells, and Representable.

@tilod
Copy link
Author

tilod commented Aug 11, 2015

(Sorry for the partial post. I'm on vacation and typing on the Smartphone isn't fun)

So I don't know about @JakeBecker, but I often need multiple models in the Form/Twin. For me it's either easy 1:1 mapping between form and model or super complex custom setup with multiple models. There nothing much between.

What I also recognized: When specifing a Twin as a mapper, you have to duplicate the 'property' definitions (I would have used backticks, but they are not on my phone keyboard) in both form and twin. The form needs to specifiy the validations and the twin the getters ans setters, but the property names and structures needs to be the same.

@tilod
Copy link
Author

tilod commented Aug 11, 2015

I still agree that the mapping should go into a separate layer. The only thing a form should know about persistence is, that calling '#save' on the model does it. I like this.

But there are some caveats with the twin. One is the duplication of property definitions. But thats not too bad I think. I am also uncertain if getters and setters per property are sufficient. I would like to have a method for reading and writing the whole twin that I can override. For writing there is '#sync', so this is fine. But I cannot find a method for reading. If there are only getters and setters, the logic for reading and writing is spread over 100 lambdas and it would also get difficult if the data has to be read in a certain order (which is awful by itself, but sometimes happens).

@apotonick
Copy link
Member

Thanks @tilod, valid comments!

  1. You can copy properties from whatever to your twin and vice-versa automatically. No need to have redundancy.
  2. #sync is not meant to be overridden, you're talking about the block, right? Well, you theoretically can override #initialize then and do your mapping there. The ::twin block is just a way to separate form and data modelling, I am totally open for discussion how to achieve that here.

@niels
Copy link

niels commented Mar 3, 2016

I believe I just ran into this myself.

We have a model with a number of Array attributes (stored as JSONb in PostgreSQL). This is the data structure we need for all internal use. When it comes to rendering forms to end users however, we usually want to represent the Arrays as newline-separated Strings.

E.g., an array ["just a sample", "array"] would – in a form in the UI – be displayed as follows:

just a sample
array

Of course when writing back to the model, the above would be turned into an Array again by splitting on newlines.

I am new to the trailblazer ecosystem and my first naive implementation was along these lines:

class Create < Trailblazer::Operation
  contract do
    property :allergens, type: Array

    def allergens
      Array(super).join("\n")
    end

    def allergens=(allergens)
      super(allergens.to_s.split(/\r?\n+/).reject(&:blank?))
    end
  end
end

With a simple_form like so:

= simple_form_for(form) do |f|
  = f.input(:allergens, as: :text)

This works great for writing. I.e. I pipe in a newline-separated String and upon calling #sync or #save, that String gets transformed into an Array, with the latter being set on the underlying model.

For reading however, my custom #allergens doesn't seem to be used when rendering the form. Instead of the desired newline-separated String, the form's textarea is filled with ["just a sample", "array"].

Reading up on the matter, I now understand that the custom getter doesn't get invoked when grabbing attributes for populating a form. I found that to be counter-intuitive as I was expecting similar behaviour than when overwriting attribute accessors on ActiveRecord models. Or perhaps I have missed the relevant documentation?

Above, you suggest using an explicit Twin. However, I don't understand how that would solve my problem. An explicit Twin would behave the same way as the one implicitly generated by contract do, would it not? Could you provide a brief example?

I suppose something like the suggested getter and setter options would work. Or perhaps, more conveniently, we could define a convention whereby forms are populated from ATTRIBUTE_for_form, if defined (don't hurt me.. I know you hate magic!). For example, if I defined allergens_for_form in my contract, this would be used to supply the value for my form field.

I would also be happy with a more explicit / manual approach, but thus far have failed to find out exactly how to go about the above. I have trouble understanding why a form seems to interact with the custom-defined setter (#allergens=) method while completely bypassing the custom-defined getter (#allergens). Forgive me if I have missed something obvious.

Last, let me propose my ideal syntax. There would be three ways to achieve the above. All of the below examples would have the same end result.

module ArraySerializer
  def deserialize(raw)
    Array(raw).join("\n")
  end

  def serialize(raw)
    raw.to_s.split(/\r?\n+/).reject(&:blank?)
  end
end

class Modularized < Trailblazer::Operation
  contract do
    property :allergens, type: Array, form_serializer: ArraySerializer
  end
end
class Methods < Trailblazer::Operation
  contract do
    property :allergens, type: Array, form_deserialize: :deserialize_array, form_serialize: :serialize_array

    protected

    def deserialize_array(raw)
      Array(raw).join("\n")
    end

    def serialize_array(raw)
      raw.to_s.split(/\r?\n+/).reject(&:blank?)
    end
  end
end
class Blocks < Trailblazer::Operation
  contract do
    property :allergens, type: Array, form_deserialize: ->(raw) { Array(raw).join("\n") }, :form_serialize: ->(raw) { raw.to_s.split(/\r?\n+/).reject(&:blank?) }
  end
end

[EDIT] For now, I am doing this:

class Model < ApplicationRecord
  def allergens_t
    Array(allergens).join("\n")
  end

  def allergens_t=(allergens)
    self.allergens = allergens.to_s.split(/\r?\n+/).reject(&:blank?)
  end
end
class Create < Trailblazer::Operation
  contract do
    property :allergens_t, type: String
  end
end
= simple_form_for(form) do |f|
  = f.input(:allergens_t, as: :text)

@tilod
Copy link
Author

tilod commented Mar 3, 2016

I'm happy this thread get's resurrected. 😛 I'm still struggling with the problem I described. I'm sure Nick will answer soon, but maybe my 2 cents are helpful too.

Overriding the getters and setters doesn't work because a form doesn't delegate the properties directly to the model but holds the values as it's own attributes and then writes them to the model when #sync is called. So when you override the getter, you define how an attribute is read from the form and not how the form reads the property from the model. Overriding the setter does work because it defines how the attribute gets written to the form – and the form then writes it as-is to the model when #sync is called. However, this can be troublesome too because it can make validation harder (depending on what you want the validate: The "serialized" or "unserialized" params).

There is already a way to override the attribute accessors of the form itself: You can use the :setter and :getter options for this – or do it manually, as you did. But there is no clean way to define how the attributes are read from and written to the model. As I see it, your serializer approach is a more thought-out version of what I proposed in the original posting.

Let's see what Nick thinks about this after half a year and the official Trailblazer release.

@contentfree
Copy link

I'm running into this myself in my initial exposure to Reform…

I have an API where the data structure doesn't map at all to the table / model structure (and I prefer it that way). I'm likely missing something obvious, but it seems that, in opposition to its stated purpose, Reform is very coupled to the models (and therefore, why not just use the models directly…).

I can't seem to find the place to say "OK, you want to save this form data now, here's where you make all of your join / glue objects that you hid from your API" (or vice versa, going from data model to form).

Composition doesn't work here (I believe) because I'm fully leaving out intermediate join models ("has many through" models).

Seems like the ability to provide serialization both ways would be useful. (For now, I think I'm abandoning Reform since I've spent two days on it and just can't shoehorn the API data model through a Reform-driven form... but I'll be watching here, for sure!)

Edit: (I did eventually get this to work by using the block version of #save)

@apotonick
Copy link
Member

Reform is absolutely not designed to work straight on models, @contentfree. The Composition feature in turn has nothing to do with associations, what makes you think so? It's designed to allow you arbitrary containers of objects. And, again, those objects could be domain objects that internally map things to persistent objects. I absolutely do appreciate that you want an API that is not simply reflecting your database, @contentfree! That's what all my gems are about (Roar/Representable, Reform, Cells, ...).

Please don't assume that Reform does everything for you, though. It's a form object that helps to decouple validation from persistent objects. The data mapping is a "free service" that comes from the underlying Disposable gem, which exists to have an intermediate object layer so you can design your forms/APIs the way you want it, and not the way the database layout dictates you.

I think it needs more docs on how to do that, though, but please don't expect Reform to be an all-mighty monster gem!

@contentfree
Copy link

…long delay…

So is there supposed to be an object between the form object and my persistent object? (Remember: this is my first exposure to Reform.) To solve my problem I ended up overriding save and manually tying attributes from the form to the data models involved.

Obviously, that doesn't seem right, but since the structure of the data coming from the form is different from the actual data object model it's what I had to do to get my work done. I worked at it for quite a while, trying to figure out the Right Way. But that's what lead me to express that, from my newb perspective, it seems like the form data has to line up very neatly with the data model for it work (without overriding methods like save in the form object).

After looking at @zavan's referenced issue, maybe I need to add a twin in between the form and the data object? Or maybe you guys have come to a new conclusion that there should be the ability to provide getters and setters?

No doubt improved documentation would help here.

Here's generally what I'm doing and I'm sure it ain't right: https://gist.github.com/contentfree/96485d4d3c3dab099f0247a269721418

@apotonick
Copy link
Member

@contentfree Your snippets do look perfectly fine to me. After having a look at it, I strongly advice you to use Trailblazer, though, as your controller code is the perfect candidate for an operation.

@RKushnir
Copy link

RKushnir commented Jul 5, 2016

@apotonick Did you proceed with your idea of the following?

class AlbumForm < Reform::Form
  twin do
    property :title, getter: -> {..} # specify how to read in a lambda.

I'd like to use a feature like that. Or are there any examples of how to do it in other ways? Thanks.

@fran-worley
Copy link
Collaborator

fran-worley commented Sep 7, 2016

@RKushnir I'm guessing that composition and renaming are too simplistic for you?
http://trailblazer.to/gems/disposable/api.html#renaming
http://trailblazer.to/gems/disposable/api.html#composition

Other options would be to manually provide the getter and then skip sync:
http://trailblazer.to/gems/disposable/api.html#overriding-getter-for-presentation

@RKushnir
Copy link

RKushnir commented Sep 7, 2016

@fran-worley Right. One of the examples where I'd want this is when array attribute in the model becomes a comma-separated string in the form, and then back.

I'm actually doing it the way you suggested, overriding the getter & setter(and from what I see Sync::SkipGetter is included by default in the form class). But I wonder if this solution is good enough.

It doesn't allow me to run validations before I try to transform the value. So I'll get an error if I try to call split(',') on something which is not a string(i.e. array or hash).
Also, not critical, but I'd like to keep the original input, so in case of validation error I could give it back to user saying "last friday" is not a valid date, you fool" :) and the input field would have the value.

@fran-worley
Copy link
Collaborator

@RKushnir can we chat in gitter? I think what you're describing (essentially coercion) is different from the main purpose of this ticket (data mapping).

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

7 participants