With nexedi/neoppod@fd80cc30 NEO/py added support to encode the compression
algorithm with the 'Compression' parameter. Before this, compression could
only be true (= with compression) or false (= without compression). Now
the absence of compression is encoded with 0. Any other number than 0
encodes a compression algorithm. The mapping is currently:

	1 = zlib

In the future, 2 could mean zstd [1].

[1] https://github.com/facebook/zstd/issues/1134

This reverts commit 93f25df5.

This patch aims to switch the compression parameter to the same
data type that it had before the introduction of msgpack: boolean [1].

1. It’s a smaller size on the wire [2].
2. It simplifies NEO/go code (we don't need to re-implement pythons
   __bool__ in golang to cast Optional[int] into a boolean).
3. It doesn’t complicate NEO/py code: in fact it doesn’t even
   need a protocol change because __bool__ keeps the same.
4. It’s more coherent regarding the protocol: compression can really only
   be either True or False. A compression of any other value than 0 or 1
   is meaningless.

We now need to explicitly cast compression to bool and not forget this when
adding new packets / code, but this is the same what happened to other packet
arguments (last argument of AskStoreTransaction needs to be explicitly cast
to list after switch to msgpack [3]).

---

[1] https://lab.nexedi.com/nexedi/neoppod/-/blob/ee44a3b0/neo/lib/protocol.py#L1113
[2] See msgpack specification: https://github.com/msgpack/msgpack/blob/master/spec.md?plain=1#L166-L178
[3] nexedi/neoppod@9d0bf97a

This reverts commit 0e4bb572.

This reverts commit d779695c.

Currently it can be:

- nil
- bool
- integer

In python this isn't really a problem due to '__bool__', but in Golang
we don't have this. Yes - it would be possible to make a parser in
Golang to decode all of these values as either True/False, but this is
much more complex than just adjusting python to send consistent data
types. It's also easier to describe a protocol where one parameter is
always 'bool' instead of having a parameter that is 'bool | int | NoneType'.

This is the decoding part of 37dd83e9

NEO/py sometimes sends 'None' for boolean values (being equal to 'False'
due to pythons 'bool(None) == False' logic). Let's accept python based
False values (= Nil) as equally acceptable.

Without this patch we can see errors as:

xtesting.go:330: load 0285cbad6e147b66:0000000000000005: returned err unexpected:
        have: neo://127.0.0.1:30002/1: load 0285cbad6e147b66:0000000000000005: 127.0.0.1:48928 - 127.0.0.1:33051 .251: decode: decode: M: AnswerObject.Compression/op: got opcode 00;  expected any from c3c2

In pre-msgpack protocol an 'INVALID_{TID,OID}' was always
decoded as 'None' in NEO/py [1]. But in msgpack protocol
this isn't true anymore. An `INVALID_TID` is now decoded
as an `INVALID_TID`. And this then leads to errors later [2].
Instead of restoring NEO/py to a pre-msgpack behaviour, we
fix this on NEO/go side by encoding 'INVALID_{TID,OID}' to NIL
on the wire. This is good as it makes the messages we send
smaller.

[1] https://lab.nexedi.com/nexedi/neoppod/-/blob/6332112cba979dfd29b40fe9f98d097911fde696/neo/lib/protocol.py#L579-583
[2] With SQLite backend we can see the following exception:

Traceback (most recent call last):
  File "/home/levin/neo/neo/tests/functional/__init__.py", line 192, in start
    self.run()
  File "/home/levin/neo/neo/tests/functional/__init__.py", line 288, in run
    getattr(neo.scripts,  self.command).main()
  File "/home/levin/neo/neo/scripts/neostorage.py", line 32, in main
    app.run()
  File "/home/levin/neo/neo/storage/app.py", line 196, in run
    self._run()
  File "/home/levin/neo/neo/storage/app.py", line 227, in _run
    self.doOperation()
  File "/home/levin/neo/neo/storage/app.py", line 301, in doOperation
    poll()
  File "/home/levin/neo/neo/storage/app.py", line 145, in _poll
    self.em.poll(1)
  File "/home/levin/neo/neo/lib/event.py", line 160, in poll
    to_process.process()
  File "/home/levin/neo/neo/lib/connection.py", line 508, in process
    self._handlers.handle(self, self._queue.pop(0))
  File "/home/levin/neo/neo/lib/connection.py", line 93, in handle
    self._handle(connection, packet)
  File "/home/levin/neo/neo/lib/connection.py", line 108, in _handle
    pending[0][1].packetReceived(connection, packet)
  File "/home/levin/neo/neo/lib/handler.py", line 125, in packetReceived
    self.dispatch(*args)
  File "/home/levin/neo/neo/lib/handler.py", line 75, in dispatch
    method(conn, *args, **kw)
  File "/home/levin/neo/neo/storage/handlers/client.py", line 67, in askObject
    o = app.dm.getObject(oid, at, before)
  File "/home/levin/neo/neo/storage/database/manager.py", line 484, in getObject
    before_tid and u64(before_tid))
  File "/home/levin/neo/neo/storage/database/sqlite.py", line 336, in _getObject
    r = q(sql + ' AND tid=?', (partition, oid, tid))
OverflowError: Python int too large to convert to SQLite INTEGER

This reverts commit 4b25a225.

Structures are encoded as arrays, but this isn't true for the structure
'RowList'. RowList in python is simply a list of lists. It's not a
nested array, as using a structure would expect. Therefore we need a
special case for RowList. Alternatively we could adjust proto.go to no
longer use 'RowList', but simply '[][]CellInfo', but this would perhaps
break N encoding.

