Feature request or bug: adding properties directly to map keys.

[johesoman]

Hi Tommi!

I was surprised that it is not possible to add properties to keys directly, but I don't know if it is intended behavior or not. I found this when I was playing around with error messages, so the example focuses on that. As you can see the first expression evaluates with the error message, while the second one does not.

(-> [:map [:leet [:re {:error/message "should be '1337'"} #"^1337$"]]]
    (m/explain {:leet "1338"})
    (me/humanize {:wrap :message})) ;; => {:leet "should be '1337'"}

(-> [:map [:leet {:error/message "should be: '1337'"} #"^1337$"]]
    (m/explain {:leet "1338"})
    (me/humanize {:wrap :message})) ;; => {:leet "unknown error"}

If this is intended behavior, maybe the the fact that the second example does not work could be included in the docs.

[ikitommi]

Excellent observation, I was sure there was an issue about it, but seems there wasn't.

My first thought was that the key properties could be merged into child (here: value) properties, but it would raise an ambiguity issue: :optional should not be merged as it would mask the potentially relevant :optional in the value schema itself.

Another option to resolve this would be to introduce parent/child relationship between schema instances. This way, we could look parent properties at runtime when needed. Similar example with :and, where the error should be looked from parent if not present at child:

[:and {:error/message "fail"} int? [:> 18]]

for maps, we could create key-schema behind the scenes that is marked as parent for the value.

what do you think?

[ikitommi]

related #80 (the parent/child)

[johesoman]

I think that creating a key-schema behind the scenes and marking it as a parent for the value seems like a good idea!

[ikitommi]

Should be resolved for first alpha.

[rschmukler]

@ikitommi are you still thinking the parent / child relationship is the way to go on this? Would this mean that transformers would end up being able to reference them via m/-parent or something similar? And then getting properties becomes (m/properties (m/-parent schema))?

It may be worth stating that this could end up leading to inconsistent behavior for transformers, specifically if some of them end up checking parent properties, while others don't. I imagine it will be easy enough to get them to be consistent in malli.core but I do think it may lead to unintentional bugs in libraries / applications defining their own transformers. It also may be surprising for users that this is malli's behavior in core, but when consuming a library that didn't implement parent checking, the behavior is different.

In the case of merging properties into the child, and specifically merging {:optional true} into a child, isn't that essentially a no-op? Aren't the "key-props" irrelevant in the child schema, and properties more or less open maps?

ie. aren't

[:map [:name {:optional true} string?]]

and

[:map [:name {:optional true} [string? {:optional true} ;; this optional basically has no effect anyway
                                                        ;; and is the result of "merging" the parent
                                                        ;; into the child
  ] 

essentially equivalent?

Even if they aren't, it may be worth thinking whether writing the merge logic for parent to child properties is more complex than having parent / child. As always, no strong opinions, just thinking out loud.

[ikitommi]

Not happy with merge as the properties would be copied to children, potentially masking child properties. Also, things like serialization and walking over schemas would just see the copied properties.

(m/form [:map [:x {:optional true} :string]])
; => [:map [:x {:optional true} [:string {:optional true}]]]

In top-level, I guess the options are:

1) it's a feature

• entry meta is just for itself (e.g. :optional)
• having extra keys could later give warning
• doesn't resolve things, kinda sucks

2) have a generic way for childs to access the parent data

• pushes all the work to extensions & user provided code
• sucks, big time

3) have a way for each schema application (transformation, value generation etc) to resolve this

• applications use the properties differently, some short-circuit (generators), some go too deep (e.g. errors) -> but only once per application (e.g. the generic code which pulls errors messages, looks one step up too)
• would yield "just works", user provided code for a given application doesn't have to know about this
• might be a good goal
• need to implement and see if this is better than 1

... for the 3, there are few options, but I'll take the simplest path first and create a draft PR for reviews, have a good hunch how to do that without too many tears.

[ikitommi]

Started on the PR: #212

[rschmukler]

3 sounds promising. I agree with the hesitation for blanket overriding. The only question I have is whether this somehow limits extension outside of code in core, but since it looks like your approach is via a -parent function, I think advanced users should be fine. Will follow along with #212

[ikitommi]

I think this is resolved now, using 3. final part was the humanization, fixed in #462.

[ikitommi]

as a side note, not 100% happy how the handling of entries complexes everything. Maybe this can be simplified with 1.0.0?