WIP frame: added remaining request response

[mmatczuk]

No description provided.

[mmatczuk]

Get rid of boilerplate docs i.e.

// AuthChallenge response message type.
type AuthChallenge struct {
	Token frame.Bytes
}

// ReadAuthChallenge reads and returns AuthChallenge from the buffer.
func ReadAuthChallenge(b *bytes.Buffer) AuthChallenge {
	return AuthChallenge{frame.ReadBytes(b)}
}

Should be

// AuthChallenge spec https://github.com/apache/cassandra/blob/10c685222fc415586ae28a01e7896063a3f2f0d3/doc/native_protocol_v4.spec#L54.
type AuthChallenge struct {
	Token frame.Bytes
}

func ReadAuthChallenge(b *bytes.Buffer) AuthChallenge {
	return AuthChallenge{frame.ReadBytes(b)}
}

Add this Read function to issues.exclude in golangci-lint file, it supports regular expressions.

[mmatczuk]

It is a bit exaggerated. I'd suggest moving Value, Inet and OpCode to separate type declarations outside of this group.

[mmatczuk]

What is the purpose of Value type?? I looks like it should be just Bytes.

[mmatczuk]

It would me much better to call those functions with Parse prefix instead of Read to avoid ReadWriteX.

[mmatczuk]

The code must NOT panic.

I suggest we add a buffer wrapper that would be capable of recording errors.

func ReadConsistency(b *bytes.Buffer) Short {
	c := ReadShort(b)
	if c > 10 {
		panic(unknownConsistencyErr)
	}
	return c
}

With

type Buffer struct {
    bytes.Buffer
    errors []error
}

func (b *Buffer) RecordError(err error)

The above becomes

func ReadConsistency(b *Buffer) Short {
	c := ReadShort(b)
	if c > 10 {
		b.RecordError(unknownConsistencyErr)
	}
	return c
}

The ReadConsistency func is still wrong as validation should be handled by Consistency type.

[mmatczuk]

s/Invalid/Unsupported/g

[mmatczuk]

s/single X/a X value/g

[mmatczuk]

Get rid of banners

//------------------------------- WRITE TESTS ---------------------------------

//------------------------------- BENCHMARKS ----------------------------------

If you need sth like that most likely those should be put to a separate file.

I'd suggest to benchmarks to xxx_bench_test.go and remove the "read write barriers"...

[mmatczuk]

We must talk about it.

[mmatczuk]

Each file shall have it's test file i.e.

batch.go
execute.go
options.go
...
request_test.go

should be

batch.go
batch_test.go
execute.go
execute_test.go
options.go
options_test.go
...

https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/io/fs/

[mmatczuk]

Do not use _ in file names. Typically _ is a tag separator for tags.

https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/os/

So auth_challenge.go becomes authchallenge.go

[mmatczuk]

How come auth_response.go is in request?

[mmatczuk]

Let's break comments at 120 lines, no need to break them too early.

[mmatczuk]

Always use field names when instantiating a struct there should be a linter for that.

[mmatczuk]

After thinking about it error.go with public error shall be in response because it's about server responding with some errors.
There may be some request errors too they belong to request pkg.

[mmatczuk]

When in doubt READ then ASK.

Linting errors are silenced with https://golangci-lint.run/usage/false-positives/

[mmatczuk]

Use field names.

[mmatczuk]

Do not prefix test names with anything like here

	for _, tc := range cases {
		t.Run(fmt.Sprintf("AuthResponse Test %s", tc.name), func(t *testing.T) {

=== RUN   TestValidErrorCodes
--- PASS: TestValidErrorCodes (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_server
    --- PASS: TestValidErrorCodes/Short_reading_test_server (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_protocol
    --- PASS: TestValidErrorCodes/Short_reading_test_protocol (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_authentication
    --- PASS: TestValidErrorCodes/Short_reading_test_authentication (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_overload
    --- PASS: TestValidErrorCodes/Short_reading_test_overload (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_is_bootstrapping
    --- PASS: TestValidErrorCodes/Short_reading_test_is_bootstrapping (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_truncate
    --- PASS: TestValidErrorCodes/Short_reading_test_truncate (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_syntax
    --- PASS: TestValidErrorCodes/Short_reading_test_syntax (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_unauthorized
    --- PASS: TestValidErrorCodes/Short_reading_test_unauthorized (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_invalid
    --- PASS: TestValidErrorCodes/Short_reading_test_invalid (0.00s)
=== RUN   TestValidErrorCodes/Short_reading_test_config
    --- PASS: TestValidErrorCodes/Short_reading_test_config (0.00s)

vs

=== RUN   TestValidErrorCodes
=== RUN   TestValidErrorCodes/server
=== RUN   TestValidErrorCodes/protocol
=== RUN   TestValidErrorCodes/authentication
=== RUN   TestValidErrorCodes/overload
=== RUN   TestValidErrorCodes/is_bootstrapping
=== RUN   TestValidErrorCodes/truncate
=== RUN   TestValidErrorCodes/syntax
=== RUN   TestValidErrorCodes/unauthorized
=== RUN   TestValidErrorCodes/invalid
=== RUN   TestValidErrorCodes/config
--- PASS: TestValidErrorCodes (0.00s)
    --- PASS: TestValidErrorCodes/server (0.00s)
    --- PASS: TestValidErrorCodes/protocol (0.00s)
    --- PASS: TestValidErrorCodes/authentication (0.00s)
    --- PASS: TestValidErrorCodes/overload (0.00s)
    --- PASS: TestValidErrorCodes/is_bootstrapping (0.00s)
    --- PASS: TestValidErrorCodes/truncate (0.00s)
    --- PASS: TestValidErrorCodes/syntax (0.00s)
    --- PASS: TestValidErrorCodes/unauthorized (0.00s)
    --- PASS: TestValidErrorCodes/invalid (0.00s)
    --- PASS: TestValidErrorCodes/config (0.00s)

Shorter is better.

[mmatczuk]

Also test names can be sentences i.e. {"is_bootstrapping", should be {"is bootstrapping", as space is converted to _ automatically.

[mmatczuk]

Please move the mass append tesing harness to frame/frametesting package.

[mmatczuk]

Maybe it's not needed actually.

Say we we have the Buffer wrapper as explained in #24 (comment).

Then all the functions can become methods.

Then this code

			massAppendBytes(StringToBytes("CREATED"),
				StringToBytes("AGGREGATE"),
				StringToBytes("test"),
				StringToBytes("myaggregate"),
				StringListToBytes([]string{"int", "int"})),

Can become

var b frame.Buffer
b.WriteString("CREATED").WriteString("AGGREGATE")...

It could even become

var b frame.Buffer
b.WriteStrings("CREATED",  "AGGREGATE")...

[mmatczuk]

I opened some issues please address them first.
There is plenty of comments but what stands out is the following:

• Panics vs error handling
• Types types types, each const value must be assigned to a type and exported.
• Handling type unmarshalling and unmarshalling errors, implement text.Marshaller and text.Unmarshaller interfaces for the types