certainly we need to adjust this in protogen and not in zproto-marshal,
the question is only what's the best way how this could be done..

In pre-msgpack protocol, the msgcode increment logic is like this:

Notify 	 add +1 to next msgcode
Request  add +0 to next msgcode   (next msg is answer & should therefore be the same)
Answer 	 add +1 to next msgcode

('Answer' msgcode is adjusted by 'AnswerBit')

In post-msgpack protocol, the logic is a bit different:

Notify 	 add +1 to next msgcode
Request  add +0 to next msgcode   (next msg is answer & should therefore be the same)
Answer 	 add +2 to next msgcode

So here we produce gaps after Request/Answer pairs.

In the current version, this patch breaks the functionality of
the pre-msgpack protocol, because for both protocols the same
msgcodes are used. To still support 'N' we need to use different
msgcodes depending on if 'N' or 'M' is used.

This fixes a stucked client: now the handshake packet is correctly
proceeded.

This is a fix of 1cf2712f:
'AppendString' is the equivalent to NEO/py 'NEO' encoding, not
'AppendBytes'.

Because 'err' was locally assigned inside the loop of 'DialLink' [1],
it was always 'nil' as a function return value, even when
'handshakeClient' actually returned an error. This lead to the
unfortunate situation that the function sometimes returned 'link=nil'
and 'err=nil', so that the function caller tried to access 'link'
attributes which lead to 'runtime error: invalid memory address or nil
pointer dereference'. This patch fixes this and now the function
correctly returns an error if the dialing fails.

[1] `peerConn, err := networker.Dial(ctx, addr)`

Not only for performance reasons (at least 3% faster) but also because of
several ugly things in the way packets were defined:
- packet field names, which are only documentary; for roots fields,
  they even just duplicate the packet names
- a lot of repetitions for packet names, and even confusion between the name
  of the packet definition and the name of the actual notify/request packet
- the need to implement field types for anything, like PByte to support new
  compression formats, since PBoolean is not enough

neo/lib/protocol.py is now much smaller.

* master:
  go/zodb: Handle common options in zurl in generic layer

/reviewed-by @kirr
/reviewed-on !4

* kirr/t+new-uri:
  Revert "Y client: Adjust URI scheme to move client-specific options to fragment"
  fixup! client.go: Fix URI client option parsing for supported + unsupported options
  client.go: Fix URI client option parsing for supported + unsupported options
  fixup! client_test: Add tests for NEO URI parser
  client_test: Add tests for NEO URI parser
  fixup! client: Refactor openClientByURL for easier testing
  client: Refactor openClientByURL for easier testing
  Y go/zodb: Handle common options in zurl in generic layer

Offload drivers from handling options such as ?read-only=1 and force
them to deal with such options only via DriverOptions, never zurl.

See added comment for details.

/reviewed-by @levin.zimmermann
/reviewed-on kirr/neo!4

This reverts commit 4c9414ea.
This patch was added at a time when nexedi/neoppod!18 wasn't
resolved yet, but we already wanted to proceed with WCFS. Now the NEO MR
is resolved and we decided to mostly leave the NEO zurl as it was
originally implemented in nexedi/neoppod!6.
This means we don't need this patch anymore which changed the NEO
zurl format.

readonly is handled by common zodb.OpenDriver.

Before this patch, the parser ignored options which were already supported
by the client (for instance 'read-only') and even raised an error. But the
client can already use this option: as a9246333 describes this should happen in the
local storage URL parser.

Furthermore not-yet-supported client options (for instance compress) broke
the NEO client before this patch. Now these options only raise a warning which
informs the user that they are ignored. Why? We want to use pre-complete NEO
in real-world projects together with NEO/py clusters. Those real-world projects
may already specify options which aren't supported by our NEO/go client yet.
But it doesn't matters so much, because those options are mostly
relevant for other NEO/py cluster clients (e.g. zope nodes). Instead of
filtering those parameters before parsing them to NEO/go in a higher
level (e.g. SlapOS), NEO/go should already support any valid NEO URL
and raise warnings for not yet implemented features.

- use simplified parseURL signature - DriverOptions are not passed nor changed there.
- read-only is handled by generic zodb layer not neo.parseURL .

This test was missing so far. Particularly recent changes of the
NEO URI scheme [1], but also problems with valid old URI [2] stressed
out the necessity for comprehensive NEO URI parser tests.

[1] 4c9414ea
[2] levin.zimmermann/neoppod@573514c6 (comment 184417)

- no need to pass DriverOptions into parseURL - it is only zurl that is
  parsed, and also DriverOptions should not be changed by the opener.

- no need to document "If anything fails within this process an error
  and nil are returned." because that is standard omnipresent Go convention.

With all the recent changes of the NEO URI scheme we need to reliably test
the function which parses the URI and convert it into the different
parameter. Testing is much simpler if we can only analyse how the URI
parsing works. Therefore this patch moves NEO URI parsing to an
external function.

Offload drivers from handling options such as ?read-only=1 and force
them to deal with such options only via DriverOptions, never zurl.

See added commend for details.

I missed the following build failure in go/neo/cmd:

    # lab.nexedi.com/kirr/neo/go/neo/cmd/neo
    ./storage.go:128:37: cannot use master (variable of type string) as []string value in argument to neo.NewStorage