metosin / malli

High-performance data-driven data specification library for Clojure/Script.
Eclipse Public License 2.0
1.44k stars 204 forks source link

Decision: Open or Closed maps by default? #31

Closed ikitommi closed 4 years ago

ikitommi commented 4 years ago

Schema has closed maps, Spec has open maps. Which one should mallidefault to? Controlled via :map property, which would be (depeding on the default) either :closed or :open.

Closed by default

[:map [:x int?]]

[:map {:open true} [:x int?]]

Open by default

[:map [:y int?]]

[:map {:closed true} [:x int?]]
ikitommi commented 4 years ago

This could also be a property of a value transformation like in spec-tools.

ikitommi commented 4 years ago

Proposal: open by default, can be closed via a) schema property :closed and via a value transformer at runtime.

metametadata commented 4 years ago

I'd vote for closed maps by default because in my code they are the majority of specs and also it seems to be safer by default (e.g. imagine specing the service which sends data to a third party: the closed map will ensure that nothing potentially sensitive slips through the service by mistake).

ikitommi commented 4 years ago

Thanks for the comments! If schemas were open by default, one could:

1) use a "close all maps" visitor to close them. Could be a helper for this, e.g. open & close helpers.

2) one strip away the extras in&out with schema transformers. There is an example of this: https://github.com/metosin/malli/blob/master/README.md#value-transformation

lomin commented 4 years ago

Imho being "spiritual compatible" to clojure.spec regarding open maps is valuable, since malli could piggyback on the thoughts and development which is put into this design philosophy by Cognitect and most of the Clojure community.

ikitommi commented 4 years ago

:closed property might not be a good idea. It doesn't compose with :map-of, e.g. getting this to work would be hard:

[:and 
 [:map {:closed true} [:x int?]]
 [:map-of string? string?]]

If merging :map & :map-of( #43) would have an good solution, we could have the closed. But need to figure out how to do that robustly, see #52 for possible errors.

Maybe something like this:

closed map

[:map 
 {:closed true}
 [:x int?]
 [:y int?]]

closed map with extra keys

[:map 
 {:closed true
  :extra-entry [string? string?]}
 [:x int?]
 [:y int?]]
ikitommi commented 4 years ago

Btw, JSON Schema also allows extra keys by default. So, let's hava that as the default.

JSON Schema examples:

Also, JSON Schema has the following map constraints:

esuomi commented 4 years ago

In practice I've never worked on a long-running system with external integrations(1) where some non-specified fields wouldn't come and go at the weirdest times for various reasons so I'll definitely give my vote for having open schema as default. For team/system internal and those places where strictness has significant value having the option to make the schema closed makes more sense.

Question: Would this be configurable on different levels in nested structures as well? I'm assuming yes, but I also wonder how that would work with composition and whatnot, eg. does merging open+close become open or should it error?

And name for property, how about

(1) that is, integrations to systems done by some other team/company within the same organization or client environment

ikitommi commented 4 years ago

For the question: it's not a nested property: having a top-level closed schema doesn't make the children closed, you have to close those yourself. In schema-tools, there are helpers like open-schema which opens up schemas recursively and with-keyword-keys and with-keys that modify only the given schema. Has worked really well, using actively both variants and there is an issue to port those in to malli, with a polished api most likely.

About merging. plan (#82) is to change mali.core/merge into malli.set/union, which would give clear math on how it should work: becomes open.

union

As with any opinions, might be a good idea to have option to override.

ikitommi commented 4 years ago

Closed maps coming via #156

ikitommi commented 4 years ago

In master. Also, with malli.util/open-schema and malli.util/closed-schema helpers.