Support named arguments

[MrFoxPro]

Basic usage:

use edgedb_protocol::eargs;

let mut args = eargs! { "a" => "a".to_string() };
args.extend(eargs! { "b" => Some(5_i64) });

edb.query_required_single_json("select { a := <str>$a, b := <optional int64>$b }", &args)
.await?;

[MrFoxPro]

I realised it probably doesn't need indexmap crate, it's possible to use simple std::collections::HashMap instead.

[MrFoxPro]

I don't like .clone() here, I think it's possible to avoid it, but I'm not sure how!

[MrFoxPro]

Also I'm not sure about Cardinality::Many. Is it relevant for query arguments?

[msullivan]

Also I'm not sure about Cardinality::Many. Is it relevant for query arguments?

I don't think so, no.

[msullivan]

Thanks for the contribution!

I think this will suffer from the same problem that started all of this: it doesn't understand how to put the arguments in the correct order. It will need to consult the expected order from the server and put them in that order.
It should also be able to get all of the cardinalities that way.

[msullivan]

Sure, but I think I would rather not merge #299 and instead do the logic while handling named arguments themselves. It seems weird to me to do the ordering at the object encoding layer, since the object is ordered in the protocol.

…

On Wed, Mar 27, 2024, 14:09 MrFoxPro ***@***.***> wrote: Thanks for the contribution! I think this will suffer from the same problem that started all of this: it doesn't understand how to put the arguments in the correct order. It will need to consult the expected order from the server and put them in that order. It should also be able to get all of the cardinalities that way. For me it works fine when merged with #299 <#299> — Reply to this email directly, view it on GitHub <#304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACTC7PUS5IQRUPVAMRYR4LY2MYRNAVCNFSM6AAAAABFK26ZLOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTHE4TIOBWGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[MrFoxPro]

As I understand the problem is that now driver only checks

edgedb-rust/edgedb-protocol/src/query_arg.rs

Lines 195 to 246 in 0bf0949

	fn check_descriptor(&self, ctx: &DescriptorContext, pos: TypePos) -> Result<(), Error> {
	use Descriptor::*;
	use Value::*;
	let mut desc = ctx.get(pos)?;
	if let Scalar(d) = desc {
	desc = ctx.get(d.base_type_pos)?;
	}
	match (self, desc) {
	(Nothing, _) => Ok(()), // any descriptor works
	(_, Scalar(_)) => unreachable!("scalar dereference to a non-base type"),
	(BigInt(_), BaseScalar(d)) if d.id == codec::STD_BIGINT => Ok(()),
	(Bool(_), BaseScalar(d)) if d.id == codec::STD_BOOL => Ok(()),
	(Bytes(_), BaseScalar(d)) if d.id == codec::STD_BYTES => Ok(()),
	(ConfigMemory(_), BaseScalar(d)) if d.id == codec::CFG_MEMORY => Ok(()),
	(DateDuration(_), BaseScalar(d)) if d.id == codec::CAL_DATE_DURATION => Ok(()),
	(Datetime(_), BaseScalar(d)) if d.id == codec::STD_DATETIME => Ok(()),
	(Decimal(_), BaseScalar(d)) if d.id == codec::STD_DECIMAL => Ok(()),
	(Duration(_), BaseScalar(d)) if d.id == codec::STD_DURATION => Ok(()),
	(Float32(_), BaseScalar(d)) if d.id == codec::STD_FLOAT32 => Ok(()),
	(Float64(_), BaseScalar(d)) if d.id == codec::STD_FLOAT64 => Ok(()),
	(Int16(_), BaseScalar(d)) if d.id == codec::STD_INT16 => Ok(()),
	(Int32(_), BaseScalar(d)) if d.id == codec::STD_INT32 => Ok(()),
	(Int64(_), BaseScalar(d)) if d.id == codec::STD_INT64 => Ok(()),
	(Json(_), BaseScalar(d)) if d.id == codec::STD_JSON => Ok(()),
	(LocalDate(_), BaseScalar(d)) if d.id == codec::CAL_LOCAL_DATE => Ok(()),
	(LocalDatetime(_), BaseScalar(d)) if d.id == codec::CAL_LOCAL_DATETIME => Ok(()),
	(LocalTime(_), BaseScalar(d)) if d.id == codec::CAL_LOCAL_TIME => Ok(()),
	(RelativeDuration(_), BaseScalar(d)) if d.id == codec::CAL_RELATIVE_DURATION => Ok(()),
	(Str(_), BaseScalar(d)) if d.id == codec::STD_STR => Ok(()),
	(Uuid(_), BaseScalar(d)) if d.id == codec::STD_UUID => Ok(()),
	(Enum(val), Enumeration(EnumerationTypeDescriptor { members, .. })) => {
	let val = val.deref();
	if members.iter().any(|c| c == val) {
	Ok(())
	} else {
	let members = {
	let mut members = members
	.into_iter()
	.map(|c| format!("'{c}'"))
	.collect::<Vec<_>>();
	members.sort_unstable();
	members.join(", ")
	};
	Err(InvalidReferenceError::with_message(format!(
	"Expected one of: {members}, while enum value '{val}' was provided"
	)))
	}
	}
	// TODO(tailhook) all types
	(_, desc) => Err(ctx.wrong_type(desc, self.kind())),
	}
	}

