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

Dry::Types::Struct constructor might be an anti-pattern #72

Closed
backus opened this Issue Apr 6, 2016 · 20 comments

Comments

Projects
None yet
5 participants
@backus
Collaborator

backus commented Apr 6, 2016

Right now Structs are "strict" by default. They error if you omit a key but they don't error if you supply extra keys. Not exactly my definition of "strict" but close enough.

If I want to specify that a field has a default value though I still have to provide key => nil for that field if I want to use the strict type. If I want the default to be applied when the key is omitted then I have to specify constructor_type(:schema). Now I can omit all keys though! Oh boy.

Out of curiosity I played with the three options to see how they behave in different scenarios:

Description :strict :schema :symbolized
Omitting key for default value StructError Sets default value Sets default value
Including extra junk keys Value silently ignored Value silently ignored Value silently ignored
Providing nil for field with default value Sets default value Sets default value Sets default value
Providing nil for field without default StructError ConstraintError ConstraintError
Providing invalid type for coercible field StructError TypeError TypeError
Initializing with empty hash StructError values without defaults set to nil values without defaults set to nil
Providing valid input but it string keys StructError all values set to nil all values set properly

So should we add a few more constructor_types? I don't think so. A wise man once said

a swiss army knife won’t ever replace a set of proper tools dedicated for specific tasks. Coercion logic varies, it depends on the context, in a web application there are different coercion rules than in a context of loading data from, let’s say, a relational databases. We can’t just dump this logic into one bucket.

I think that is good advice for this situation. The keys in a hash are just as important and potentially complex in terms of coercion rules as the values.


I think a constructor for a Struct should be configurable in the following ways:

I should be able to specify...

  1. how to handle missing keys
  2. how to handle unexpected keys
  3. that a single key should be coerced from a string to a symbol (or vice versa)
  4. that all keys should be coerced from a string to a symbol

I should be able to specify these rules for part of a hash or a whole hash.

Thoughts? I have ideas for the implementation but this is already a long post

@solnic

This comment has been minimized.

Member

solnic commented Apr 6, 2016

Thank you, this is a great summary, esp that table with constructor behaviors. I don't have the brain power to suggest what should be done exactly, all I can do now is to tell you why we have 3 types of hash constructors and why they do what they do.

  • :strict - this was the first type I created, use case: loading data from the database and instantiating struct objects. I care about having all keys/values in place already, I may not care about additional keys that were relevant to do the low-level mapping, but they are not needed in the "domain" land. If we wanted to drop these extra keys in the low-level mapping layer, we would've added more complexity to the system.
  • :symbolized - a non-strict schema where keys are being symbolized, use case: handling input from http requests, we want to sanitize it and potentially coerce some values
  • :schema - same as :symbolized but w/o key type coercion

Now, seeing the differences in behavior between :symbolized and :schema is obviously a bug from my point of view.

Regarding configuring behavior of struct constructor in a fine-grained manner, I don't like this. Structs should be very simple, they should not be used as your data mapping/coercion/sanitization layer. My intention was to provide a simple API to define domain data types. Struct API is not a virtus' replacement, that was never my intention.

I need to understand what use cases people have in order to provide better feedback.

ps. that "wise man" quote...is this what I said? it rings a bell heh

@AMHOL

This comment has been minimized.

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 6, 2016

When I started to use dry-types I was a little bit surprised with missing key errors and that's why I still use :schema as a constructor. But after some time I realized that it is not necessary and now I use mappers and dry-validation to build proper constructor input. I'm fine with it. I also reworked my specs so now they always provide all keys to build a model. But I think it is very common case that should be covered in the docs.

@solnic

This comment has been minimized.

Member

solnic commented Apr 6, 2016

As I said, this is really, really needed for db queries. Otherwise you may get nils without noticing it early enough. The common case is that it is needed in specs, as you wrote. For that I wrote a 30LOC extension which builds up a whole attr hash for test objects. I'm gonna turn it into an official extension soon, although any help with that would be highly appreciated.

@backus

This comment has been minimized.

Collaborator

backus commented Apr 6, 2016

I can appreciate that you don't want key coercion to be in scope of dry-rb. I think then there should be three types of structs:

  1. Forbids omitting keys without defaults, forbids additional keys, sets defaults when keys for attributes with default values are omitted
  2. Forbids omitting keys without defaults but allows additional keys which are ignored (I believe this is what @solnic says is necessary for)
  3. A constructor which lets you both omit keys and provide additional keys. Missed keys become nil (and the type for that attribute can throw an error if this is an invalid value) and additional keys are ignored.

I think these structs should be introduced and constructor_type should be trashed. I think this makes a lot of sense because it approximately fits three of the kinds of type behavior you get out of the box with dry-types:

  1. Dry::Types::Strict::* types require types to be exactly correct. Forbidding additional keys AND omitting keys matches Dry::Types::Strict::* behavior
  2. Dry::Types::Coercible::* types try to do some basic transformations. I understand that, in this case, there isn't a Kernel method which reduces a hash down to a subset but this still feels similar in spirit.
  3. Dry::Types::* types are just definitions. They don't coerce or perform checks. Likewise, a struct which allows keys to be omitted and allows extraneous keys seems to match this idea of being "just a definition"
@solnic

This comment has been minimized.

Member

solnic commented Apr 7, 2016

When people started asking about different behavior for struct constructors, my immediate thought was to introduce more struct types, but I couldn't figure out good names for them, so I just did the constructor_type "trick". Whatever we do, it should still be backed by hash constructors, it's lower-level but it is useful in different contexts too.

