scryer-prolog icon indicating copy to clipboard operation
scryer-prolog copied to clipboard

Add support for using Term as query in run_query()

Open bakaq opened this issue 11 months ago • 2 comments

This introduces a feature that was discussed quite a bit before (https://github.com/mthom/scryer-prolog/pull/2465#issuecomment-2311231935, https://github.com/mthom/scryer-prolog/discussions/2500#discussioncomment-10436375, https://github.com/mthom/scryer-prolog/issues/2637#issuecomment-2427924784): using a structured Term instead of a wobbly string for queries. This makes it easier to procedurally generate queries, and also provides a good way to avoid injection attacks.

In the way I implemented, Machine::run_query() can now accept either (things that can be turned into) strings or a Term. A simple example (taken from the tests):

let mut machine = MachineBuilder::default().build();

// X = a.
let query = Term::compound("=", [Term::variable("X"), Term::atom("a")]);

let complete_answer: Vec<_> = machine
    .run_query(query)
    .collect::<Result<_, _>>()
    .unwrap();

assert_eq!(
    complete_answer,
    [LeafAnswer::from_bindings([("X", Term::atom("a"))])]
);

This could get even more ergonomic if we eventually make macros for creating Terms (though I think it would have a lot of limitations on the allowed syntax). Imagine:

let query = term!(X = a);

After doing this I'm really glad we are getting rid of parser::ast::Term in rebis-dev, because that type is a mess! However, from what I've read of the rebis-dev branch, it not existing will probably make this feature a bit harder to implement.

This also serves as even more incentive to base the C embedding interface on a Term based API instead of on top of JSON serialization.

bakaq avatar Jan 28 '25 17:01 bakaq

Anything we can do to prevent string injection would be very welcome for making Scryer an easy choice as a web native language!

jjtolton avatar Jan 30 '25 18:01 jjtolton

Converted to a draft because this will be significantly different in rebis-dev, so probably doesn't make much sense to merge in the current state.

bakaq avatar Feb 16 '25 07:02 bakaq