Refactoring: Encode / Decode Sips.types at low level

[dominique-vassard]

How things are

Today, all Bolt.Sips.Types are encoded / decoded at "high level" while this operations should be done a "low level", i.e in the bolt encoding layer. This means that they are encoded twice:

• first encoded to a internals-compliant structure / decoded from binary to list
• then encoded into Bolt-compliant binary format

Or decoded twice:

• first decoded from binary to list
• then converted to a Bolt.Sips.Type

How they will be

Each Bolt.Sips.Type should encoded / decoded at the bolt encoding / decoding layer.

Why?

• Performance: I imagine that encoding / decoding only once will be faster that twice, even if there is little gain
• Further development: Code things once
• Readability: When searching for encoding / decoding, check only one place

Implications

End user won't be impacted, as everything will be internally handled.

Some big changes are expected: Bolt version will be split in their own files. AS I figure out when testing, a request with unsupported data hangs, then it has to be handled before being sent to server. Architecture will reflect the one used in: https://github.com/dominique-vassard/bolt-neo4j with encoder_v1, encoder_v2, etc. This will be clearer and future version could be easily added, tested and not lost inside one big file. bolt_protocol.ex should follow the same pattern as bolt v3 introduces ne messages and having everything in the same file is just a mess.

This change also means that bolt version should be passed all the way down to the encoding / decoding layer. In order to hide this from a user perspective (and to not break current API), it will be stored in conn.

Let's do it! :)

[dominique-vassard]

Regarding conn and bolt_version. Touching conn appears to be a bad idea as it implies massive changes for the least. Storing it at a lower level (in the DBConnection state) seems a better idea, it's more focused and it allows to access this value in the dbconnection callbacks such as handle_begin, handle_rollback, etc. Which is pretty nice as Bolt V3 introduces special message for transaction (BEGIN, COMMIT, ROLLBACK) and invalidates the usage of "BEGIN/COMMIT/ROLLBACK" in cypher queries.

[florinpatrascu]

Storing it at a lower level (in the DBConnection state)

I thought about this too, but then for the bolt+routing we'll first have to connect before starting a pool of connections, check the server version and decide the routing strategy only after we have the server version. Storing it in the DBC state is probably not the ideal way, when recalculating the routes?! Not sure yet.

[dominique-vassard]

I don't know, I didn't look deep enough in bolt+routing to understand exactly how it works. but the state which I update is one used in Bolt.Sips.Protocol then it's not the one where pool is stored. In fact there was only sock in it, now it contains %ConnData{sock: sock, bolt_version: 1}. so I imagine that adding a routes key is manageable.

I'm not 100% sure but I don't think there will be problem as only one connection is made and the first message exchanged with the server is the handshake which contained its preferred bolt version. After that, messing with the routes won't change the bolt version.

[florinpatrascu]

oh I misunderstood what you meant by DBConnection state, sorry 👍

[dominique-vassard]

No problem.

I'll be off for 1.5 week at least, so don't expect a PR soon :)

[dominique-vassard]

done :)