mozilla / mentat

UNMAINTAINED A persistent, relational store inspired by Datomic and DataScript.
https://mozilla.github.io/mentat/
Apache License 2.0
1.65k stars 115 forks source link

[tx] `transact_builder` is not type-checking input values #663

Closed ncalexan closed 6 years ago

ncalexan commented 6 years ago

It appears that transact_builder is not type-checking the provided values, so that, say:

builder.add(sl.clone(), in_progress.get_entid(&ATTR).expect("attr"), TypedValue::typed_string("test")).expect("to add");

and

builder.add(sl.clone(), in_progress.get_entid(&ATTR).expect("attr"), TypedValue::Keyword(kw!(:some/value).into())).expect("to add");

successfully transact. It's not clear to me where in the stack the error is:

In either case this is critical!

ncalexan commented 6 years ago

Oh dear. transact_builder invokes transact_terms

https://github.com/mozilla/mentat/blob/ab957948b4f642e9d4e5e6ac8169c8b382295573/db/src/tx.rs#L785

which invokes transact_simple_terms

https://github.com/mozilla/mentat/blob/ab957948b4f642e9d4e5e6ac8169c8b382295573/db/src/tx.rs#L544

which skips the first stage of the transact pipeline that turns entities into terms... and along the way does all of the schema-aware typechecking

https://github.com/mozilla/mentat/blob/ab957948b4f642e9d4e5e6ac8169c8b382295573/db/src/tx.rs#L532

It's probably not hard to address this, with some slight duplication of work, by growing typechecking assertions in the SchemaTypeChecking trait and invoking them as part of the term processing loop. It will be a little awkward that passing an EDN :keyword/value ident as the value to an attribute of type :db.type/ref will look up the entid for you, but (as written) passing a term TypedValue::Keyword(...) as the value to an attribute of type :db.type/ref won't look up the entid for you... but that's the advantage and disadvantage of using a lower level interface.

ncalexan commented 6 years ago

Addressed by https://github.com/mozilla/mentat/commit/46c2a0801f450f4f62482ee067ed272e1057a747.