that shape is correct, but it doesn't use this information to align shape in correct way. I guess it will require a lot of effort to change this behaviour, or users will have to enforce correct order and provide cardinality themselves explicitly.

[MrFoxPro]

Ok, maybe I exaggerated about level of effort required.

[MrFoxPro]

@msullivan would it be sufficient to align input shape to required one here:

edgedb-rust/edgedb-protocol/src/codec.rs

Lines 657 to 691 in 0bf0949

	fn encode(&self, buf: &mut BytesMut, val: &Value)
	-> Result<(), EncodeError>
	{
	let (shape, fields) = match val {
	Value::Object { shape, fields } => (shape, fields),
	_ => Err(errors::invalid_value(type_name::<Self>(), val))?,
	};
	ensure!(shape == &self.shape, errors::ObjectShapeMismatch);
	ensure!(self.codecs.len() == fields.len(),
	errors::ObjectShapeMismatch);
	debug_assert_eq!(self.codecs.len(), shape.0.elements.len());
	buf.reserve(4 + 8*self.codecs.len());
	buf.put_u32(self.codecs.len().try_into()
	.ok().context(errors::TooManyElements)?);
	for (codec, field) in self.codecs.iter().zip(fields) {
	buf.reserve(8);
	buf.put_u32(0);
	match field {
	Some(v) => {
	let pos = buf.len();
	buf.put_i32(0); // replaced after serializing a value
	codec.encode(buf, v)?;
	let len = buf.len()-pos-4;
	buf[pos..pos+4].copy_from_slice(&i32::try_from(len)
	.ok().context(errors::ElementTooLong)?
	.to_be_bytes());
	}
	None => {
	buf.put_i32(-1);
	}
	}
	}
	Ok(())
	}
	}

I mean just walk over input shape elements, match corresponding element by name, place it in correct order and try to adjust correct cardinality?

If so, is it worth to modify this PR and preserve only HashMap -> Value transformation?

[msullivan]

What I think that I would recommend (and this might be what you were saying), is to make an implementation of encode for HashMap (or maybe for an iterator of str, value pairs?) that constructs an object Value with the correct order and cardinality, and then encodes that. It would consult the encoder's ctx in order to do that.

[MrFoxPro]

I was struggling with making something more generic than hashmap, ended up with simple solution.

[msullivan]

The encoder looks good and matches what I had in mind.

Could you please add some tests for this? The rust bindings seem substantially under-tested, unfortunately, but we need to start improving that, and seeing examples as tests will make it easier to review the design of the interface.

I think the easiest way to add tests would be by adding them to edgedb-tokio/tests/func/client.rs

Also, I can read and write rust but I wouldn't call myself idiomatic; @aljazerzen could you please review the proposed interface here?

[msullivan]

Actually running the tests requires the tests being able to find an edgedb-server binary to run. This can be accomplished with something like

PATH=$PATH:$(dirname $(edgedb server info --latest --bin-path)) cargo test --workspace --all-features

[MrFoxPro]

The encoder looks good and matches what I had in mind.

Could you please add some tests for this? The rust bindings seem substantially under-tested, unfortunately, but we need to start improving that, and seeing examples as tests will make it easier to review the design of the interface.

I think the easiest way to add tests would be by adding them to edgedb-tokio/tests/func/client.rs

Also, I can read and write rust but I wouldn't call myself idiomatic; @aljazerzen could you please review the proposed interface here?

For sure, but I probably will wait for another review so I don't need to rewrite tests later too.
Also, I'm not familiar with writing Rust tests, so my tests could be not so good.

[MrFoxPro]

Examples for providing None values:

eargs! {
   "me" => 1 as i64,
   "e" => None::<Vec<f32>>,
   "q" => None::<String>,
   "sphere" => None::<String>,
   "lower" => None::<i64>,
   "upper" => None::<i64>,
   "geoarea" => None::<String>
}

[aljazerzen]

This looks like a sound API design to me.

Before merging, take a look at the few low-priority comments I've left about naming and definitely add a few tests.

For an example, you can see the tests I've added in #308.
If you want, I can get this PR over the line.

I've approved by mistake

[MrFoxPro]

This looks like a sound API design to me.

Before merging, take a look at the few low-priority comments I've left about naming and definitely add a few tests.

For an example, you can see the tests I've added in #308. If you want, I can get this PR over the line.

I will be happy if you add tests

[aljazerzen]

@MrFoxPro I've moved a few things around and added a bit of docs. Thank you for contributing!