wolf4ood / gremlin-rs

Gremlin Rust
Apache License 2.0
108 stars 30 forks source link

Implement graph binary protocol #217

Open criminosis opened 2 weeks ago

criminosis commented 2 weeks ago

@wolf4ood ~wanted to give you a heads up of the PR coming. If you get a chance please look at it while it's in this preliminary draft stage because I've made some significant overhauls and it'd be nice to get your feedback before I continue.~ It's no longer in draft.

I'll annotate particular areas below 👍 .

But in addition to implementing the GraphBinaryV1 protocol this PR makes a few other changes:

I intend to continue this next week, parameterizing the remaining test files so GraphBinary can piggyback on the tests that already exist 👍 .

criminosis commented 2 weeks ago

Also looks like there's something up with the grocv status?

criminosis commented 1 week ago

@wolf4ood flipping this to ready since I've completed what I had left outstanding. Let me know what you think, as always happy to make changes.

About a net negative 1k lines despite the graph binary protocol being added from parameterizing all the tests. This means any future protocol that gets added will just need to be added as an additional annotation in tests/common.rs and it will flow down to all the tests as an additional case. But means the graph binary protocol is now as tested as the prior protocols.

There was a few tests that were adjusted namely either skipping the older GraphSONV2 protocol because it doesn't support the needed message type or modifying the assertions to handle the different response type GraphBinary returns, usually making fallback assertions on the richer T (as in T::Id) type instead of simply a String("id") being the key in a hashmap, etc.

On the note of tests, the actual tests are passing but the grcov GH Action seems broken which is what is causing the overarching red x on the result for the PR FYSA. I believe that's out of my control to fix in this PR, but happy to rebase if you fix it 👍 .

criminosis commented 1 week ago

As previously mentioned so long as people were using the default GraphSONV3 (de)serializer and not explicitly setting it I believe this should be a transparent change. I didn't change the default, just added the new protocol as an option after renaming the enum. Figured changing the default is something that is your discretion.

criminosis commented 1 week ago

Flipping back to draft. While integrating this branch into my application logic I found some placed JanusGraph does things a little bit differently. Also discovered edge properties aren't being serialized in either GraphSONV2 or GraphSONV3, so fixing that as well atm.

criminosis commented 1 week ago

K, back to ready. Completed trying it out with my application and at least for my purposes it all works.