Handle server v4.3 changes in answering UPDATE query

[hongquan]

In v4.3, when we execute UPDATE query with named parameters (passed via Value::Object), the server answers CommandDataDescription with different order of object fields, that makes our client failed with MismatchObjectShape.

This PR is to let the client flexible with that.

[hongquan]

Sorry, it includes many changes in coding style, just because my editor (Helix) runs Rustfmt automatically.

[Dhghomon]

Thanks for the PR!

I think this looks good, nice comments to show what's being done at each step and why. So looks like the changes are impl PartialEq for ObjectShapeInfo (for the ensure! macro on line 678) and doing the mapping inside fn encode, everything else is coding style. On that note, there are some files that don't work well with rustfmt but looks good for this one.

Line 711 could be a bit more readable as something like this:

server_field_names.sort_unstable_by_key(|(key, _)| our_field_names.get(key));

And I would change server replied ObjectShape to server returns an ObjectShape (just wording inside a comment)

Also I see the change adds a dependency to edgedb-protocol which has very few of them but log is so incredibly small that I think it's fine to add it here.

I'll ping @fantix to get a second opinion just in case!

P.S. The context is that this trick doesn't work anymore after 4.3: https://quan.hoabinh.vn/post/2023/08/querying-edgedb-with-name-parameters-in-rust

[hongquan]

Thank you, @Dhghomon, I updated as per your suggestion.

[fantix]

Thanks for the PR! Overall, I don't think sorting the codecs/fields per object is a good the right idea regarding performance to move forward around the issue you met. Let's try to figure out why this stopped working for you in 4.3 first.

In v4.3, when we execute UPDATE query with named parameters (passed via Value::Object), the server answers CommandDataDescription with different order of object fields, that makes our client failed with MismatchObjectShape.

Is there a reference PR/issue that elaborates on this change? Or could you please provide an example (schema + query) of how you get a MismatchObjectShape? To my knowledge, the input desc is still a list that maps to the params in the query, or it's a server bug.

Sorry, it includes many changes in coding style, just because my editor (Helix) runs Rustfmt automatically.

Please kindly put the reformatting changes in a separate commit.

[hongquan]

Is there a reference PR/issue that elaborates on this change? Or could you please provide an example (schema + query) of how you get a MismatchObjectShape? To my knowledge, the input desc is still a list that maps to the params in the query, or it's a server bug.

This issue is found out by me, so this PR is the first place it is reported.
I discovered the issue by debugging the edgedb-protocol, that's why in this PR, I added log dependency.
I don't have a minimal example to demonstrate yet. I encountered this issue when my personal website (source code) stopped working after upgrading server to v4.3. I had to fork edgedb-rust, adding log, setup tracing to observe what was wrong, and found out it was from the server response.

[hongquan]

the input desc is still a list that maps to the params in the query

Yes, still the list, but it is not the problem. The problem is the order of elements in that list, which server responded.

Whether it is server bug or not, it is by design. Normally, in other languages (Python, JS), the order of object/dictionary fields doesn't matter, so even when server responds with swapped order, the clients in those languages are still fine.
But in Rust client, because we encode the object as a list of fields, so suddenly the order becomes a matter.

I don't think sorting the codecs/fields per object is a good idea regarding performance

Yes, if the Rust client have another way to encode the Object, we will not need to do sort. By the way, I tried to reduce the performance impact by using sort_unstable, and avoid allocation.

Please kindly put the reformatting changes in a separate commit.

If you approve the approach of this PR, I will recreate another PR with those coding style changes stripped.

[fantix]

Actually, the underneath object fields/elements in EdgeDB are, by design, a stable list (this is crucial for the protocol/type descriptors), not a PL-ish order-free dictionary (btw recent Python dict maintains the insertion order). Again, the issue you met looks like a misorder bug, and your reproduction is the key (and appreciated!) to a proper fix.

my personal website (source code) stopped working after upgrading server to v4.3.

Could you point me to a more specific query in your repo that stopped working?

[hongquan]

Could you point me to a more specific query in your repo that stopped working?

Every UPDATE queries which use named parameters. The misorder doesn't happen with CREATE, doesn't happen with UPDATE and positional parameters.

I've just remembered that I created a small internal tool, to debug the issue, where I issue an UPDATE query with just two parameters. You can see it here.

[msullivan]

OK, I can confirm that in 4.3 the server started listing those parameters in a different order, though I haven't tested anything with edgedb-rust. The CLI will prompt for them in the reverse order.

UPDATE BlogCategory FILTER .id = <uuid>$id SET { title := <str>$title };

was the query

[hongquan]

Ok. I close this PR and wait for the fix from server.

[msullivan]

I don't think the server is doing anything wrong

[hongquan]

I don't think the server is doing anything wrong

Oh, so which side should be fixed? The server or this Rust client?

[msullivan]

Probably the Rust client, though we still need to reproduce a failure.

[hongquan]

I will introduce EdgeDB in a FOSS conference, FOSSASIA Summit 2024. I hope this bug be addressed soon.

[msullivan]

I will introduce EdgeDB in a FOSS conference, FOSSASIA Summit 2024. I hope this bug be addressed soon.

A minimal reproducible example would go a long way towards getting this fixed.

[MrFoxPro]

I can confirm #298 is fixed by this PR.

[MrFoxPro]

Should be fixed with #304

[hongquan]

Closing in favor of #304 .
Thank @MrFoxPro for making this progress.