Can't generate schema of a composite spec

[punit-naik]

Currently, if a spec if defined using s/merge or s/keys i.e. a composite spec, generating datomic schema out of it using spectomic.core/datomic-schema does not generate schema for all the individual specs it is made of.
If this is implemented, it will alleviate the pain of individually populating the spec vector to be fed to the spectomic.core/datomic-schema function.
Overriding of the individual specs' schemas should still be possible.

[kennyjwilli]

We have also thought that this would be a useful feature.

The way you implemented this feature in #11 is by including a separate function datomic-schema-from-composite-spec that parses a spec and returns the schema. I think the operation here is a bit different. Really this feature is asking for is a way to return all the keys for a Spec describing a map. That result could then be passed to datomic-schema or datascript-schema. For example,

(spectomic/datomic-schema [(with-map-keys ::user)])
=> [{:db/ident       :entity/id
     :db/valueType   :db.type/uuid
     :db/cardinality :db.cardinality/one
     :db/unique      :db.unique/identity
     :db/index       true}
    {:db/ident       :user/name
     :db/valueType   :db.type/string
     :db/cardinality :db.cardinality/one}
    {:db/ident       :user/favorite-foods
     :db/valueType   :db.type/string
     :db/cardinality :db.cardinality/many}
    {:db/ident       :user/orders
     :db/valueType   :db.type/ref
     :db/cardinality :db.cardinality/many}]

I think the with-map-keys approach is good, but the implementation is not entirely clear. The user may pass a Spec to with-map-keys that cannot be parsed so easily (i.e. one using a custom predicate or some form that we did not handle in our parser). The way we solve this problem now is by falling back on the generator defined for the Spec. We could do the same here when we cannot parse the Spec -- take the generator off of the spec and generate a number of samples and figure out the keys. Because a map can have optional keys we'd need to ensure that we are able to generate a complete set of the keys (including all optional keys). And this needs to be done with a high degree of confidence that all the map's keys are included (we wouldn't want to generate schema that includes a different set of keys each time).

A possible implementation here is to continue generating values for the map spec, taking the keys off that map and storing them in a set. We'd then continue doing this until that set stops changing or N number of iterations was hit. If N iterations was hit and the set of keys was still changing then we throw an exception.

Alternatively, the with-map-keys could be implemented only as a Spec parser and throw an exception when it does not understand the form. I think this would handle the majority of cases. I'd be interested in seeing the results of the above generator approach before implementing this function only as a Spec parser.

[punit-naik]

My datomic-schema-from-composite-spec internally uses datomic-schema function. One can still use the datomic-schema function directly by saying (datomic-schema (split-composite-spec ::user)). Here the split-composite-spec is doing the job of with-map-keys. It will extract out all its individual spec components.
I just added the extra function datomic-schema-from-composite-spec because I wanted to give the user an option to override the schema of some individual spec components the way we currently do using (datomic-schema [[:entity/id {:db/index true}] :user/orders :user/favourite-foods]).Maybe I could improve upon the naming of the function.
I can work on the generator approach in a separate PR as an enhancement to this i.e. when the composite spec is unparseable.

[kennyjwilli]

Your function correctly uses datomic-schema on the keys of a map Spec. The problem is coming up with a way to get the list of keys from a map Spec. The generator approach may be able to solve this in the general case.

Yes, the function needs to be renamed. composite-spec is not a universally recognized term. I'd vote for with-map-keys.

[punit-naik]

Thanks for the clarification. But I still did not understand the case for using generators. Is it because if there are multiple levels of compositions in a spec i.e. if ::user is made of ::orders which is in turn a collection of ::order, we want to generate schema for ::order as well and just not ::orders which is a ref on ::order?
If that is the case, I have written a function which recursively fetches all the basic component specs of a composite spec. But I think in this case the generator approach will be more fool proof.
Am I right in my understanding?

[sparkofreason]

Link to code I've been using to extract map keys for composite specs:

https://github.com/Provisdom/maali/blob/master/src/provisdom/maali/rules.cljc#L73

[punit-naik]

@kennyjwilli Instead of using the generator approach, I walked through the form/description of the composite spec and basically merged all the optional keys of a component spec into the required keys, regenerated the entire composite spec and generated a sample record for it. After that I just took out all the keys from that sample record and fed it into the datomic-schema function. What do you think about this approach?
Please take a look at the linekd PR.

[punit-naik]

@sparkofreason So sorry, just saw your code. Looks like You and me are following the same approach.

[kennyjwilli]

The with-map-keys function usage looks good. I do not agree with the implementation, however. The simplest form of this function would be to parse the map Spec and return the map's keys. If you cannot parse the map Spec, then throw an error.

Take a look at @sparkofreason's code. We need a function that takes a map Spec and returns all keys (optional and required) for that Spec. That result can be passed to the datomic-schema function. This function does not need to take an overrides map. Overriding something means you will explicitly pass the attribute to the datomic-schema function.

A note on coding style: You use an atom here within a function definition. Although the function is pure, this is not idiomatic Clojure. That function could be rewritten using cond-> to remove the use of the atom.

[punit-naik]

@kennyjwilli Will be omitting the atom to make the code more idiomatic.
As for the with-map-keys function, I thought of overloading it with another schema-override-map param as I thought the user might want to add properties like db/unique and didn't want to go through the hassle of writing code for it. Anyways, if this additional functionality is not required, I will remove it.

[punit-naik]

@kennyjwilli Please take a look at #14 for the updated code.