I really hate this macro FWIW, so I'd be OK with adding more struct classes.

I should also mention that performance is a concern here, one of the important aspects is to make it work good & fast with ROM relations, so the original behavior of struct with strict constructor should be preserved (and I don't care if it remains Struct or becomes Struct::SomethingNew).

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 7, 2016

@solnic how do you see a mechanism of official extensions?

@backus I'm not sure if 1st proposed constructor should use a default value if a key is omitted or if an input value is nil. The last seems to me more strict.

@solnic

This comment has been minimized.

Member

solnic commented Apr 7, 2016

@flash-gordon I don't consider the lib to be stable enough to provide a public API for official extensions. I don't even know what kind of extensions people may want to build. Do you have some ideas already?

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 7, 2016

@solnic not yet. I had some issues at the very beginning and resolved them with a number of temporary patches. After some time with new dry-t releases I removed all of them so I don't have any suggestions at the moment. That's why I ask about your vision :)

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 7, 2016

Just remembered that I actually have one extension that I add to all my < Struct classes via mixin:

  def with(attributes)
    self.class.new(to_h.merge(attributes))
  end

Mixin is quite appropriate as an extension IMO :) Maybe that #with should be added to the lib so people would add writers to their classes less often. You know, they won't just stop ask how add writers automatically in the near future :)

@backus

This comment has been minimized.

Collaborator

backus commented Apr 8, 2016

@solnic before you take your OSS break then can we agree on an actionable set of struct classes which I can PR while your gone? My proposal would be

  1. Dry::Types::Strict::Struct - rejects extraneous keys as well as omitted keys unless the omitted key corresponds to an attribute with a default value
  2. Dry::Types::Struct - sets omitted keys to nil and ignores extraneous keys
  3. Dry::Types::Schema < Dry::Types::Struct - allows extraneous keys but fails if keys are omitted unless the omitted key corresponds to an attribute with a default value
@solnic

This comment has been minimized.

Member

solnic commented Apr 9, 2016

It doesn't sound right in Dry::Types::Struct to set omitted keys to nil. What would be the use case for that?

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 9, 2016

@backus about Dry::Types::Strict::Struct. Do omitting a key and passing nil will behave equally for default values?

@backus

This comment has been minimized.

Collaborator

backus commented Apr 11, 2016

It doesn't sound right in Dry::Types::Struct to set omitted keys to nil. What would be the use case for that?

I figured you might just want an object with that flexibility. I personally don't want that. If you don't see a use for it then strike that from the list.

@backus about Dry::Types::Strict::Struct. Do omitting a key and passing nil will behave equally for default values?

In an ideal world I would prefer that a default value can be specified only by omitting the key. nil is a value that people attach meaning to so I think that assuming nil can be substituted for a default value is an opinion that dry-types shouldn't encode.

@solnic

This comment has been minimized.

Member

solnic commented Apr 12, 2016

In an ideal world I would prefer that a default value can be specified only by omitting the key. nil is a value that people attach meaning to so I think that assuming nil can be substituted for a default value is an opinion that dry-types shouldn't encode.

That's not entirely true. If we're talking about structs that are being built from data that is, let's say, persisted in your database, then you are totally right. However, when you're dealing with input coming from a form, then nil has a different meaning. It could mean that we really do want a nil or that we should use some default. That's why we have different hash constructors with specialized behavior that is designed to work well in such cases.

@flash-gordon

This comment has been minimized.

Member

flash-gordon commented Apr 12, 2016

Moreover defaults return provided default value when you pass nil to them. Making behavior different when you pass nil as a value of input hash would be quite confusing (you will have to raise an error in such case).

@backus

This comment has been minimized.

Collaborator

backus commented Apr 12, 2016

Alright. Given that we have different preferences on what should be interpreted as default values @solnic could you propose the Struct types you would like to see?

@solnic

This comment has been minimized.

Member

solnic commented Apr 13, 2016

I just want existing behavior of Struct to be preserved, I don't care how it's called, and I don't mind if you add more types, too. You can rename Struct with current behavior to something else, and change existing behavior to something that fits better. I really don't have a preference.

@sagmor

This comment has been minimized.

sagmor commented Apr 13, 2016

About the nil and defaults, maybe an undefined value could be useful.

Something like this could work:

MeaningfulNil = Dry::Types['strict.nil'] | Dry::Types['strict.int'].default(42)

MeaningfulNil[nil] # => nil
MeaningfulNil[Dry::Types::Undefined] # => 42

Then a Struct that don't care about missing keys would encode them as Undefined rather than nil
Let me know if you think there's another use case for something like that and I could explore it further.

@sagmor

This comment has been minimized.

sagmor commented Apr 13, 2016

About the structs dilema, I think that all this Struct types can get quite confusing since they hide the meaning of stuff (What does Struct::Strict should really mean?), to me the struct "configuration" kinda makes sense. And in a way, calling constructor_type is already a kind of configuration.

It seams that it could be made simple if by default Struct behaved just like it does now (expect symbols for every key and discards unknown keys) and include some modules that perform very specific preprocessing on the initialization hash.

So something like:

class MyStruct < Dry::Types::Struct
   constructor :fill_undefined_keys, :reject_extra_keys, :simbolize_keys
end

That would be very explicit about what's expected and can be implemented by including modules that change the hash argument on initialize and call back super (the only gotcha would be to properly handle the order so :simbolize_keys executes always first)

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